Skip to content
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

Merged
merged 6 commits into from
Jul 27, 2022

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Jul 26, 2022

Add a new project config flag transactionMetrics.acceptTransactionNames that can be either

  • strict - Only accept transaction names from low-cardinality sources.
  • clientBased - Accept all transaction names, except for sentry.javascript.browser and sentry.javascript.react, where strict 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

@jjbayer jjbayer requested a review from a team July 26, 2022 15:07
Comment on lines 238 to 245
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)
Copy link
Member

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).

Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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.

jjbayer added a commit to getsentry/sentry that referenced this pull request Jul 27, 2022
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.
@jjbayer jjbayer merged commit dbb9763 into master Jul 27, 2022
@jjbayer jjbayer deleted the feat/metrics-accept-transaction-names branch July 27, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants