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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hubs/Scopes Merge 1 - Introduce IScopes interface. #3297

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Apr 2, 2024

#skip-changelog

馃摐 Description

Tests on this PR will likely fail, there's more PRs migrating IHub references to IScopes and fixing tests.

TODO: this should target a new branch for a new major release.

  • Introduce IScopes
  • IHub extends IScopes so any Hub can be passed in as IScopes. All methods on IHub have been moved to IScopes.
  • IHub and methods tied to it like getCurrentHub, cloneMainHub and clone have been deprecated.
    • For API that still returns an IHub we're wrapping IScopes using HubScopesWrapper
  • In a follow up PR we'll introduce Scopes as a replacement for Hub
  • Added ScopesAdapter as a replacement for HubAdapter
  • Sentry.getCurrentScopes() replaces Sentry.getCurrentHub()
  • Sentry.cloneMainHub() will be replaced in a follow up PR by something like Sentry.forkRootScopes() or similar.
  • There's plenty of TODO comments in this PR that have to be cleaned up in follow up PRs.

Follow up PRs for changing from IHub to IScopes:

馃挕 Motivation and Context

This is the first PR of many to merge hubs and scopes which is a prerequisite for Performance powered by OpenTelemetry.

馃挌 How did you test it?

馃摑 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

馃敭 Next steps

Copy link
Contributor

github-actions bot commented Apr 2, 2024

Messages
馃摉 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 馃毇 dangerJS against 27f2398

Copy link
Collaborator

@lbloder lbloder left a comment

Choose a reason for hiding this comment

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

Commented on three instances where the HubScopesWrapper could be used to make it compile when -Werror is disabled.

Otherwise LGTM 馃憤

sentry/src/main/java/io/sentry/Sentry.java Show resolved Hide resolved
sentry/src/main/java/io/sentry/Sentry.java Show resolved Hide resolved
sentry/src/main/java/io/sentry/Sentry.java Show resolved Hide resolved
sentry/src/main/java/io/sentry/IScopes.java Show resolved Hide resolved
sentry/src/main/java/io/sentry/IScopes.java Show resolved Hide resolved
sentry/src/main/java/io/sentry/IScopes.java Show resolved Hide resolved
void reportFullyDisplayed();

/**
* @deprecated See {@link IHub#reportFullyDisplayed()}.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Replace IHub with IScopes

sentry/src/main/java/io/sentry/IScopes.java Show resolved Hide resolved
sentry/src/main/java/io/sentry/IScopes.java Show resolved Hide resolved
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

public interface IScopes {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason we name it plural IScopes instead of IScope?

Copy link
Member Author

Choose a reason for hiding this comment

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

IScope is already present (i.e. the scope) and IScopes is a set of multiple scopes (isolation scope and current scope) which (sometimes) fork together or separately. IScopes (and Scopes) is meant to be a drop-in replacement for hub.

* @deprecated See {@link IHub#reportFullyDisplayed()}.
*/
@Deprecated
default void reportFullDisplayed() {
Copy link
Member

Choose a reason for hiding this comment

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

Given that we'll do a major bump I guess it's worth considering removing deprecated methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Created #3364 to track and added it to 8.0.0 milestone.

@adinauer adinauer changed the base branch from main to 8.x.x April 16, 2024 13:16
@adinauer adinauer merged commit bf5857e into 8.x.x Apr 16, 2024
9 of 28 checks passed
@adinauer adinauer deleted the feat/hsm-1-introduce-iscopes branch April 16, 2024 13:25
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.

None yet

4 participants