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

fix(issues): Don't treat %s as unique #75090

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mrduncan
Copy link
Member

@mrduncan mrduncan commented Jul 26, 2024

If an event already has a parameterized logentry.message, ensure that uniq_id doesn't try to treat the %s as unique.

Example event payload:

{
  "logentry": {
    "message": "Hard time limit (%ss) exceeded for %s[%s]",
    "formatted": "Hard time limit (123s) exceeded for sentry.tasks.example[01234567-890a-bcde-f012-34567890abcd]",
    "params": [123, "sentry.tasks.example", "01234567-890a-bcde-f012-34567890abcd"]
  }
}

Prior to this change, these would have been parameterized as Hard time limit (%ss) exceeded for <uniq_id> (internal example).

This is currently pretty hard to test end-to-end and is also only catching a subset of possible % replacements, will follow up with a future PR to improve that but for now this will address the most common case.

Reminder: this parameterization is still only enabled experimentally and is not widely released.

If an event already has a parameterized logentry message, ensure that
uniq_id doesn't try to treat the %s as unique.
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jul 26, 2024
Copy link

codecov bot commented Jul 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.12%. Comparing base (9de90e9) to head (e7e21d0).
Report is 728 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #75090       +/-   ##
===========================================
+ Coverage   57.05%   78.12%   +21.07%     
===========================================
  Files        6735     6748       +13     
  Lines      300698   301174      +476     
  Branches    51732    51810       +78     
===========================================
+ Hits       171551   235301    +63750     
+ Misses     124394    59542    -64852     
- Partials     4753     6331     +1578     
Files Coverage Δ
src/sentry/grouping/parameterization.py 96.52% <100.00%> (+20.41%) ⬆️

... and 2077 files with indirect coverage changes

return False # Don't replace short tokens
if token_str[0] == "<" and token_str[-1] == ">":
return False # Don't replace already-parameterized tokens
if "%s" in token_str:
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to add %d in here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly a combination of the difficulty in testing this and I want to limit the amount of additional work we have do do on each token_str until we have a better way to do better performance testing as well

@getsantry
Copy link
Contributor

getsantry bot commented Sep 5, 2024

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you add the label WIP, I will leave it alone unless WIP is removed ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@getsantry getsantry bot added the Stale label Sep 5, 2024
@mrduncan mrduncan added the WIP label Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components Stale WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants