OpenTelemetry tracing bridge #1410
Conversation
|
Some areas are unfortunately impossible to test (setting tags on spans/transactions for example), because the data isn't readable within the test. |
|
I may have missed some things, but it's hard to say without real world usage. So I am marking this PR as ready for review. |
module/apmotel/span.go
Outdated
| haveHTTPContext = true | ||
| haveHTTPHostTag = true | ||
| httpHost = v.Value.Emit() | ||
|
|
There was a problem hiding this comment.
The previous attempt at the Otel bridge used a lot more attribute settings, maybe that's useful? https://github.com/elastic/apm-agent-go/pull/1203/files#diff-4bd41254646c6c5baf269f5e8128d6f7d327bd52ee17256ccedfe075bd5de1fdR70-R167
There was a problem hiding this comment.
It seems trickier than that. The logic between the one here (from apmot) and in that branch are quite different.
Some attributes are added, and others are removed.
I wonder if we need a spec to have a consistent behavior, as every language implementation is going to have this problem.
There was a problem hiding this comment.
I wonder too. @felixbarny would you happen to know?
There was a problem hiding this comment.
We don't have to map all attributes but we have to map some. We have a cross-agent spec for that: https://github.com/elastic/apm/blob/main/specs/agents/tracing-api-otel.md#attributes-mapping
There was a problem hiding this comment.
I have updated the attributes based on the mentioned spec, while also keeping the http and db context, which aren't specified.
Co-authored-by: Marc Lopez Rubio <[email protected]>
Co-authored-by: Marc Lopez Rubio <[email protected]>
Co-authored-by: Marc Lopez Rubio <[email protected]>
Co-authored-by: Marc Lopez Rubio <[email protected]>
Co-authored-by: Marc Lopez Rubio <[email protected]>
dmathieu
force-pushed
the
otel-tracer
branch
2 times, most recently
from
5c0aff6 to
d9803ed
Compare
| case "messaging.system": | ||
| s.span.Type = "messaging" | ||
| s.span.Subtype = v.Value.Emit() | ||
| serviceTargetType = v.Value.Emit() |
There was a problem hiding this comment.
Looking at the spec, looks like we're not handling the messaging.url attribute:
type = 'messaging';
subtype = a['messaging.system'];
if (!netName && a['messaging.url']) {
netName = parseNetName(a['messaging.url']);
}
serviceTargetType = subtype;
serviceTargetName = a['messaging.destination'] || null;There was a problem hiding this comment.
Yeah. The spec is weird about that. netName is never used with messaging. And with an if/else case as they have the net case would never run if the messaging one does.
This section therefore seems like dead code. I've written a note to follow up on figuring out if the spec should be fixed, but didn't get to it yet.
There was a problem hiding this comment.
After talking with @SylvainJuge about it, I have opened a spec fix: elastic/apm#787
This setups a bridge (our own SDK actually) which allows us to use the OpenTelemetry APIs alongside our agent.