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

Don't have transactions go through post_process #81065

Open
JoshFerge opened this issue Nov 20, 2024 · 4 comments
Open

Don't have transactions go through post_process #81065

JoshFerge opened this issue Nov 20, 2024 · 4 comments
Assignees

Comments

@JoshFerge
Copy link
Member

JoshFerge commented Nov 20, 2024

given that for post_process we use rabbitmq/celery, and its quite high throughput, we should consider that for transactions, this may not be necessary.

We can cut the number of rabbitmq tasks we send by ~40% if we simply don't use post_process for transactions.

We can also cut out the usage of rc-processing for transactions for this.

We'd move this transaction sampling over to save_event_transaction.

@JoshFerge
Copy link
Member Author

JoshFerge commented Nov 21, 2024

Roll out strategy:

Part 1:

what to watch for during rollout:

Part 2:

Stop producing post_process for transactions

  • see that post_process enqueues decline
  • see logging messages for "not found" decline

@mwarkentin
Copy link
Member

send a receive signal.

@JoshFerge during SRE office hours it was raised that sending signals is not an asynchronous event - basically all of the existing (and future) receivers of those signals are still processed synchronously. So we need to understand what is being triggered by that signal and understand how that impacts things. Are we going to be doing those signal handlers inside of save_event_transaction now too?

@JoshFerge
Copy link
Member Author

send a receive signal.

@JoshFerge during SRE office hours it was raised that sending signals is not an asynchronous event - basically all of the existing (and future) receivers of those signals are still processed synchronously. So we need to understand what is being triggered by that signal and understand how that impacts things. Are we going to be doing those signal handlers inside of save_event_transaction now too?

i've changed the implementation to not use signals and call what the signal did previously directly. we also confirmed that these calls are very fast https://sentry.sentry.io/traces/?project=1&query=span.op%3Atasks.post_process_group.transaction_processed_signal&statsPeriod=24h&visualize=%7B%22chartType%22%3A1%2C%22yAxes%22%3A%5B%22avg%28span.duration%29%22%5D%7D

JoshFerge added a commit that referenced this issue Nov 22, 2024
…vent_transaction (#81077)

first PR of #81065

no behavior changes until option is turned on. need to change
post_process as well before we can turn the option on. equivalent of
https://github.com/getsentry/sentry/blob/b84d3d9238ab75822b076d16de1111c3035fe073/src/sentry/tasks/post_process.py#L614
in post_process
@JoshFerge
Copy link
Member Author

TODO as a follow up, we can remove this update to rc-processing in save_event if its a transaction https://github.com/getsentry/sentry/blob/master/src/sentry/ingest/consumer/processors.py#L81-L88

there may be other places where we're doing unecessary work as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants