Fix OpenTracing shim under legacy AspNetCore activities #3506
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3506 +/- ##
==========================================
+ Coverage 86.67% 86.72% +0.04%
==========================================
Files 275 275
Lines 9958 9950 -8
==========================================
- Hits 8631 8629 -2
+ Misses 1327 1321 -6
|
|
@open-telemetry/dotnet-approvers I'm planning to join the SIG meeting today in case you have any Q about this PR. |
Can't make the SIG, but we're running a local fork with this PR and it's resolved the broken trace issues we previously had when using the shim. |
There was a problem hiding this comment.
Approving since this PR seems to take things in the right direction. It might even resolve #2257.
Though there's a lot about this shim that seems kinda rough. For starters, it wraps the OpenTelemetry shim API which in turn wraps the Activity API. Probably would make more sense to use the Activity API directly.
| @@ -330,5 +330,29 @@ public void Start() | |||
| Assert.NotNull(span); | |||
| Assert.Equal("foo", span.Span.Activity.OperationName); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void Start_UnderAspNetCoreInstrumentation() | |||
There was a problem hiding this comment.
I think this test would have passed before this PR. Is it testing a scenario that was not already covered?
There was a problem hiding this comment.
The old code fails on the assert at line 350
There was a problem hiding this comment.
also on 355 if check of 350 was removed.
Yeah, I think that reviewing the whole shim seems a good idea. |
Changes
Fixes OpenTracing spans under the legacy Activity "Microsoft.AspNetCore.Hosting.HttpRequestIn" (enabled by AspNetCore instrumentation package). The OpenTracing spans under the legacy Activity can be missing, having broken parent/active span relationships and cause the premature termination of the legacy activity.
The special handling for "Microsoft.AspNetCore.Hosting.HttpRequestIn" on the OpenTracing shim was present since the first version of the shim, see #197. It got its current behavior on #994. However, that behavior makes the first OT span under the legacy activity to not be created, the legacy activity to be stopped where the first OT span finishes, and any subsequent OT span to be created under the context of the legacy activity parent (which typically means creating a new trace).
The OpenTracing spans should be integrated with OpenTelemetry traces, consider the case of third-party libraries instrumented with OpenTracing. The fix simply consists in removing the special case for "Microsoft.AspNetCore.Hosting.HttpRequestIn".
CHANGELOG.mdupdated for non-trivial changes