Skip to content

ref: replace legacy keys with canonical keys in samples#70752

Merged
asottile-sentry merged 1 commit into
masterfrom
asottile-canonical-key-in-samples
May 14, 2024
Merged

ref: replace legacy keys with canonical keys in samples#70752
asottile-sentry merged 1 commit into
masterfrom
asottile-canonical-key-in-samples

Conversation

@asottile-sentry

@asottile-sentry asottile-sentry commented May 13, 2024

Copy link
Copy Markdown
Contributor

the first large chunk of CanoncalKeyDict errors are related to sample data still having the legacy keys -- this fixes those

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 13, 2024
@asottile-sentry asottile-sentry force-pushed the asottile-canonical-key-in-samples branch from ee304c7 to 22a9758 Compare May 13, 2024 14:50
@asottile-sentry asottile-sentry changed the title ref: replace legacy message with logentry in samples ref: replace legacy keys with canonical keys in samples May 13, 2024
@asottile-sentry asottile-sentry force-pushed the asottile-canonical-key-in-samples branch from 22a9758 to beef0d9 Compare May 13, 2024 14:57
@asottile-sentry asottile-sentry marked this pull request as ready for review May 13, 2024 16:26
@asottile-sentry asottile-sentry requested review from a team and jan-auer May 13, 2024 16:26

@jan-auer jan-auer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(I didn't check usages in the code, yet) Are these samples used in tests? I'm asking as IIRC at least message is still used by SDKs and is documented as a valid key. We could fix such tests by running the payload through librelay's normalize function, if that's the case.

@asottile-sentry

Copy link
Copy Markdown
Contributor Author

(I didn't check usages in the code, yet) Are these samples used in tests? I'm asking as IIRC at least message is still used by SDKs and is documented as a valid key. We could fix such tests by running the payload through librelay's normalize function, if that's the case.

they're used in tests and I believe public samples -- they are injected directly into eventstore in tests which I believe is why they're triggering canonical rewriting there -- (the canonical rewriting just makes the message show up in logentry)

@asottile-sentry asottile-sentry force-pushed the asottile-canonical-key-in-samples branch from beef0d9 to e12d62f Compare May 14, 2024 08:50
"dist": null,
"platform": "csharp",
"culprit": "Samples.AspNetCore.Mvc.Controllers.HomeController in PostIndex",
"message": "Param is null!",

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.

this was duplicated below as logentry already

"dist": "2",
"platform": "java",
"message": "",
"logentry": {},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't the empty ones be removed?

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.

I think these are intentional to avoid the code which injects a default message here so I left them

"culprit": "script-src 'self' 'unsafe-eval' 'unsafe-inline' www.example.com",
"message": "Blocked 'script' from 'example.org'",
"sentry.interfaces.Csp": {
"logentry": {"formatted": "Blocked 'script' from 'example.org'"},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
"logentry": {"formatted": "Blocked 'script' from 'example.org'"},
"logentry": {
"formatted": "Blocked 'script' from 'example.org'"
},

@codecov

codecov Bot commented May 14, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.00%. Comparing base (f06829a) to head (e12d62f).

Additional details and impacted files
@@             Coverage Diff             @@
##           master   #70752       +/-   ##
===========================================
+ Coverage   56.77%   80.00%   +23.23%     
===========================================
  Files        6495     6506       +11     
  Lines      290288   290793      +505     
  Branches    50055    50123       +68     
===========================================
+ Hits       164798   232661    +67863     
+ Misses     125049    57696    -67353     
+ Partials      441      436        -5     
Files Coverage Δ
src/sentry/utils/samples.py 84.15% <100.00%> (+32.43%) ⬆️

... and 1991 files with indirect coverage changes

@asottile-sentry asottile-sentry merged commit a05dfe2 into master May 14, 2024
@asottile-sentry asottile-sentry deleted the asottile-canonical-key-in-samples branch May 14, 2024 09:21
@github-actions github-actions Bot locked and limited conversation to collaborators May 29, 2024
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