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.

Sign up for GitHub

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 29, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/sentry/options/defaults.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


# Mock out integrations and services for tests
register("mocks.jira", default=False)

Expand Down
37 changes: 37 additions & 0 deletions src/sentry/utils/sdk.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import copy
import inspect
import random

import sentry_sdk
from django.conf import settings
Expand Down Expand Up @@ -111,6 +112,7 @@


UNSAFE_TAG = "_unsafe"
EXPERIMENT_TAG = "_experimental_event"


def is_current_event_safe():
Expand All @@ -137,6 +139,16 @@ def is_current_event_safe():
return True


def is_current_event_experimental():
"""
Checks if the event was explicitly marked as experimental.
"""
with configure_scope() as scope:
if scope._tags.get(EXPERIMENT_TAG):
return True
return False


def mark_scope_as_unsafe():
"""
Set the unsafe tag on the SDK scope for outgoing crashes and transactions.
Expand All @@ -148,6 +160,16 @@ def mark_scope_as_unsafe():
scope.set_tag(UNSAFE_TAG, True)


def mark_scope_as_experimental():
"""
Set the experimental tag on the SDK scope for outgoing crashes and transactions.

Marking the scope will cause these crashes and transaction to be sent to a separate experimental dsn.
"""
with configure_scope() as scope:
scope.set_tag(EXPERIMENT_TAG, True)


def set_current_event_project(project_id):
"""
Set the current project on the SDK scope for outgoing crash reports.
Expand Down Expand Up @@ -249,6 +271,7 @@ def configure_sdk():
sdk_options = dict(settings.SENTRY_SDK_CONFIG)

relay_dsn = sdk_options.pop("relay_dsn", None)
experimental_dsn = sdk_options.pop("experimental_dsn", None)
internal_project_key = get_project_key()
upstream_dsn = sdk_options.pop("dsn", None)
sdk_options["traces_sampler"] = traces_sampler
Expand All @@ -274,6 +297,12 @@ def configure_sdk():
else:
relay_transport = None

if experimental_dsn:
transport = make_transport(get_options(dsn=experimental_dsn, **sdk_options))
experimental_transport = patch_transport_for_instrumentation(transport, "experimental")
else:
experimental_transport = None

class MultiplexingTransport(sentry_sdk.transport.Transport):
def capture_envelope(self, envelope):
# Temporarily capture envelope counts to compare to ingested
Expand All @@ -299,6 +328,14 @@ def capture_event(self, event):
self._capture_anything("capture_event", event)

def _capture_anything(self, method_name, *args, **kwargs):
# Experimental events will be sent to the experimental transport.
if experimental_transport:
rate = options.get("store.use-experimental-dsn-sample-rate")
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.

getsentry/sentry-python@caa4140#diff-771cb012ea65fcb826b77e2abb36b504fdf3407fbe8054857b05fe5a41ed5164R595-R599

Copy link
Member

@untitaker untitaker Jun 24, 2022 •
edited
Loading

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?

return

# Upstream should get the event first because it is most isolated from
# the this sentry installation.
Expand Down