Instrument the OTel trace API's auto SDK #2001
Conversation
MrAlias
force-pushed
the
nonrecording-trace-api-sdk
branch
2 times, most recently
from
2f5c92a to
d650cf5
Compare
MrAlias
force-pushed
the
nonrecording-trace-api-sdk
branch
3 times, most recently
from
5a0200f to
2ea1657
Compare
MrAlias
force-pushed
the
nonrecording-trace-api-sdk
branch
from
2ea1657 to
0933d79
Compare
There was a problem hiding this comment.
overall, looks great.
wonder if we can share a lot of this between the auto/sdk probe and this new one in:
- C code - we can have a common header with the common functionality.
probe.goalso has common logic of parsing the perf events.- e2e tests can be unified.
There was a problem hiding this comment.
This looks very similar to the autosdk I think we should consider to unify them to one test where we could check both functionalities.
There was a problem hiding this comment.
If we want to combine these tests, it will take a lot of careful redesign of our testing system and (bats) utilities. These are testing independent workflows for independent code-paths that originate based on different mechanism. If we want to unify this effort, I would say we add this complexity after this PR is merged.
| @@ -87,6 +88,7 @@ func NewInstrumentation(ctx context.Context, opts ...InstrumentationOption) (*In | |||
| kafkaProducer.New(c.logger, Version()), | |||
| kafkaConsumer.New(c.logger, Version()), | |||
| autosdk.New(c.logger), | |||
| otelTrace.New(c.logger), | |||
There was a problem hiding this comment.
Don't we want this to be opt-in same as we have below the check for
if c.globalImpl {
p = append(p, otelTraceGlobal.New(c.logger))
}I think it should be the same opt-in flag - or do we want to remove that flag entirely and have this on by default (both the global and the non-recording)
There was a problem hiding this comment.
I think we can remove the flag. It was originally intended to set user expectations about our initial partial support for the OTel tracing API. At this point we provide full coverage of the API (for recent versions of otel-go).
I can track the removal of this opt-in flag in an issue an tackle it in a follow-on PR.
| if (wrote_flag) { | ||
| // Already wrote flag value. | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
This is kind of an invalid state - if we already wrote we should have been unloaded I guess.
It also can take some time for the unload to happen so this covers this, right?
There was a problem hiding this comment.
It also can take some time for the unload to happen so this covers this, right?
Right, this is my thought.
There was a problem hiding this comment.
FWIW, this is how the auto/sdk probe also handles this.
| // TODO: tune this. This is a just a guess, but we should be able to determine | ||
| // the maximum size of a span (based on limits) and set this. Ideally, we could | ||
| // also look into a tiered allocation strategy so we do not over allocate | ||
| // space (i.e. small, medium, large data sizes). | ||
| #define MAX_SIZE 2048 | ||
|
|
||
| // Injected const. | ||
| volatile const u64 span_context_trace_id_pos; | ||
| volatile const u64 span_context_span_id_pos; | ||
| volatile const u64 span_context_trace_flags_pos; |
There was a problem hiding this comment.
we could share this in a header with auto/sdk probe that uses the exact same consts.
There was a problem hiding this comment.
I'm on board if you would like to refactor this after this PR is merged. I would prefer to keep the complexity of this PR to a minimum and making changes to other probes to support a new header is not something I want to include here.
There was a problem hiding this comment.
SIG meeting feedback. Refactor this PR to handle dedup.
Part of #974
Depends on open-telemetry/opentelemetry-go#6456.