Skip to content

ref(grouping): Add SDK name as a tag on event_manager metrics#59379

Merged
vartec merged 7 commits into
masterfrom
vartec/add_sdk_to_grouping_metrics
Nov 6, 2023
Merged

ref(grouping): Add SDK name as a tag on event_manager metrics#59379
vartec merged 7 commits into
masterfrom
vartec/add_sdk_to_grouping_metrics

Conversation

@vartec
Copy link
Copy Markdown
Contributor

@vartec vartec commented Nov 3, 2023

Adding SDK tag to event_manager metrics.

Implements: #59053

@vartec vartec requested a review from lobsterkatie November 3, 2023 21:37
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Nov 3, 2023
"sentry:grouping_config": new_grouping,
}
for (key, value) in changes.items():
for key, value in changes.items():
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^ auto-formatting

Copy link
Copy Markdown
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, to match our conventions, I'd do ref(grouping): Add SDK name..., but otherwise, LGTM!

Can you please update the relevant tests, also? This may not find all of them, but a good place to start is to search for mock_metrics_incr.

@vartec vartec changed the title dev(grouping): Adding SDK name as a tag to event_manager grouping metrics ref(grouping): Adding SDK name as a tag to event_manager grouping metrics Nov 3, 2023
@vartec vartec marked this pull request as ready for review November 3, 2023 22:39
@vartec vartec requested a review from a team as a code owner November 3, 2023 22:39
Copy link
Copy Markdown
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this more closely (which I should have done the first time, sorry), a few small things:

  • We use present tense rather than gerund in our PR titles/commit messages ("add" rather than "adding").

  • There's one spot, called out below, where I don't think we need to add an SDK tag.

  • I think it's worth deciding the scope of this change and either revising the PR title or pulling most of your changes into a separate PR. Of the metrics you've modified here, only the one in _calculate_event_grouping and the one labeled grouping.in_app_frame_mix are actually grouping metrics. (You'd be forgiven for thinking that the one in _calculate_span_grouping is also about grouping, but when we say "grouping," what we really mean is specifically event grouping.) So I'd either a) pull "grouping" out of the PR title, and make it just about event_manager metrics, or b) restrict it just to grouping metrics.

  • In either case, I'd also add the tags in the following spots:

    • The event_manager.apply_server_fingerprinting metric.
    • The event_manager.background_grouping metric. For this one, you'll need to switch from the decorator to a context manager inside the function calling metrics.timer (which is what metrics.wraps does internaly).
    • The metric_tags passed to save_error_events inside of save.

Comment thread src/sentry/event_manager.py Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 3, 2023

Codecov Report

Merging #59379 (0c0a135) into master (c2e0590) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #59379      +/-   ##
==========================================
- Coverage   80.77%   80.77%   -0.01%     
==========================================
  Files        5164     5164              
  Lines      225813   225813              
  Branches    37995    37995              
==========================================
- Hits       182403   182401       -2     
- Misses      37883    37884       +1     
- Partials     5527     5528       +1     

see 9 files with indirect coverage changes

@vartec vartec changed the title ref(grouping): Adding SDK name as a tag to event_manager grouping metrics ref(grouping): Add SDK name as a tag on event_manager metrics Nov 4, 2023
Copy link
Copy Markdown
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the save_event.calculate_event_grouping metric!

@vartec vartec merged commit 260b4cd into master Nov 6, 2023
@vartec vartec deleted the vartec/add_sdk_to_grouping_metrics branch November 6, 2023 21:31
@vartec vartec added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Nov 7, 2023
@getsentry-bot
Copy link
Copy Markdown
Contributor

PR reverted: 90781a3

getsentry-bot added a commit that referenced this pull request Nov 7, 2023
…#59379)"

This reverts commit 260b4cd.

Co-authored-by: vartec <2284726+vartec@users.noreply.github.com>
@lobsterkatie
Copy link
Copy Markdown
Member

We reverted this because it was blowing up the number of tags we're keeping track of in DD. We're going to look into doing some normalization to reduce the number of values.

vartec pushed a commit that referenced this pull request Nov 7, 2023
Adding SDK tag to event_manager metrics.

Implements: #59053
vartec pushed a commit that referenced this pull request Nov 8, 2023
#59501 Normalizes SDK tags to reduce their cardinality. 
Related to #59075 and #59379.

- non-Sentry SDK tags are ignored (collapsed into `"other"`)
- official Sentry SDK tags are normalized and shortened: 
    -  `sentry.javascript.*` are mostly kept as-is
    -  `sentry.native.*` are collapsed to 3 levels
    -  all other `sentry.*` are collapsed to 2 levels

---------

Co-authored-by: Katie Byers <katie.byers@sentry.io>
Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
vartec pushed a commit that referenced this pull request Nov 9, 2023
…ics (#59572)

This re-applies previously reverted #59379 combining it with #59504 to
normalize SDK tags reducing their cardinality.

Implements: #59053

---------

Co-authored-by: Katie Byers <katie.byers@sentry.io>
Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
vartec pushed a commit that referenced this pull request Nov 9, 2023
…ics (#59572)

This re-applies previously reverted #59379 combining it with #59504 to
normalize SDK tags reducing their cardinality.

Implements: #59053

---------

Co-authored-by: Katie Byers <katie.byers@sentry.io>
Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
@github-actions github-actions Bot locked and limited conversation to collaborators Nov 22, 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 Trigger: Revert Add to a merged PR to revert it (skips CI)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants