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

[BUG] Sentry scope migration fo 2.x #129

Merged
merged 7 commits into from
Sep 23, 2024
Merged

Conversation

pedroserrudo
Copy link
Contributor

@pedroserrudo pedroserrudo commented Aug 6, 2024

migrate sentry scope usage to v2.x
more info

@JonasKs
Copy link
Member

JonasKs commented Aug 6, 2024

Thanks!

No idea what's happened to my pipelines.. I'll try to find time this week.

@pedroserrudo
Copy link
Contributor Author

pedroserrudo commented Aug 6, 2024

Thanks!

No idea what's happened to my pipelines.. I'll try to find time this week.

it seems to be some sort of version incompatibility between potery, celery and uvicorn, it needs a general version bump

@JonasKs
Copy link
Member

JonasKs commented Aug 7, 2024

Hmm, you might be right! PR welcome! If no one submits one, I'll try to have a look this week.

Copy link
Member

@sondrelg sondrelg left a comment

Choose a reason for hiding this comment

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

Forcing users onto a new sentry-sdk version seems a bit aggressive. Could we instead add something like this?

import pkg_resources

installed_version = pkg_resources.get_distribution("sentry_sdk").version

...

if installed_version < "0.14.3":
    with sentry_sdk.configure_scope() as scope:
        scope.set_tag('transaction_id', guid)
else:
    scope = sentry_sdk.get_isolation_scope()
    scope.set_tag('transaction_id', guid)

And substitute 0.14.3 for whatever the version before the new API was added is?

If this package is 3.8+ (is it?) we should be able to use from importlib.metadata import version instead

Copy link
Member

@sondrelg sondrelg left a comment

Choose a reason for hiding this comment

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

I think this looks good!

@JonasKs
Copy link
Member

JonasKs commented Aug 22, 2024

Sorry for the delay, @pedroserrudo. This looks perfect, thanks. I'll look into pipelines on Friday after work, and get this out in the weekend. 😊

@JonasKs
Copy link
Member

JonasKs commented Sep 8, 2024

Hmmm, pipelines still seem to fail due to dependencies. 🤔

@JonasKs
Copy link
Member

JonasKs commented Sep 20, 2024

Hi, could you look into the failing tests? 😊 They're passing in main now.

@JonasKs JonasKs merged commit e57f044 into snok:main Sep 23, 2024
11 checks passed
@JonasKs
Copy link
Member

JonasKs commented Sep 23, 2024

Thanks so much 😊 I'll release tomorrow.

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