ref: Add support for custom sampling context to span first#5628
ref: Add support for custom sampling context to span first#5628sentrivana wants to merge 1 commit intoivana/span-first-13-testsfrom
Conversation
Codecov Results 📊✅ 5 passed | Total: 5 | Pass Rate: 100% | Execution Time: 1.08s All tests are passing successfully. ❌ Patch coverage is 42.86%. Project has 15841 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.
| } | ||
|
|
||
| if propagation_context.custom_sampling_context: | ||
| sampling_context.update(propagation_context.custom_sampling_context) |
There was a problem hiding this comment.
Custom sampling context can silently override built-in keys
Medium Severity
Using sampling_context.update(propagation_context.custom_sampling_context) allows user-provided custom sampling context to silently override built-in keys like name, trace_id, parent_span_id, parent_sampled, and attributes. Unlike the old start_transaction path where the built-in keys were transaction_context and parent_sampled (uncommon names), keys like name and attributes are very common and likely to be used innocuously by someone calling scope.set_custom_sampling_context({"name": "my_feature"}), resulting in a corrupted sampling context reaching the traces_sampler.
| 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?
| ... | ||
|
|
||
|
|
||
| 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?
2 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.