Uh oh!
There was an error while loading. Please reload this page.
feat(dynamic-sampling): Add possibility to run dynamic sampling from sentry-relay - #2091
Conversation
sentry-relay| let sampling_config = serde_json::from_str::<SamplingConfig>((*sampling_config).as_str())?; | ||
| let root_sampling_config = | ||
| serde_json::from_str::<SamplingConfig>((*root_sampling_config).as_str())?; |
There was a problem hiding this comment.
I have tried to use the ? operator for curiosity and I don't understand why the compiler allows me to return a Result when the type is RelayStr.
| - Scrub sensitive keys (`passwd`, `token`, ...) in Replay recording data. ([#2034](https://github.com/getsentry/relay/pull/2034)) | ||
| - Add document_uri to csp filter. ([#2059](https://github.com/getsentry/relay/pull/2059)) | ||
| - Store `geo.subdivision` of the end user location. ([#2058](https://github.com/getsentry/relay/pull/2058)) | ||
| - Add new FFI function for running dynamic sampling. ([#2091](https://github.com/getsentry/relay/pull/2091)) |
There was a problem hiding this comment.
Please move this to py/CHANGELOG.md
| raise ValueError(error) | ||
| def run_dynamic_sampling(sampling_config, root_sampling_config, dsc, event): |
There was a problem hiding this comment.
Should we take an entire envelope as interface, instead? In Relay's pipeline, DS also reads information from other envelope headers.
There was a problem hiding this comment.
I wanted to keep it stupid simple for now, in case we want, we can extend it to have an envelope as well.
| return json.loads( | ||
| decode_str( | ||
| rustcall( | ||
| lib.run_dynamic_sampling, | ||
| encode_str(sampling_config), | ||
| encode_str(root_sampling_config), | ||
| encode_str(dsc), | ||
| encode_str(event), | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Please add free=True to decode_str, since the owned string returned by FFI needs to be released. You can make this more readable like so:
| returnjson.loads( | |
| decode_str( | |
| rustcall( | |
| lib.run_dynamic_sampling, | |
| encode_str(sampling_config), | |
| encode_str(root_sampling_config), | |
| encode_str(dsc), | |
| encode_str(event), | |
| ) | |
| ) | |
| ) | |
| result_json=rustcall( | |
| lib.run_dynamic_sampling, | |
| encode_str(sampling_config), | |
| encode_str(root_sampling_config), | |
| encode_str(dsc), | |
| encode_str(event), | |
| ) | |
| returnjson.loads(decode_str(result_json, free=True)) |
| assert str(e.value) == 'json atom at path ".foobar" is missing from rhs' | ||
| def test_run_dynamic_sampling_with_valid_params_and_match(): |
There was a problem hiding this comment.
These tests are quite elaborate and will have to change every time we change something about sampling internals, particularly because they check the contents of the returned rules. Should we instead make more general checks, such as assert "merged_sampling_configs" in result and leave the rest to the DS unit tests in Rust?
There was a problem hiding this comment.
That's a very good idea yes!
| use std::os::raw::c_char; | ||
| use std::slice; | ||
| use crate::core::{RelayBuf, RelayStr}; |
There was a problem hiding this comment.
Please order imports according to the style guide. This concerns the chrono and crate imports.
| serde_json::from_str::<SamplingConfig>((*root_sampling_config).as_str())?; | ||
| // We can optionally accept a dsc and event. | ||
| let dsc = serde_json::from_str::<DynamicSamplingContext>((*dsc).as_str()); | ||
| let event = serde_json::from_str::<EphemeralEvent>((*event).as_str()); |
There was a problem hiding this comment.
Instead of parsing some synthetic EphemeralEvent, you should just load an actual Annotated<Event> directly:
| let event = serde_json::from_str::<EphemeralEvent>((*event).as_str()); | |
| let event = Annotated::<Event>::from_json((*event).as_str())?; | |
| let event = event.value().ok_or(<some error>)?; |
| sampling_config: *const RelayStr, | ||
| root_sampling_config: *const RelayStr, | ||
| dsc: *const RelayStr, | ||
| event: *const RelayStr, |
There was a problem hiding this comment.
You can pass all of these as references and then don't have to worry about dereferencing the pointer below. This is something we've not done consistently in the codebase, but we should move towards:
| sampling_config:*constRelayStr, | |
| root_sampling_config:*constRelayStr, | |
| dsc:*constRelayStr, | |
| event:*constRelayStr, | |
| sampling_config:&RelayStr, | |
| root_sampling_config:&RelayStr, | |
| dsc:&RelayStr, | |
| event:&RelayStr, |
| use anyhow::Context; | ||
| use chrono::Utc; |
There was a problem hiding this comment.
Please move these two imports to the next import group (above once_cell).
* master: (178 commits) ref(crons): Allow `_` in monitor slugs (#2100) fix(span-metrics): Scrub subdomains in metric tags (#2103) feat(normalization): Scrub span description URLs (#2095) feat(dynamic-sampling): Add possibility to run dynamic sampling from `sentry-relay` (#2091) ref(dynamic-config): Remove transaction metrics allowlist from Sentry (#2092) feat(protocol): Support old effective-directory format (#2048) ref(spans): Add more tags to span metrics (#2090) fix(server): Support HTTP half-close connections (#2089) ref(spans): Use transactions namespace with `span` prefix (#2087) ref(spans): Update span metrics feature flag name (#2086) feat(metrics): Extract more metrics from spans (#2080) build: Bump tempfile to remove flagged dependency (#2085) feat(filter): Add document_uri csp filter (#2059) build: Use jemalloc on Linux (#2084) ci(gha): add debugging for "Couldn'\''t write tracker file" issue (#2081) feat(crons): Pass-through minimal `sdk` field (#2073) fix(quota): Parse large limits (#2079) instr(server): More details on unexpected envelope drop (#2077) fix(ci): Skip flaky integration test (#2076) feat(dynamic-sampling): Add support for `trace.replay_id` in matching rules (#2070) ...
This PR adds a new function to
sentry-relaycalledrun_dynamic_samplingwhich will run dynamic sampling on a twoSamplingConfigs.