-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
🐛 FIX #52676 update sentry scope configuration to remove deprecation wa… #53398
base: master
Are you sure you want to change the base?
🐛 FIX #52676 update sentry scope configuration to remove deprecation wa… #53398
Conversation
…cation warning
@saimadib is attempting to deploy a commit to the Airbyte Growth Team on Vercel. A member of the Team first needs to authorize it. |
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.
Hey @saimadib I think you did great to resolve this issue but I would Add a docstring to explain the purpose of the with_command_context decorator to enhance readibility. Using a decorator to set Sentry context for pipeline commands is a good way to encapsulate this logic.
…mproved readability and maintainability
…hub.com/saimadib/airbyte into saimadib/fix-sentry-deprecation-warning
Hi @Charlesnorris509 , thanks for the feedback! I've updated the PR to include a docstring for the with_command_context and with_step_context decorator, which should enhance readability. Let me know your feedback!. |
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.
The updated code looks great! The detailed docstrings make it much clearer and more structured, and the integration of Sentry for error tracking and context management is well done. The docstrings really help with readability and maintainability. Additionally I test the integration locally and it seems like you passed the 'ruff-format' test. I'll Approve the change. But i would recommend preparing some procedure for Handling missing attributes and writing unit tests would further improve the robustness and reliability of the code
@natikgadzhi can you ask someone from tooling team to review this contribution? |
just following up to see if there are any updates on the review from the tooling team? |
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.
This is pretty good work @saimadib! 👏🏼
Let me format the thing and run tests — I think our CI really didn't consider that folks will try and change airbyte-ci itself, so I might have to dance around it for a bit.
A few things that we need to do:
- I'm not sure, but should we update sentry dependency in the pyproject.toml?
- If we do so, don't forget to
poetry lock
in the pipelines package. - Bump the pipelines package version in
pyproject.toml
. Patch-level bump is fine - Add a README entry to pipelines/README.md
@@ -37,56 +37,94 @@ def before_send(event: Dict[str, Any], hint: Dict[str, Any]) -> Optional[Dict[st | |||
|
|||
|
|||
def with_step_context(func: Callable) -> Callable: | |||
"""Decorator to add Sentry context for pipeline step execution. |
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.
Thank you for adding docs!
}, | ||
) | ||
# Using get_current_scope() instead of configure_scope() as per Sentry 2.x guidelines | ||
scope = sentry_sdk.get_current_scope() |
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.
Dropping with
here might mean you're not discarding scope once you're done, but honestly, I don't see lots of harm in this.
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.
Thank you for bringing this up! I can clarify the implementation choice with reference to Sentry's official migration guide.
The change we made is exactly as recommended in Sentry's migration documentation. According to the guide, we should replace the deprecated scope configuration:
with sentry_sdk.configure_scope() as scope:
# do something with scope
with the new recommended approach:
scope = sentry_sdk.get_current_scope()
# do something with scope
While the with
statement might seem to provide better scope cleanup, the new get_current_scope()
method is specifically designed to work with Sentry's scope management system in version 2.x.
Thank you for the feedback! @natikgadzhi Let me address each point:
I am not certain with the version, Could you help me with the version? |
…rning
Fixes #52676
What
sentry_sdk.configure_scope is deprecated and will be removed in the next major version
How
sentry_utils.py
to useget_current_scope()
instead of deprecatedconfigure_scope()
with_step_context
decorator to follow Sentry SDK's new best practicesReview guide
airbyte-ci/connectors/pipelines/pipelines/helpers/sentry_utils.py
configure_scope()
withget_current_scope()
poetry run pytest .
in the pipelines directoryUser Impact
Can this PR be safely reverted and rolled back?