-
Notifications
You must be signed in to change notification settings - Fork 760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Runtime context API #948
Runtime context API #948
Conversation
Codecov Report
@@ Coverage Diff @@
## master #948 +/- ##
==========================================
- Coverage 68.74% 68.17% -0.57%
==========================================
Files 213 218 +5
Lines 5912 5955 +43
Branches 967 969 +2
==========================================
- Hits 4064 4060 -4
- Misses 1580 1627 +47
Partials 268 268
|
I'm not going to lie, this is kind of a 🤯 The second part, where we're using it, looks great. The first part though, for us mere mortals, why would we (or the user hosting us?) want to change the |
There are many scenarios where big companies/customers have their own context management, especially when they have interop story with native code, or they have interaction with another runtime/component that has different context management model). I would expect 99% of the users to just use it as a "context aware" dictionary, only 1% would need to specify a non-default implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We already have CorrelationContext & DistributedContext so I am a bit worried about just general confusion arising from having three "context" things. Maybe the "ContextSlot" classes should all be named "RuntimeContextSlot" as a hint to their relationship?
The
That's a good point, let me rename them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is a fairly advanced scenario but makes sense for those running an older version of .NET and/or require a customisable solution.
@reyang this is great! Thanks for getting this started.
New Relic's .NET agent uses a number of storage mechanisms to flow context, and we have a similar abstraction to what this PR contains. We support the mechanisms in the PR: General questions:
I'm not sure there are any easier answers here, though I am curious, @reyang, if you have encountered any issues with your experience on OpenCensus? |
Not in OpenCensus .NET/C# as I only worked on OpenCensus Python. |
Ah, thank you for drawing my attention closer to this. This is a nice way of handling things.
👍 cool, yea that makes sense One more thing to highlight just so that it's in peoples' minds is that using |
|
Interesting! I wasn't aware that Python as a notion of async local. I suppose the .NET use case would be similar in that it can be nice at times to be able to interrupt the "magic" of async local and prevent it from flowing context where you don't want it to flow - like dedicated background worker threads. |
It is called |
Fixes #.
Changes
Trying to build a foundation for us to address #893 (comment) and the 2nd point of #809 (comment).
I haven't got time to work on the
Apply
andSnapshot
part which I've commented out in the PR (and leave them for another PR). These are required for writing test cases. I haven't got time to write test cases but here goes how to use it:And to give some idea on how we can suppress instrumentation from the exporter:
Please provide a brief description of the changes here. Update the
CHANGELOG.md
for non-trivial changes.For significant contributions please make sure you have completed the following items: