AspNet HttpModule: Fix bug when adding to web.config #584
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
… Currently, going to the root '/' causes both the DatadogModule and the CustomLoggingModule to run twice.
…t and still operate correctly. Only creates the trace on the first OnBegin and ends the trace on the last OnEnd or OnError. Small fix Another small fix Last small fix OMG last small fix I swear
…n simultaneously handle adding the module both statically and dynamically.
…rst HttpModule invoked and only respond to the callbacks with that object instance.
zacharycmontoya
added
type:bug
area:automatic-instrumentation
bobuva
reviewed
Dec 10, 2019
| @@ -72,6 +74,13 @@ private void OnBeginRequest(object sender, EventArgs eventArgs) | |||
| return; | |||
| } | |||
|
|
|||
| // Only one HttpModule will respond to ASP.NET lifecycle events | |||
| // Do nothing if another HttpModule has already been run | |||
| if (httpContext.Items[_httpContextOwningModuleKey] is TracingHttpModule) | |||
There was a problem hiding this comment.
I assume this accounts for it not being in the Items list?
There was a problem hiding this comment.
That's correct. If the key is not present, then the Items lookup will return null, and a null value will cause the is check to return false. I thought to follow the previous convention with this style of check, but would there be another way to express this concisely?
…t handlers for one TracingHttpModule per HttpApplication object.
lucaspimentel
previously approved these changes
Dec 10, 2019
lucaspimentel
approved these changes
Dec 10, 2019
zacharycmontoya
deleted the
zach/feature/aspnet-httpmodule-resiliency-final
branch
MikeGoldsmith
pushed a commit
to lightstep/ls-trace-dotnet
that referenced
this pull request
Mar 20, 2020
The previous implementation of the HttpModule did not behave correctly when it was invoked more than once in one ASP.NET request. Multiple scopes would be created and only the innermost one would be closed, so the root span would not be closed and the Datadog UI would show no traces. The new implementation registers the first HttpModule to hit the event handlers as the only one allowed to create/modify Scope objects, so only one scope is created and closed. This fix is important to support customers who have other HttpModules in their system web.config that subscribe to the BeginRequest and would like our Tracer to start spans before hitting them. As a result, the customer will enter our HttpModule before the other HttpModule's in their web.config which causes our the ASP.NET application to create and invoke two instances of our HttpModule: once from the web.config declaration and once from the dynamic module after referencing our NuGet package.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Changes proposed in this pull request:
The previous implementation of the HttpModule did not behave correctly when it was invoked more than once in one ASP.NET request. Multiple scopes would be created and only the innermost one would be closed, so the root span would not be closed and the Datadog UI would show no traces. The new implementation registers the first HttpModule to hit the event handlers as the only one allowed to create/modify
Scopeobjects, so only one scope is created and closed.This fix is important to support customers who have other HttpModules in their system
web.configthat subscribe to theBeginRequestand would like our Tracer to start spans before hitting them. As a result, the customer will enter our HttpModule before the other HttpModule's in theirweb.configwhich causes our the ASP.NET application to create and invoke two instances of our HttpModule: once from theweb.configdeclaration and once from the dynamic module after referencing our NuGet package.@DataDog/apm-dotnet