-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
ref(transactions): dont send transactions to post_process #81085
Conversation
dce58ed
to
c57c1ea
Compare
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## jferg/tx-post-process-1 #81085 +/- ##
============================================================
+ Coverage 57.52% 78.48% +20.95%
============================================================
Files 7204 7215 +11
Lines 319305 319819 +514
Branches 43991 44047 +56
============================================================
+ Hits 183686 251003 +67317
+ Misses 130864 62421 -68443
- Partials 4755 6395 +1640 |
f89cfd2
to
1505605
Compare
c57c1ea
to
aa9812d
Compare
1505605
to
f85382e
Compare
aa9812d
to
af6770e
Compare
f85382e
to
5ccfc69
Compare
af6770e
to
4e620f4
Compare
4e620f4
to
41fa796
Compare
transaction_processed_signal_mock: mock.MagicMock, | ||
mock_record_sample: mock.MagicMock, | ||
eventstream_insert: mock.MagicMock, |
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.
Are you missing a patch()
?
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.
ah yes, thank you. fixed.
41fa796
to
5267ffa
Compare
rollout strategy: |
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.
TODO: split this up into two separate options as well.
closing this PR as per #81077 (comment), we've decided to not modify post_process as all for this change. deletion from rc processing consolidated into the same option as the rest of the changes. |
Once we've deployed #81079, we no longer need to send transactions to post_process.
We delete the redis key from the processing store right in save event as its no longer needed, and take advantage of the existing
raw / skip_consume
functionality to tell the event stream not to carry on the transaction to post_process after it's been inserted into snuba.sentry/src/sentry/event_manager.py
Line 1144 in dce58ed
Once this is merged and the option is turned on, we can delete all of the option code, as well as code that is no longer active in post_process.
part of #81065
note that when this is deployed, there could be some transactions that go through post_process that do not have values in the processing store. this shouldn't cause problems, and wil monitor logs from post_process here:
sentry/src/sentry/tasks/post_process.py
Line 531 in dce58ed