feat(dynamic-sampling): Track metric every time a DSC is built #3575
Conversation
| Ordering::Greater | Ordering::Equal => { | ||
| relay_statsd::metric!( | ||
| counter(RelayCounters::MissingDynamicSamplingContext) += 1, | ||
| sdk_name = sdk_name.as_str() |
There was a problem hiding this comment.
This tag has technically unbounded cardinality but we know that the SDK names are not many.
There was a problem hiding this comment.
You know that sdk_name is in SUPPORTED_SDK_VERSIONS, so it is bounded, right?
There was a problem hiding this comment.
Yeah in this case this is a good point, I forgot I was checking the presence in the map.
| // The list is defined here: | ||
| // https://docs.sentry.io/product/performance/performance-at-scale/getting-started | ||
| static SUPPORTED_SDK_VERSIONS: Lazy<HashMap<&'static str, &'static str>> = Lazy::new(|| { | ||
| let mut supported_sdk_versions = HashMap::new(); |
There was a problem hiding this comment.
HashMap::from([("key", "value"), ...])
| /// | ||
| /// This function is just temporary since it's used to compare SDK versions when an [`Event`] is | ||
| /// received. | ||
| fn compare_versions(version1: &str, version2: &str) -> std::cmp::Ordering { |
There was a problem hiding this comment.
Can we use the https://docs.rs/semver/latest/semver/ crate for this? It's already part of our dependency tree.
There was a problem hiding this comment.
I imagined there was something, will use that, thanks!
| Ordering::Greater | Ordering::Equal => { | ||
| relay_statsd::metric!( | ||
| counter(RelayCounters::MissingDynamicSamplingContext) += 1, | ||
| sdk_name = sdk_name.as_str() |
There was a problem hiding this comment.
You know that sdk_name is in SUPPORTED_SDK_VERSIONS, so it is bounded, right?
| /// Emits a metric when an [`Event`] is inside an [`Envelope`] without [`DynamicSamplingContext`]. | ||
| /// | ||
| /// This function is a temporary function which has been added mainly for debugging purposes. Our | ||
| /// goal with this function is to validate how many times the DSC is not |
There was a problem hiding this comment.
The end of the sentence is missing.
We could, no strong opinions here. I didn't want to pollute our issues stream with this, since it might be very noisy. @jjbayer what do you think? |
| return true; | ||
| } | ||
|
|
||
| false |
There was a problem hiding this comment.
Since I wanted easier testability of the function, I returned a bool here.
| ..Default::default() | ||
| }); | ||
|
|
||
| let Some(sdk_name) = should_have_dsc(&event) else { |
| should_have_dsc = "true" | ||
| ); | ||
| } | ||
| _ => { |
This PR adds a new metric that collects the number of times the DSC is built by Relay when it was not found on a transaction that should contain the DSC.
The logic in this PR performs a semantic versioning comparison between the minimum version of an SDK that should support a DSC and the current SDK version of the SDK that sent the event.
Closes: #3570
#skip-changelog