-
Notifications
You must be signed in to change notification settings - Fork 215
Improve OTel bridge compatibility with existing Azure instrumentation #2455
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
81c9681 to
7854d2d
Compare
Mpdreamz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| // This simply ensures our transaction activity is always created. | ||
| internal static readonly ActivityListener Listener = new() | ||
| { | ||
| ShouldListenTo = s => s.Name == ElasticApmActivitySource.Name, | ||
| Sample = (ref ActivityCreationOptions<ActivityContext> _) => ActivitySamplingResult.AllData | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevejgordon @Mpdreamz Sorry to revive this, but the Listener is never registered via ActivitySource.AddActivityListener(Listener);, so the intended behavior (per the comment) isn't actually correct.
This issue (activities not being created) is hidden when OpenTelemetryBridgeEnabled is true (the default), since ElasticActivityListener registers a listener for all ActivitySources. But if the bridge is disabled then no activity is created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for highlighting this @aheubusch. Is this manifesting in an error or issue for you? I think the intent was focused on the scenario where the bridge is in use, but it's complex code and there may be knock on effects we didn't consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No error or issue in my application (bridge is enabled). One of my unit tests (bridge is disabled) started failing after an update from 1.28 to 1.34 because it assumed that StartTransaction always creates an Activity (except when ignoreActivity=true).
In general this change in behavior appears to be unintentional, so I wanted to raise it.
This ensures that the Azure SDK instrumentation doesn't use the existing
Activityspan ID, which, when combined with the OTel bridge, can lead to spans being parented by themselves.This also enhances the logic of the OTel bridge so that we skip span creation for activities coming from the Azure SDK libraries if our Azure instrumentation assemblies are loaded. This ensures we don't create essentially repeated child spans.
Fixes #2452