[ASP.NET Core] Do not reset baggage on request stop event#4274
Conversation
vishweshbankwar
changed the title
| var textMapPropagator = Propagators.DefaultTextMapPropagator; | ||
| if (textMapPropagator is not TraceContextPropagator) | ||
| { | ||
| Baggage.Current = default; |
There was a problem hiding this comment.
should this be done at SDK side instead? Could you run stress tests to confirm there are no leaks if we do not remove it at all.
There was a problem hiding this comment.
I did run stress test. Did not see any leaks.
There was a problem hiding this comment.
should this be done at SDK side instead?
Are you referring to OnEnd in SDK? I have not looked in to that but just thinking it will impact all activities as we need to first check the activity ending is ASP.NET Core one.
There was a problem hiding this comment.
Baggage should be independent of any activity, so why we need to check which activity is being stopped?
There was a problem hiding this comment.
Only for ASP.NET Core activity. For other one's we do not do anything right now as well.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4274 +/- ##
==========================================
- Coverage 83.32% 83.30% -0.03%
==========================================
Files 314 314
Lines 12525 12522 -3
==========================================
- Hits 10437 10431 -6
- Misses 2088 2091 +3
|
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
github-actions
bot
added
the
Stale
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
github-actions
bot
removed
the
Stale
|
|
||
| [Fact] | ||
| public async Task BaggageClearedWhenActivityStopped() | ||
| public async Task BaggageIsNotClearedWhenActivityStopped() |
There was a problem hiding this comment.
Not a blocking comment... but if the instrumentation had never had this behavior, then we likely would never had this test in the first place. Perhaps we just remove this test completely?
Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com>
|
/easycla |
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
github-actions
bot
added
the
Stale
5 participants
Fixes #2992
Follow up #4274 (comment)
Ran the stress test using bombardier tool command
bombardier-windows-amd64.exe -d 12h -r 50 --header="baggage: key=value" <url>Monitored the process using
dotnet-counters monitorfor any memory leakssnippets after ~10 hr run
With this change
With main
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes