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

feat: Allow starting the SDK with an initial scope #2982

Merged
merged 13 commits into from
May 4, 2023

Conversation

kabiroberai
Copy link
Contributor

@kabiroberai kabiroberai commented May 2, 2023

📜 Description

This PR adds a SentryOptions.initialScope property, enabling simple configuration of the initial scope used when starting the SDK.

💡 Motivation and Context

The reasons for this change are twofold:

  1. Helps avoid the possibility of race conditions between starting the SDK and calling configureScope
  2. Allows clients to pass a SentryScope subclass to override certain behaviors. For more context, see the discussion in fix: Propagate span when copying scope #2952.

There is precedent for this option; see #2952 (comment) which mentions a similar initialScope option in the JS SDK.

💚 How did you test it?

Unit tests + production app.

📝 Checklist

You have to check all boxes before merging:

  • 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.

one of these days i'll get the changelog entry correct first go 🙃
@kabiroberai kabiroberai changed the title Allow starting the SDK with an initial scope feat: Allow starting the SDK with an initial scope May 2, 2023
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

Thanks @kabiroberai for this PR.

Looks good to me.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, @kabiroberai. We are close, but as pointed out in the comment below, this PR doesn't align with the approach we already have on JavaScript. I'm sorry if my communication wasn't clear enough, so you now have to adapt the PR.

* Inits and configures Sentry (SentryHub, SentryClient) and sets up all integrations. Make sure to
* set a valid DSN.
*/
+ (void)startWithOptions:(SentryOptions *)options andScope:(nullable SentryScope *)scope NS_SWIFT_NAME(start(options:scope:));
Copy link
Member

Choose a reason for hiding this comment

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

h: I prefer to align with JavaScript instead of adding an extra method. We should add a method initialScope to the options, basically what we have for SentrySDK.configureScope. The public-facing API should look like this

SentrySDK.start { options in
    options.dsn = "dsn"
    options.initialScope { scope in
        scope.setEnvironment("debug")
        // ...
    }
}

I also think that the API above is cleaner than having to init the options and the scope yourself:

let options = Options()
options.dns = "dsn"

let initialScope = Scope()
scope.setEnvironment("debug")

SentrySDK.start(options: options, scope: initialScope)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, done! I also made it possible to pass a whole Scope object by setting initialScopeFactory directly instead of calling initialScope:, which should solve for use case 2 mentioned in the PR description.

@kabiroberai kabiroberai requested a review from philipphofmann May 3, 2023 22:43
@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Merging #2982 (810ad99) into main (fd6a31c) will decrease coverage by 0.014%.
The diff coverage is 100.000%.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #2982       +/-   ##
=============================================
- Coverage   80.565%   80.552%   -0.014%     
=============================================
  Files          265       265               
  Lines        24709     24728       +19     
  Branches     10953     10963       +10     
=============================================
+ Hits         19907     19919       +12     
- Misses        4186      4191        +5     
- Partials       616       618        +2     
Impacted Files Coverage Δ
Sources/Sentry/SentryOptions.m 97.066% <100.000%> (+0.028%) ⬆️
Sources/Sentry/SentrySDK.m 90.178% <100.000%> (+0.044%) ⬆️

... and 8 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd6a31c...810ad99. Read the comment docs.

* Provides an initial scope to use when starting the SDK.
* @param callback A block that configures a fresh scope as desired.
*/
- (void)initialScope:(void (^)(SentryScope *scope))callback;
Copy link
Contributor Author

@kabiroberai kabiroberai May 4, 2023

Choose a reason for hiding this comment

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

I wonder whether we should rename this to -configureInitialScope: and rename initialScopeFactory to initialScope@philipphofmann what do you think? EDIT: brought the API in line with the JS one.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @kabiroberai 👏. LGTM

@philipphofmann
Copy link
Member

The failing CI jobs have nothing to do with the changes in this PR. We have some fixes on the main branch for https://github.com/getsentry/sentry-cocoa/actions/runs/4879105885/jobs/8705357840?pr=2982, but I don't want it to block this PR.

@philipphofmann philipphofmann merged commit 2b4a663 into getsentry:main May 4, 2023
@philipphofmann
Copy link
Member

I will update the docs.

@philipphofmann
Copy link
Member

Docs PR getsentry/sentry-docs#6838.

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