Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

HttpHandlerDiagnosticListener did not send DiagnosticSource Stop event on netfx in W3C mode - #40777

Merged
lmolkova merged 1 commit into
dotnet:masterfrom
lmolkova:master
Sep 4, 2019
Merged

lmolkova merged 1 commit into
dotnet:masterfrom
lmolkova:master

Conversation

@lmolkova

@lmolkova lmolkova commented Sep 3, 2019

Copy link
Copy Markdown

HttpHandlerDiagnosticListener monkey-patches WebRequest to enable tracing with DiagnosticSource.

HttpHandlerDiagnosticListener uses presence of tracing headers to determine if a request was initially instrumented (Activity.Current which is AsyncLocal does not survive in WebRequest callbacks). Based on header presence, when response tarts, it notifies a listener with 'Stop' callback.

In #35882 it started to support W3C trace-context and now Stop event is not sent if a new W3C traceparent is present, but old Request-Id is not.

This issue only reproduces when tracing is on and W3C mode for Activity is on. By default tracing is off and no events are sent. When tracing is on, default Activity format is the old one, that works with Request-Id header, so problem does not repro.

Tracing tools that enable W3C, can workaround it with adding Request-Id header themselves.

@tommcdon

tommcdon commented Sep 3, 2019

Copy link
Copy Markdown
Member

@sywhang @noahfalk

@tommcdon tommcdon added this to the 5.0 milestone Sep 3, 2019
@davidsh

davidsh commented Sep 3, 2019

Copy link
Copy Markdown
Contributor

You should run Outerloop tests before final merge especially since the tests affected are Outerloop. And each new commit to a PR does not kick off a new Outerloop run. So, you need to kick off Outerloop manually each time with "/azp run"

@davidsh

davidsh commented Sep 3, 2019

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 4 pipeline(s).

@sywhang sywhang left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@lmolkova

lmolkova commented Sep 4, 2019

Copy link
Copy Markdown
Author

linux outerloop test failures are not related to this change.

@sywhang, @davidsh Thanks for review and triggering the tests

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants