This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Inject W3C headers in netfx HTTP diagnostics hook #35880
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@vancem could you please have a look? |
stephentoub
reviewed
Mar 15, 2019
| if (e.MoveNext()) | ||
| // do not inject header if it was injected already | ||
| // perhaps tracing systems wants to override it | ||
| if (request.Headers[TraceParentHeaderName] == null) |
There was a problem hiding this comment.
Nit: Above the code uses request.Headers.Get(id) and here it's using request.Headers[id]. IIRC, one just calls the other, but is there a reason for the different access pattern?
There was a problem hiding this comment.
no, no particular reason, thanks for noticing it
stephentoub
reviewed
Mar 15, 2019
| StringBuilder baggage = new StringBuilder(); | ||
| do | ||
| request.Headers.Add(TraceParentHeaderName, activity.Id); | ||
| if (activity.TraceStateString != null) |
There was a problem hiding this comment.
Accessing TraceStateString looks like an O(N) operation. Seems like it should be cached into a local rather than accessing it twice, once to check for null and the other to add it.
stephentoub
reviewed
Mar 15, 2019
| { | ||
| // do not inject header if it was injected already | ||
| // perhaps tracing systems wants to override it | ||
| if (request.Headers[RequestIdHeaderName] == null) |
There was a problem hiding this comment.
Nit: Same question/comment about Get vs indexer
vancem
approved these changes
Mar 15, 2019
macrogreg
pushed a commit
to open-telemetry/opentelemetry-dotnet-instrumentation
that referenced
this pull request
Sep 24, 2020
* Inject W3C headers in Http Desktop hook * do not inject headers if they were injected before * check that w3c headers are empty when format is Hierarchical * review comments Commit migrated from dotnet/corefx@dee28e2
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
* Inject W3C headers in Http Desktop hook * do not inject headers if they were injected before * check that w3c headers are empty when format is Hierarchical * review comments Commit migrated from dotnet/corefx@dee28e2
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depending on the Activity.IdFormat we want to inject W3C distributed tracing headers or Request-Id headers.
This change implements this and also prevents injection of any of the correlation headers if it was injected before e.g. by tracing system or explicitly in the app code.