-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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(project-release): POST should not call Snuba #69831
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #69831 +/- ##
=======================================
Coverage 79.84% 79.84%
=======================================
Files 6486 6486
Lines 288285 288315 +30
Branches 49667 49671 +4
=======================================
+ Hits 230178 230202 +24
- Misses 57705 57711 +6
Partials 402 402
|
These will get sentry/src/sentry/api/serializers/models/release.py Lines 528 to 529 in d80487e
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was no_snuba
meant to be used more widely used than just the health data?
https://github.com/getsentry/sentry/pull/23568/files
I don't know if it's fair to say we're ignoring the flag as opposed to it was an added variable that we can utilize here 🤷
quick comment/question though
1ec7b28
to
436ea3b
Compare
Updated the PR so now the logic is that at the time of creation, first_seen and last_seen will be none. The type is already nullable so we should be find. |
436ea3b
to
545230f
Compare
environment_id=None, | ||
versions=[o.version for o in item_list], | ||
) | ||
if no_snuba_for_release_creation: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: why is snuba being called in a serializer? It doesn't make sense to me to have something that is supposed to serialize data make IO calls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ good point 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave that decision to someone who has more context on release and merge this fix. @nhsiehgit would rotation backlog would be the right place to put this refactory ticket?
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Customer is hitting rate limiting when they shouldn't. ticket: https://getsentry.atlassian.net/browse/OPS-5972
It turns out project release endpoint is intentionally skipping snuba on POST requests but this call is ignoring the passed flag
no_snuba
.More context on investigation: https://sentry.slack.com/archives/C039ZGT5K/p1714161375978469
I'm not familiar with release code so leaving it here to see if this is the right behavior. Is it ok to set tag_values to empty at the time of release creation?