-
Notifications
You must be signed in to change notification settings - Fork 91
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tag has technically unbounded cardinality but we know that the SDK names are not many.
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.
You know that sdk_name is in SUPPORTED_SDK_VERSIONS, so it is bounded, right?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You know that sdk_name is in SUPPORTED_SDK_VERSIONS, so it is bounded, right?
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.
Can't we just emit a sentry error whenever we have a missing dsc at a certain sample rate, instead of doing this semver dance with a hardocded list of SDKs then just to have a metric which doesn't even include the SDK version?
/// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I wanted easier testability of the function, I returned a bool
here.
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.
This is intended as a temporary metric right?
..Default::default() | ||
}); | ||
|
||
let Some(sdk_name) = should_have_dsc(&event) else { |
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.
nit: just use unwrap here
should_have_dsc = "true" | ||
); | ||
} | ||
_ => { |
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.
nit: use None
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