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(txnames): Include transactions without source in clustering #51437

Merged
merged 4 commits into from
Jun 23, 2023

Conversation

jjbayer
Copy link
Member

@jjbayer jjbayer commented Jun 22, 2023

When an SDK does not provide a transaction source annotation, Relay does one of two things:

  1. When the transaction name is expected to contain identifiers (high-cardinality), leave the source empty.
  2. When the transaction name is expected to be low-cardinality, set it to unknown.

In the first case, we want to run the clusterer to detect high-cardinality patterns. We have to make the simplifying assumption that a transaction name containing / is a URL.

Note: This PR includes source:null transactions in the clustering input. To also apply the discovered rules in Relay, we need another change in https://github.com/getsentry/relay/.

ref: https://github.com/getsentry/team-ingest/issues/129

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 22, 2023
@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Merging #51437 (eac9c9b) into master (55cf7d2) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #51437      +/-   ##
==========================================
- Coverage   81.24%   81.24%   -0.01%     
==========================================
  Files        4896     4900       +4     
  Lines      205353   205432      +79     
  Branches    11101    11101              
==========================================
+ Hits       166843   166905      +62     
- Misses      38264    38281      +17     
  Partials      246      246              
Impacted Files Coverage Δ
...y/ingest/transaction_clusterer/datasource/redis.py 97.60% <100.00%> (+0.05%) ⬆️

... and 29 files with indirect coverage changes

@jjbayer jjbayer changed the title Feat/txnames sample unknown feat(txnames): Include transactions without source annotation in clustering Jun 22, 2023
@jjbayer jjbayer changed the title feat(txnames): Include transactions without source annotation in clustering feat(txnames): Include transactions without source in clustering Jun 22, 2023
@jjbayer jjbayer marked this pull request as ready for review June 22, 2023 12:52
@jjbayer jjbayer requested review from a team and k-fish June 22, 2023 12:52
@jjbayer jjbayer merged commit 05628da into master Jun 23, 2023
@jjbayer jjbayer deleted the feat/txnames-sample-unknown branch June 23, 2023 06:46
jjbayer added a commit to getsentry/relay that referenced this pull request Jun 27, 2023
getsentry/sentry#51437 expanded the scope of
transaction name clustering to transactions from old SDKs that do not
send a `source` annotation yet. This PR enables the application of rules
to this type of transactions in Relay.

Because there is a potential of triggering the cardinality limiter, I
gated this new behavior with a feature flag.

The condition for applying clusterer rules has now become more
complicated, so I decided to remove / ignore the `scope` configuration
flag entirely.
@github-actions github-actions bot locked and limited conversation to collaborators Jul 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants