-
Notifications
You must be signed in to change notification settings - Fork 94
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(metrics): Two modes for accepting transaction names [INGEST-1515] #1352
Conversation
let sdk_name = event | ||
.client_sdk | ||
.value() | ||
.and_then(|sdk| sdk.name.value()) | ||
.map(|s| s.as_str()) | ||
.unwrap_or_default(); | ||
|
||
!["sentry.javascript.browser", "sentry.javascript.react"].contains(&sdk_name) |
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: Let's move this entire thing into a utility function for readability, something like fn is_browser_sdk(&Event)
.
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.
Are we fine with just specific js sdks? I thought a lot of them had high cardinality names, along with some other sdk's like Ruby?
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.
We could also turn this around this and specify the SDKs that we trust to send only low-cardinality names. @jan-auer what do you think?
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.
It depends on both the prevalence and the absolute volume of high cardinality in these platforms. We could start with just JS and as we increase the sample rate extend it with more SDKs or even specific integrations that cause problems.
Relay has two modes for handling transaction names with source unknown (see getsentry/relay#1352): - In strict mode, treat all transaction names with source unknown as high-cardinality, and drop the name in the extracted metrics. - In clientBased mode, treat unknown as low-cardinality, except for browser JS SDKs. This PR adds the corresponding project config, such that a percentage of orgs can be consistently opted into clientBased behavior.
Add a new project config flag
transactionMetrics.acceptTransactionNames
that can be eitherstrict
- Only accept transaction names from low-cardinality sources.clientBased
- Accept all transaction names, except forsentry.javascript.browser
andsentry.javascript.react
, wherestrict
rules apply.In both cases, either omit the transaction name (when source is
unknown
), or replace by sentinel<< unparametrized >>
.By adding this to project configs, we can add an option in sentry to consistently sample a percentage of orgs into the new behavior.
Sentry-side implemented in getsentry/sentry#37102