Skip to content

fix: Do not call before_send for transactions #731

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

Merged
merged 1 commit into from
Jun 25, 2020

Conversation

rhcarvalho
Copy link
Contributor

This matches the behavior with JS and the specs.

The change in JS was introduced in getsentry/sentry-javascript#2600.
See notes under "Few other fixes".

@rhcarvalho rhcarvalho requested a review from untitaker June 23, 2020 13:57
@rhcarvalho
Copy link
Contributor Author

Got a test failure in test_aws.py:

https://travis-ci.com/github/getsentry/sentry-python/jobs/352702298#L1612-L1616

image

Quick glance at the test implementation and I guess it could be flakey.

cc @untitaker

@rhcarvalho rhcarvalho force-pushed the rhcarvalho/transactions-no-before-send branch from 46ad44b to 82c5a05 Compare June 23, 2020 15:27
@rhcarvalho
Copy link
Contributor Author

Rebased to fix merge conflict.

@rhcarvalho rhcarvalho merged commit e9389b0 into master Jun 25, 2020
@rhcarvalho rhcarvalho deleted the rhcarvalho/transactions-no-before-send branch June 25, 2020 10:34
@vmarkovtsev
Copy link

Is there a standard way to filter out transactions now? I've got all my backend endpoint handler entry points wrapped in spans and need to silence a few unimportant ones to save $$$ on the events quota.

@rhcarvalho
Copy link
Contributor Author

Is there a standard way to filter out transactions now? I've got all my backend endpoint handler entry points wrapped in spans and need to silence a few unimportant ones to save $$$ on the events quota.

@vmarkovtsev you can use sentry_sdk.scope.add_global_event_processor and pass in a func to silence transaction events (and in fact do anything before_send could do). Make sure to check for event.get("type") == "transaction".

This is a workaround. Keep in mind that manually sampling transactions will potentially skew the aggregate metrics you see in Sentry.

@rhcarvalho
Copy link
Contributor Author

@vmarkovtsev also worth noting that event processors run right before the transaction is sent, which means that the sampling decision has already been propagated to other services for the life time of the transaction. It might or might not be of importance to you.

If you're doing manual instrumentation, you should also be able to override the sampling decision early on by starting a transaction with sampled=False.

We're working on a more general solution for controlling transaction sampling from SDKs.

@vmarkovtsev
Copy link

In my case, I am using aiohttp integration which automatically starts transactions for each incoming HTTP request. I am not sure how to set sampled=False then... But yeah, canceling the sampling whatsoever is better ofc.

Thanks for the workaround! It should work fine for me 👍

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

Successfully merging this pull request may close these issues.

3 participants