-
Notifications
You must be signed in to change notification settings - Fork 464
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
feat(metrics): Stronger recursion protection #2426
Conversation
# or similar will be disabled. In the background thread this can | ||
# never happen, but in the force flush case which happens in the | ||
# foreground we might make it here unprotected. | ||
with recursion_protection(): |
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.
In the background thread, this is basically always a no-op as in_metrics
is forcefully set to True
. In the force flush case we might be flipping it here. Why is flush
itself unprotected? Two reasons:
- on the sentry side we currently patch it, so making it a noop would break the logic there
- it's potentially brittle since
_emit
is invoked in more than one place, this should be safer.
@@ -495,8 +517,10 @@ def _get_aggregator_and_update_tags(key, tags): | |||
|
|||
callback = client.options.get("_experiments", {}).get("before_emit_metric") | |||
if callback is not None: | |||
if not callback(key, updated_tags): | |||
return None, updated_tags | |||
with recursion_protection() as in_metrics: |
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.
Motivation here is to intentionally protect the callback. That way we only poke around on the thread local when we need to. This also means any user of the _get_aggregator_and_update_tags
gets automatic protection and does not need to manually take care of marking their functions as noop.
This strengthens the recursion protection to cover two more cases:
before_emit_metric
in itself reporting a metric. This was previously not protectedRefs getsentry/sentry#57791