This repository was archived by the owner on Jun 10, 2020. It is now read-only.
Support W3C incoming headers in opt-in mode - #735
Merged
Merged
Conversation
Liudmila Molkova (lmolkova)
requested review from
Sergey Kanzhelev (SergeyKanzhelev) and
Cijo Thomas (cijothomas)
August 7, 2018 17:29
Liudmila Molkova (lmolkova)
force-pushed
the
lmolkova/supportW3COnCore
branch
from
August 13, 2018 21:52
89367dc to
4d043ee
Compare
Liudmila Molkova (lmolkova)
force-pushed
the
lmolkova/supportW3COnCore
branch
from
August 16, 2018 19:05
4d043ee to
5fdfa9f
Compare
added 6 commits
August 16, 2018 12:05
Author
|
ready for review Cijo Thomas (@cijothomas) ;) |
Cijo Thomas (cijothomas)
approved these changes
Aug 21, 2018
| <PackageReference Include="System.Collections.Immutable.Analyzers" Version="1.2.0-beta2"> | ||
| <PrivateAssets>All</PrivateAssets> | ||
| </PackageReference> | ||
| <PackageReference Include="System.Diagnostics.DiagnosticSource" Version="4.5.0" /> |
Contributor
There was a problem hiding this comment.
2.4.1 -> to 2.5.0-beta1 - please update this as well.
| // W3C | ||
| if (this.enableW3CHeaders) | ||
| { | ||
| SetW3CContext(httpContext.Request.Headers, activity, out sourceAppId); |
Contributor
There was a problem hiding this comment.
can we log all the header values in Eventsource with Verbose logging level? It'll help with support tickets.
There was a problem hiding this comment.
I like this idea, I also wonder if it is secure?
Contributor
There was a problem hiding this comment.
AS discussed, lets not do it now. We can ask Cx to add TI to dump headers, if a need arises.
| } | ||
|
|
||
| [Fact] | ||
| public void OnHttpRequestInStartWithW3CHeadersAndRequestIdIsTrackedCorrectly() |
Contributor
There was a problem hiding this comment.
rename unit test to indicate OnBeginRequest bein tested
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
Implements w3c distributed tracing standard.
This change enables opt-in W3C support via settings.
To enable it, set
ApplicationInsightsServiceOptions.RequestCollectionOptions.EnableW3CDistributedTracingto true and provide options to theAddApplicationInsights telemetryThis change ensures W3C ids are set on the telemetry (operationId, parentId and Id). 'legacy' ids are set in custom dimensions whenever they are different from the W3C ones.
This change requires UI queries change to support querying for custom dimensions for the cases when operation id or parent id do not match on the legacy-W3C boundary.
This PR depends on changes in DependencyCollector that will be shipped in 2.8.0-beta1 (microsoft/ApplicationInsights-dotnet-server#945) and cannot be merged until that. Sharing it to get the early feedback.Cijo Thomas (@cijothomas) Reiley Yang (@reyang) Sergey Kanzhelev (@SergeyKanzhelev) please review