Skip to content

⚡️ Speed up function has_tracing_enabled by 15% #4940

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

⚡️ Speed up function has_tracing_enabled by 15% #4940

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
16 changes: 9 additions & 7 deletions sentry_sdk/tracing_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,12 @@ def has_tracing_enabled(options):
if options is None:
return False

return bool(
options.get("enable_tracing") is not False
and (
options.get("traces_sample_rate") is not None
or options.get("traces_sampler") is not None
)
if options.get("enable_tracing") is False:
return False

return (
options.get("traces_sample_rate") is not None
or options.get("traces_sampler") is not None
)
Comment on lines -104 to 110
Copy link
Member

Choose a reason for hiding this comment

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

I agree with removing the bool wrapper but question this part of the PR description:

Early return for enable_tracing=False: Instead of evaluating the entire boolean expression, the code immediately returns False when enable_tracing is explicitly False. This eliminates the need to check the tracing keys (traces_sample_rate and traces_sampler) in cases where tracing is disabled

Since the and operator short-circuits in Python, if options.get("enable_tracing") is False, then we will not check the tracing keys, since the first part of the and statement is already False in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree, python short circuits so should not evaluate the whole expression. the description was generated by codeflash with LLMs so its possible for there to be some gaps. I will update the description.
Although the speedups do materialize, as you can see in the runtime details in the dropdown

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to know how much of the performance improvement can be attributed to removing the bool wrapper and how much can be attributed to rewriting the expression.

Without justification that rewriting the short-circuiting statement by itself improves performance, I don't see a reason to merge the change.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. I would add that it would be, in any case, preferable to split this PR into multiple PRs, each of which only make one change, rather than combining all of the changes into a single PR.

That way, we can consider each distinct change independently from each other.



Expand Down Expand Up @@ -527,7 +527,9 @@ def _fill_sample_rand(self):
)
return

self.dynamic_sampling_context["sample_rand"] = f"{sample_rand:.6f}" # noqa: E231
self.dynamic_sampling_context["sample_rand"] = (
f"{sample_rand:.6f}" # noqa: E231
)

def _sample_rand(self):
# type: () -> Optional[str]
Expand Down
Loading