ref: Add support for custom sampling context to span first (14)#5628
ref: Add support for custom sampling context to span first (14)#5628sentrivana merged 96 commits intomasterfrom
Conversation
…first-6-add-continue-and-new-trace
…na/span-first-7-add-trace-decorator
…-first-8-bucket-based-limits-in-batcher
…-first-8-bucket-based-limits-in-batcher
Codecov Results 📊✅ 13 passed | Total: 13 | Pass Rate: 100% | Execution Time: 7.82s All tests are passing successfully. ❌ Patch coverage is 42.86%. Project has 14011 uncovered lines. Files with missing lines (2)
Generated by Codecov Action |
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. This PR will not appear in the changelog. 🤖 This preview updates automatically when you update the PR. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| def _set_custom_sampling_context( | ||
| self, custom_sampling_context: "dict[str, Any]" | ||
| ) -> None: | ||
| self.custom_sampling_context = custom_sampling_context |
There was a problem hiding this comment.
Related to the comment Cursor made - should we consider adding validation to the value that's going be set on the custom_sampling_context?
There was a problem hiding this comment.
Ideally you could set whatever you want on the custom sampling context, so I wouldn't validate. But mistakenly overriding existing keys is a concern. I'm thinking we could put the keys set by the SDK into their own subdictionary -- we're also doing this in the old code path with transaction_context. Just need to think of a nice new name since there are no transactions anymore in span first.
There was a problem hiding this comment.
Maybe just span? Or span_context? Or just context? Naming is hard.
There was a problem hiding this comment.
Maybe span_context is actually the way to go -- very unique, so unlikely to collide with anything user-set, and similar to transaction_context so there's some continuity?
Went with that here
There was a problem hiding this comment.
I agree, naming is hard 😅
I think span_context is a great name 👍🏻
tests/tracing/test_span_streaming.py
Outdated
| ... | ||
|
|
||
|
|
||
| def test_new_custom_sampling_context(sentry_init): |
There was a problem hiding this comment.
I think both of the test cases introduced here are good, but I think the names could be improved slightly to make it easier to understand (in terms of what broke) should they ever fail.
For this particular test, maybe something like test_custom_sampling_context_update_to_context_value_persists could work?
sentrivana
changed the title
3 participants
Custom sampling context allows folks to have arbitrary data accessible in the
traces_samplerin order to make a sampling decision. The SDK sets custom sampling context as well in some integrations, for example, in ASGI frameworks the ASGI scope will be available.Previously, you could provide custom sampling context as an argument to the
start_spanfunction. In the spirit of keeping the newstart_spanAPI minimal, we'll be movingcustom_sampling_contextto the propagation context and providing a dedicated API function to set it. In this PR, it's a scope method (scope.set_custom_sampling_context()). We can (and probably should) promote it to top-level API at some point in the future.