Skip to content
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

[wcf] Add AspNet parent span correction #1342

Merged
merged 12 commits into from
Sep 26, 2023

Conversation

repl-chris
Copy link
Contributor

@repl-chris repl-chris commented Sep 6, 2023

Fixes #1243

Changes

Execution context does not flow from ASP.NET to WCF, so when ASP.NET instrumentation is enabled the WCF span ends up being a sibling of the ASP.NET span, rather than a child of it. This PR hooks into the ASP.NET instrumentation and overrides the incoming request headers with the ASP.NET span, which forces it to propagate into WCF so we get the correct parent. Unfortunately, in order to do this it required reflection of a non-public property (NameValueCollection.IsReadOnly)...it's not ideal but it's the only way I could get it to work, and it will degrade gracefully back to the current behavior if it ever changes (but really, this is only for netframework so it won't ever change)

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@repl-chris repl-chris requested a review from a team September 6, 2023 22:13
@github-actions github-actions bot requested a review from CodeBlanch September 6, 2023 22:13
@utpilla utpilla added the comp:instrumentation.wcf Things related to OpenTelemetry.Instrumentation.Wcf label Sep 6, 2023
@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #1342 (a22ba2b) into main (71655ce) will increase coverage by 5.87%.
Report is 13 commits behind head on main.
The diff coverage is 72.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1342      +/-   ##
==========================================
+ Coverage   73.91%   79.78%   +5.87%     
==========================================
  Files         267      174      -93     
  Lines        9615     4967    -4648     
==========================================
- Hits         7107     3963    -3144     
+ Misses       2508     1004    -1504     
Flag Coverage Δ
unittests 79.87% <41.17%> (?)
unittests-Instrumentation.Wcf 79.20% <81.35%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...searchClient/ElasticsearchClientInstrumentation.cs 100.00% <100.00%> (ø)
...ityFrameworkCore/EntityFrameworkInstrumentation.cs 100.00% <100.00%> (ø)
...ation/EntityFrameworkInstrumentationEventSource.cs 12.00% <100.00%> (ø)
...Instrumentation.Quartz/QuartzJobInstrumentation.cs 83.33% <100.00%> (+1.51%) ⬆️
...rumentation.Wcf/TracerProviderBuilderExtensions.cs 88.88% <100.00%> (+1.38%) ⬆️
...ry.ResourceDetectors.AWS/AWSEBSResourceDetector.cs 92.00% <ø> (+0.69%) ⬆️
...ntation/ElasticsearchInstrumentationEventSource.cs 7.69% <16.66%> (-4.81%) ⬇️
...Implementation/QuartzInstrumentationEventSource.cs 33.33% <0.00%> (-16.67%) ⬇️
...cf/Implementation/WcfInstrumentationEventSource.cs 21.73% <0.00%> (-6.04%) ⬇️
...on.Wcf/Implementation/AspNetParentSpanCorrector.cs 88.67% <88.67%> (ø)

... and 109 files with indirect coverage changes

examples/wcf/server-aspnetframework/Global.asax.cs Outdated Show resolved Hide resolved
examples/wcf/server-aspnetframework/stylecop.json Outdated Show resolved Hide resolved
Comment on lines 40 to 41
private const string TelemetryHttpModuleTypeName = "OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule, OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule, PublicKeyToken=7bd6737fe5b67e3c";
private const string TelemetryHttpModuleOptionsTypeName = "OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModuleOptions, OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule, PublicKeyToken=7bd6737fe5b67e3c";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I remember usually we handle two cases (at least for internal tests visiblity) - with and withoud public key. Depending on library has this key or no.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't actually find anywhere referencing other assemblies using their strong name, so I've updated this one to be consistent with the others and just not include the PublicKeyToken at all.

@repl-chris
Copy link
Contributor Author

@Kielek I have updated the new example to the new csproj format, and have addressed all other review comments. 👍

@Kielek
Copy link
Contributor

Kielek commented Sep 15, 2023

@CodeBlanch. could you pleas also review?

@Kielek
Copy link
Contributor

Kielek commented Sep 15, 2023

@repl-chris, question not related to this PR. You are havilly working on WCF instrumentation, it is great, any chance that you help with enabling nullable feature? See #894

Separate PR.

@repl-chris
Copy link
Contributor Author

@repl-chris, question not related to this PR. You are havilly working on WCF instrumentation, it is great, any chance that you help with enabling nullable feature? See #894

Separate PR.

yeah, I can probably do the WCF-related projects 👍

Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CodeBlanch, @lachmatt, LGTM, could you please check it also?

@lachmatt
Copy link
Contributor

LGTM

@CodeBlanch CodeBlanch changed the title Add AspNet parent span correction [wcf] Add AspNet parent span correction Sep 26, 2023
@github-actions github-actions bot requested a review from CodeBlanch September 26, 2023 18:05
Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@CodeBlanch
Copy link
Member

@repl-chris I don't see a CHANGELOG update would you mind adding a note in there and then I think we are good to merge.

@github-actions github-actions bot requested a review from CodeBlanch September 26, 2023 19:32
@repl-chris
Copy link
Contributor Author

@CodeBlanch Added CHANGELOG, good to go 👍

@CodeBlanch CodeBlanch merged commit fec6a63 into open-telemetry:main Sep 26, 2023
25 checks passed
@repl-chris repl-chris deleted the AspNetParentSpans branch September 26, 2023 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.wcf Things related to OpenTelemetry.Instrumentation.Wcf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parent Activity does not propagate from ASP.NET host to WCF service
5 participants