Skip to content
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

Swap UUID generation to be lazy #2131

Closed
wants to merge 6 commits into from

Conversation

bayangan1991
Copy link

Running py-spy while the sentry sdk in a django project shows that there is a lot of time spent generating un-used uuid.

This PR simply takes the same UUID format and puts it's behind some custom getter and setter code.

Just in a fairly large django project start I saw number of UUID's used go from several hundred thousand to hundreds.

@sl0thentr0py
Copy link
Member

hey @bayangan1991 thanks for the PR, that's a valuable observation.

I'm actually working on some more general 'make the SDK as low overhead as possible', so I'll take this into account for that. I'll keep this PR open for now and come back to you in some time on if we take this in or I incorporate avoiding UUID / span creation in some other way.

Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

hey @bayangan1991 I think we'll take this in!
can you fix the one type signature for 2.7?

return self._span_id

@span_id.setter
def span_id(self, value: str) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def span_id(self, value: str) -> None:
def span_id(self, value) -> None:

our tests need to pass on 2.7 as well

Copy link
Author

Choose a reason for hiding this comment

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

Ah yeah ok. Force of habit. I dropped the extra -> None as well.

@sl0thentr0py sl0thentr0py marked this pull request as ready for review June 23, 2023 12:28
@sl0thentr0py
Copy link
Member

ty! I'll merge this on monday after some quick sanity testing on django

@antonpirker antonpirker reopened this Mar 7, 2024
@sl0thentr0py
Copy link
Member

im gonna reopen this separately since we dont have rights for the fork

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