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

(2) Move capture_* from Hub to Client #2555

Merged
merged 32 commits into from
Dec 19, 2023

Conversation

antonpirker
Copy link
Member

@antonpirker antonpirker commented Dec 1, 2023

Moved some functionality from Hub to Client:

  • Capture Event:

    • moved capture_event from Hub to Client
    • created new capture_event in Scope that calls capture_event in Client
    • made capture_event in Hub call the new capture_event in Scope
  • Capture Exception:

    • created new capture_exception in Scope
    • made capture_exception in Hub call the new one in Scope
  • Capture Message:

    • created new capture_message in Scope
    • made capture_message in Hub call the new one in Scope
  • renamed **scope_args to **scope_kwargs because it contains keyword arguments.

  • moved _update_scope from Hub to Scope and renamed it to _merge_scopes

This is preparation work for refactoring how we deal with Hubs and Scopes in the future.
Its properly easier to reason about this change when checking out the branch than looking at the diff.

Depends on: #2578 (please review the linked one first)

@antonpirker antonpirker changed the base branch from master to antonpirker/refactor-hub December 1, 2023 10:06
@antonpirker antonpirker changed the title Move capture_* from Hub to Client (2) Move capture_* from Hub to Client Dec 4, 2023
Base automatically changed from antonpirker/refactor-hub to master December 6, 2023 08:02
Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

LGTM 🌟 Left some suggestions.

sentry_sdk/client.py Outdated Show resolved Hide resolved
sentry_sdk/hub.py Outdated Show resolved Hide resolved
sentry_sdk/client.py Outdated Show resolved Hide resolved
@antonpirker antonpirker changed the base branch from master to feat/new-scopes December 11, 2023 11:32
@antonpirker antonpirker changed the base branch from feat/new-scopes to antonpirker/refactor-hub December 11, 2023 11:33
Base automatically changed from antonpirker/refactor-hub to feat/new-scopes December 11, 2023 11:54
@sl0thentr0py
Copy link
Member

@antonpirker ok I understand what you're doing but WHY are you moving these things to client and how does it help you with the further refactor?

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.

Overall I don't understand the motivation for this change. Much of these methods will move to Scope according to the RFC and the Client should be fairly similar to before with just the one capture_event (that serializes and sends to transport).

capture_exception|message|transaction etc will be done by the new Scope.

message, # type: str
level=None, # type: Optional[str]
scope=None, # type: Optional[Scope]
top_scope=None, # type: Optional[Scope]
Copy link
Member

Choose a reason for hiding this comment

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

I would not add top_scope here.

If you see the RFC, the 3 scopes will be processed by Scope.capture_event.

The managing of the scopes is currently done by the Hub and will be moved to Scope later. I think the client should just get one scope as before and the merging will be moved later from Hub -> Scope.

@antonpirker
Copy link
Member Author

You are right. I took the wrong turn here. Makes total sense to move the functions to the the scope. :-)

tests/test_client.py Outdated Show resolved Hide resolved
sentry_sdk/scope.py Outdated Show resolved Hide resolved
@antonpirker antonpirker merged commit 79e15f5 into feat/new-scopes Dec 19, 2023
120 checks passed
@antonpirker antonpirker deleted the antonpirker/refactor-hub-capture branch December 19, 2023 11:20
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