This repository has been archived by the owner on Jun 10, 2020. It is now read-only.
Support W3C incoming headers in opt-in mode #735
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
lmolkova
force-pushed
the
lmolkova/supportW3COnCore
branch
from
89367dc
to
4d043ee
Compare
lmolkova
force-pushed
the
lmolkova/supportW3COnCore
branch
from
4d043ee
to
5fdfa9f
Compare
lmolkova
changed the title
added 6 commits
August 16, 2018 12:05
…oft/ApplicationInsights-aspnetcore into lmolkova/supportW3COnCore
|
ready for review @cijothomas ;) |
cijothomas
approved these changes
Aug 21, 2018
| @@ -65,6 +65,7 @@ | |||
| <PackageReference Include="System.Collections.Immutable.Analyzers" Version="1.2.0-beta2"> | |||
| <PrivateAssets>All</PrivateAssets> | |||
| </PackageReference> | |||
| <PackageReference Include="System.Diagnostics.DiagnosticSource" Version="4.5.0" /> | |||
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); |
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?
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() |
There was a problem hiding this comment.
rename unit test to indicate OnBeginRequest bein tested
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.
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.@cijothomas @reyang @SergeyKanzhelev please review