fix: Propagate span when copying scope #2952

kabiroberai · 2023-04-24T19:20:50Z

📜 Description

This PR updates the SentryScope copy constructor to copy over the parent's span.

💡 Motivation and Context

Previously, copied scopes (such as those created in -[SentrySDK captureEvent:withScopeBlock:]) did not inherit the original scope's span. This resulted in emitted events not linking back to the relevant span. Other SDKs do copy over the parent span/transaction (for example here's the equivalent code in sentry-java).

💚 How did you test it?

I tested these changes in a production iOS app, ensuring that spans now propagate as expected.

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

kabiroberai · 2023-04-24T19:22:48Z

Tests/SentryTests/SentryScopeSwiftTests.swift

@@ -108,6 +108,36 @@ class SentryScopeSwiftTests: XCTestCase {
        XCTAssertNil(actual["transaction"])
        XCTAssertNotNil(actual["breadcrumbs"])
    }
+
+    func testInitWithScope() {


this test is mostly a direct translation of the ObjC version, but with the addition of a check that calling cloned.applyTo has the same effect as calling it on the original scope. The new check would have failed without the change to the copy constructor.

Please add some validation for assigning self.span = scope.span in the initWithScope. So basically if I remove self.span = scope.span from initWithScope this test should fail.

Oops, this was what the Event comparison was supposed to validate, but I forgot to assign a span to the scope. Removing that line goes from green -> red as expected now.

philipphofmann · 2023-04-25T15:50:39Z

Thanks for the PR, @kabiroberai. We didn't get to this today, but we'll have a look tomorrow.

philipphofmann

Thanks for the PR @kabiroberai. Could you please split this PR up into two PRs? One for fixing the copy and another one for startWithOptions:andScope. Those are two independent changes, and they should have extra Changelog entries, PR titles etc.

Before you open the PR for startWithOptions:andScope, we still need to clarify if we want to add this method to the SDK cause it's not part of our unified API, and maybe we want to add it to all of our SDKs with a static API. I will get back to you.

philipphofmann · 2023-04-26T08:41:47Z

@kabiroberai, could you elaborate on the race condition you experience between start and configureScope? Does your app do heavy work in the background already when you start the SDK?

kabiroberai · 2023-04-27T19:26:20Z

👍 moved the startWithOptions:andScope: changes out of this PR. The race condition is speculative/us being extra cautious so it's possible that it doesn't actually get triggered, though I can stress-test to double check. Our main use case for passing our own scope is that we can replace the hub's scope with a subclass — to that end a +[SentrySDK setScope:] method would achieve the same goal. Unfortunately configureScope doesn't do the trick because we can't replace the scope instance entirely, only modify its properties.

Taking a step back, the reason we need to subclass Scope is because we're using Swift's @TaskLocal to allow different (concurrent) Tasks to have different "current" scopes. IIUC other Sentry SDKs achieve a similar goal through the use of thread-local hubs, but I don't see a way to do such a thing in the Cocoa SDK yet. I'd be happy to discuss alternative approaches here — maybe on Discord?

philipphofmann · 2023-04-28T09:11:10Z

@kabiroberai, JavaScript the concept of the initialScope on the options. We could do the same on Cocoa. I guess that would work for you.

through the use of thread-local hubs

At some point, we should implement thread-local hubs, but if initialScope solves your problem, I would like to delay that a bit, as initialScope is also useful for other users.

philipphofmann

Thanks for doing this @kabiroberai 👏

Please add an entry to the changelog under a section Fixed with Propagate span when copying scope. CI fails for the benchmarks and release builds, cause that doesn't work for contributors yet. Just focus on the lints and unit tests. FYI, our unit test are a bit flaky at the moment. We did several fixes on the main branch, so merging main into this PR should help.

philipphofmann · 2023-04-28T09:16:08Z

Tests/SentryTests/SentryScopeSwiftTests.swift

@@ -108,6 +108,36 @@ class SentryScopeSwiftTests: XCTestCase {
        XCTAssertNil(actual["transaction"])
        XCTAssertNotNil(actual["breadcrumbs"])
    }
+
+    func testInitWithScope() {


Please add some validation for assigning self.span = scope.span in the initWithScope. So basically if I remove self.span = scope.span from initWithScope this test should fail.

brustolin

Thanks @kabiroberai !!

This looks good to me.

philipphofmann

Thanks, @kabiroberai 💯; LGTM. Just waiting for CI to be green.

philipphofmann · 2023-05-02T12:44:31Z

I will fix the Changelog entry so that we can merge this PR.

kabiroberai requested review from philipphofmann, brustolin and armcknight as code owners April 24, 2023 19:20

kabiroberai commented Apr 24, 2023

View reviewed changes

kabiroberai changed the title ~~Scope propagation improvements~~ feat: scope propagation improvements Apr 24, 2023

kahest added the Team: Mobile Platform label Apr 25, 2023

philipphofmann reviewed Apr 26, 2023

View reviewed changes

kabiroberai added 2 commits April 27, 2023 12:11

fix: Propagate spans

10f0ec5

Strengthen initWithScope test

bc83334

kabiroberai force-pushed the kabir/default-scope branch from 57f55f3 to bc83334 Compare April 27, 2023 19:15

kabiroberai changed the title ~~feat: scope propagation improvements~~ fix: propagate spans when copying scopes Apr 27, 2023

philipphofmann changed the title ~~fix: propagate spans when copying scopes~~ fix: Propagate span when copying scope Apr 28, 2023

philipphofmann reviewed Apr 28, 2023

View reviewed changes

philipphofmann self-assigned this Apr 28, 2023

kabiroberai added 2 commits April 28, 2023 02:29

testInitWithScope: set span on scope

dc23ad7

Update changelog

9df0030

kabiroberai requested a review from philipphofmann May 1, 2023 16:39

Merge branch 'main' into kabir/default-scope

d0a493c

brustolin reviewed May 2, 2023

View reviewed changes

philipphofmann approved these changes May 2, 2023

View reviewed changes

philipphofmann merged commit 9454d5d into getsentry:main May 2, 2023

philipphofmann mentioned this pull request May 2, 2023

Update CHANGELOG.md #2981

Merged

kabiroberai mentioned this pull request May 2, 2023

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

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Propagate span when copying scope #2952

fix: Propagate span when copying scope #2952

kabiroberai commented Apr 24, 2023 •

edited

Loading

kabiroberai Apr 24, 2023 •

edited

Loading

philipphofmann Apr 28, 2023

kabiroberai Apr 28, 2023

philipphofmann commented Apr 25, 2023

philipphofmann left a comment

philipphofmann commented Apr 26, 2023 •

edited

Loading

kabiroberai commented Apr 27, 2023

philipphofmann commented Apr 28, 2023

philipphofmann left a comment •

edited

Loading

philipphofmann Apr 28, 2023

brustolin left a comment

philipphofmann left a comment •

edited

Loading

philipphofmann commented May 2, 2023

fix: Propagate span when copying scope #2952

fix: Propagate span when copying scope #2952

Conversation

kabiroberai commented Apr 24, 2023 • edited Loading

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

kabiroberai Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

philipphofmann Apr 28, 2023

Choose a reason for hiding this comment

kabiroberai Apr 28, 2023

Choose a reason for hiding this comment

philipphofmann commented Apr 25, 2023

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann commented Apr 26, 2023 • edited Loading

kabiroberai commented Apr 27, 2023

philipphofmann commented Apr 28, 2023

philipphofmann left a comment • edited Loading

Choose a reason for hiding this comment

philipphofmann Apr 28, 2023

Choose a reason for hiding this comment

brustolin left a comment

Choose a reason for hiding this comment

philipphofmann left a comment • edited Loading

Choose a reason for hiding this comment

philipphofmann commented May 2, 2023

kabiroberai commented Apr 24, 2023 •

edited

Loading

kabiroberai Apr 24, 2023 •

edited

Loading

philipphofmann commented Apr 26, 2023 •

edited

Loading

philipphofmann left a comment •

edited

Loading

philipphofmann left a comment •

edited

Loading