Lost Baggage: Fix TelemetryHttpModule losing Baggage when switching from native to managed threads #2314
Conversation
| @@ -291,9 +291,8 @@ private static void HookOrProcessResult(HttpWebRequest request) | |||
|
|
|||
| private static void ProcessResult(IAsyncResult asyncResult, AsyncCallback asyncCallback, Activity activity, object result, bool forceResponseCopy) | |||
| { | |||
| // We could be executing on a different thread now so set the activity. | |||
| Debug.Assert(Activity.Current == null || Activity.Current == activity, "There was an unexpected active Activity on the result thread."); | |||
There was a problem hiding this comment.
When Activity is lost (more precisely, ExecutionContext is lost) in the HttpModule we restore the root (HttpIn) Activity. That makes this assert invalid. I tried to fix the HttpModule so that it restores the Activity that was last running, but it is impossible to retrieve do to the way ExecutionContext works. It isn't an issue to remove the assert, but it is unnerving. Any instrumentation running in IIS reliant on Activity.Current could run into trouble.
Codecov Report
@@ Coverage Diff @@
## main #2314 +/- ##
==========================================
+ Coverage 81.03% 81.06% +0.02%
==========================================
Files 228 228
Lines 7289 7305 +16
==========================================
+ Hits 5907 5922 +15
- Misses 1382 1383 +1
|
Would a non-abstract version work? e.g. the default |
It would yes. Something like this on the base... But it isn't as nice, IMO. Because then callers have to know about the throw behavior and put in a catch. For non-overridden implementations that evaluation will always be a throw, which is very expensive. I think the interface test ( I got the idea from ISupportExternalScope which I think was added after the logging API was introduced as a way to opt-into externally managed scopes in a non-breaking way. |
| { | ||
| Task.Yield(); | ||
|
|
||
| Assert.Null(Activity.Current); |
There was a problem hiding this comment.
@stephentoub Activity.Current uses an AsyncLocal. Occasionally this test fails only on net461. Can you Miyagi me on this and point me in the right direction? There is a similar test right above which I haven't seen fail.
What I'm trying to do is get some code running on a thread where ExecutionContext has no local values so I can verify my code properly restores things when we run into that situation. We see this in the wild when running on IIS and it decides to jump threads.
There was a problem hiding this comment.
@GrabYourPitchforks Hey anything jump out at you on this test? Sorry to bug, I'm scratching my head on this one.
Changes
TelemetryHttpModule will now restore Baggage in addition to Activity when it detects that it has been lost.
Public API Changes
Added
IRuntimeContextSlotValueAccessorso I could expose a non-dynamic method for accessing the value stored in aRuntimeContextSlot<T>as an object. I went with the interface because I think addingabstract object Value { get; set ;}onRuntimeContextSlot<T>would be breaking.AsyncLocalRuntimeContextSlot<T>,RemotingRuntimeContextSlot<T>, &ThreadLocalRuntimeContextSlot<T>now implementIRuntimeContextSlotValueAccessor.TODOs
CHANGELOG.mdupdated for non-trivial changes