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

fix: Keep PropagationContext when cloning scope #4518

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

philipphofmann
Copy link
Member

📜 Description

When cloning the scope with initWithScope, the SDK wrongly created a new propagation context instead of cloning it. This is fixe now.

💡 Motivation and Context

I found this bug in #4504. This aligns with the behavior from Java: https://github.com/getsentry/sentry-java/blob/e039872d80a9a352d10e642ff630a3d905ff89d0/sentry/src/main/java/io/sentry/Scope.java#L154

💚 How did you test it?

Unit test.

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

🔮 Next steps

When cloning the scope with initWithScope, the SDK wrongly created a new
propagation context instead of cloning it. This is fixe now.
Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.438%. Comparing base (9854a57) to head (ade1087).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4518       +/-   ##
=============================================
- Coverage   91.462%   91.438%   -0.025%     
=============================================
  Files          615       614        -1     
  Lines        68991     68842      -149     
  Branches     24749     24597      -152     
=============================================
- Hits         63101     62948      -153     
- Misses        5798      5802        +4     
  Partials        92        92               
Files with missing lines Coverage Δ
Sources/Sentry/SentryPropagationContext.m 100.000% <100.000%> (ø)
Sources/Sentry/SentryScope.m 96.741% <100.000%> (ø)
Tests/SentryTests/SentryScopeSwiftTests.swift 98.972% <100.000%> (+0.003%) ⬆️

... and 30 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 9854a57...ade1087. Read the comment docs.

Copy link

github-actions bot commented Nov 7, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1220.90 ms 1246.10 ms 25.20 ms
Size 21.90 KiB 727.75 KiB 705.85 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
6bc5ae5 1230.90 ms 1246.10 ms 15.20 ms
3d8532d 1225.27 ms 1238.80 ms 13.53 ms
fa1ec44 1216.80 ms 1244.42 ms 27.62 ms
5616e0a 1237.00 ms 1260.43 ms 23.43 ms
ed49f0c 1215.94 ms 1245.63 ms 29.69 ms
af1f4dd 1238.08 ms 1258.48 ms 20.40 ms
50bb751 1207.51 ms 1219.37 ms 11.86 ms
c838244 1213.51 ms 1243.76 ms 30.24 ms
39b1c35 1236.35 ms 1239.90 ms 3.55 ms
56ec5d0 1236.65 ms 1261.90 ms 25.25 ms

App size

Revision Plain With Sentry Diff
6bc5ae5 20.76 KiB 401.40 KiB 380.64 KiB
3d8532d 22.85 KiB 414.09 KiB 391.24 KiB
fa1ec44 21.58 KiB 670.39 KiB 648.81 KiB
5616e0a 22.85 KiB 407.45 KiB 384.60 KiB
ed49f0c 21.58 KiB 632.13 KiB 610.55 KiB
af1f4dd 22.85 KiB 414.71 KiB 391.86 KiB
50bb751 21.58 KiB 417.86 KiB 396.27 KiB
c838244 21.58 KiB 629.61 KiB 608.03 KiB
39b1c35 22.85 KiB 408.88 KiB 386.03 KiB
56ec5d0 20.76 KiB 414.44 KiB 393.69 KiB

Previous results on branch: fix/keep-propagation-context-clone-scope

Startup times

Revision Plain With Sentry Diff
4d0f394 1243.76 ms 1248.24 ms 4.49 ms

App size

Revision Plain With Sentry Diff
4d0f394 21.90 KiB 727.72 KiB 705.82 KiB

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.

LGTM

Sources/Sentry/SentryScope.m Show resolved Hide resolved
@philipphofmann philipphofmann merged commit 05ac767 into main Nov 8, 2024
63 of 65 checks passed
@philipphofmann philipphofmann deleted the fix/keep-propagation-context-clone-scope branch November 8, 2024 09:23
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.

2 participants