Skip to content

poc(sdk): Add experimental dsn for upcoming perf work #36000

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

Conversation

k-fish
Copy link
Member

@k-fish k-fish commented Jun 24, 2022

Summary

This adds an experimental dsn/transport to the MultiplexingTransport to intentionally send specific flagged events solely to a separate dsn, which will help us avoid troubles with ingesting random errors into our main Sentry project.

Will be followed up with the perf-issue experimental sdk changes to be added to getsentry along with adding the experimental dsn key (experimental_dsn) there.

Refs: getsentry/sentry-python@caa4140

This adds an experimental dsn to the MultiplexingTransport to intentionally send specific flagged events solely to a separate dsn, which will help us avoid troubles with ingesting random errors into our main Sentry project.
@k-fish k-fish requested a review from untitaker June 24, 2022 00:44
@k-fish k-fish requested a review from a team as a code owner June 24, 2022 00:44
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 24, 2022
if is_current_event_experimental():
if rate and random.random() < rate:
getattr(experimental_transport, method_name)(*args, **kwargs)
# Experimental events should not be sent to other transports even if they are not sampled.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you can do that. By setting the experimental flag on the scope you're effectively dropping the transaction for that web request. Or do you intend to push a new scope for producing the experimental event?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's pushing a new scope for capturing the exception and setting the tag there, so only the error event will be sent to the experimental dsn.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@untitaker untitaker Jun 24, 2022

Choose a reason for hiding this comment

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

ok. if this scope leaks for some reason (concurrency bug of some sort) we're royally screwed.

may i suggest creating a completely separate SDK setup (not even using the same client)?

basic_client = Hub(Client(mydsn))


with basic_client:
    capture_event(...)

inside of the with-statement, you are using the new client and can send your events. the events are on a separate transport queue just as well

the scope is not the same, all request data will be missing from your events. but on the flipside this is much stronger isolation.

this is how pytest-sentry isolates things.

I am not sure if you can live with not having any other scope data though

Copy link
Member Author

Choose a reason for hiding this comment

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

scope leaks for some reason

Like how the transports were leaking before? 😆

I don't really expect the scope to leak, do we have any active issues with scope leaking that would make this more of a concern? The failure case, (being screwed here, lol) would only happen flipping the rate switch for the experimental dsn, which we can immediately turn off, and just for SaaS events, I feel like a separate client might be a bit too defensive?

@@ -309,6 +309,9 @@
# Note: A value that is neither 0 nor 1 is regarded as 0
register("store.use-relay-dsn-sample-rate", default=1)

# A rate to apply to any events denoted as experimental to be sent to an experimental dsn.
register("store.use-experimental-dsn-sample-rate", default=0.0)
Copy link
Member

Choose a reason for hiding this comment

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

reminder, please pre-register this option in a separate PR and set it to 0 explicitly

@k-fish k-fish merged commit 9288539 into master Jun 29, 2022
@k-fish k-fish deleted the poc/perf-py-sdk-add-experimental-sdk-options-and-transport branch June 29, 2022 12:51
@github-actions github-actions bot locked and limited conversation to collaborators Jul 14, 2022
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.

3 participants