Remove the static collection in the HTTP module#1498
Conversation
andrewlock
left a comment
andrewlock
left a comment
There was a problem hiding this comment.
Hopefully this will give us more defence in support calls!
Not directly related to your changes, but should we explicitly clear the context item here too, incase the SpanContextPropagator.Instance.Inject() call fails and throws?
| scope.Dispose(); | ||
|
|
||
| // Clear the context to make sure another TracingHttpModule doesn't try to close the same scope | ||
| app.Context.Items.Remove(_httpContextScopeKey); |
There was a problem hiding this comment.
Is it worth putting this in a finally block so we're sure it's always removed?
There was a problem hiding this comment.
I don't know. I thought about it, but since scope.Dispose() is the last operation, I don't think that's an issue if an exception is thrown and another module redo the same processing. But I don't mind adding a finally block, whatever makes you feel better 😃
I guess I should move the line |
lucaspimentel
added
area:automatic-instrumentation
4 participants
The collection was there to make sure only one scope is created when the module is registered multiple times. This collection can be source of concerns for customers who see it as a GC root holding large amounts of memory. We can achieve the same results by checking the state stored in the HttpContext and this will clear any confusion.