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: decouple dual-purpose profiling IDs #3282

Merged
merged 3 commits into from Sep 18, 2023

Conversation

armcknight
Copy link
Member

one was used to profile concurrent spans in the client, and that was reused for the ID needed to identify profiles in the backend, and we wound up in a situation where one profile ID used for multiple concurrent spans was reused for each of their profiles

… those needed to identify profiles in the backend
@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #3282 (5951641) into main (881a955) will decrease coverage by 0.017%.
The diff coverage is 100.000%.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3282       +/-   ##
=============================================
- Coverage   89.223%   89.206%   -0.017%     
=============================================
  Files          501       502        +1     
  Lines        53949     54257      +308     
  Branches     19170     19477      +307     
=============================================
+ Hits         48135     48401      +266     
- Misses        4958      4997       +39     
- Partials       856       859        +3     
Files Changed Coverage
...entry/Profiling/SentryProfiledTracerConcurrency.mm 100.000%
Sources/Sentry/SentryProfiler.mm 100.000%
Tests/SentryProfilerTests/SentryProfilerTests.mm 100.000%

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 881a955...5951641. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Sep 15, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1256.57 ms 1265.68 ms 9.11 ms
Size 22.85 KiB 407.63 KiB 384.78 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
102f2a6 1225.71 ms 1244.28 ms 18.57 ms
47b41ed 1252.31 ms 1253.96 ms 1.64 ms
c00eafe 1198.26 ms 1227.62 ms 29.36 ms
e1eed6b 1259.08 ms 1270.57 ms 11.49 ms
bd2afa6 1245.24 ms 1263.18 ms 17.94 ms
e5ac362 1204.12 ms 1229.41 ms 25.29 ms
443723a 1205.24 ms 1220.52 ms 15.28 ms
005bb4c 1237.38 ms 1255.54 ms 18.16 ms
ea73af6 1241.69 ms 1252.96 ms 11.27 ms
0dfdaaa 1226.88 ms 1248.82 ms 21.94 ms

App size

Revision Plain With Sentry Diff
102f2a6 20.76 KiB 433.18 KiB 412.42 KiB
47b41ed 20.76 KiB 436.66 KiB 415.90 KiB
c00eafe 20.76 KiB 432.87 KiB 412.11 KiB
e1eed6b 20.76 KiB 432.17 KiB 411.41 KiB
bd2afa6 20.76 KiB 420.55 KiB 399.79 KiB
e5ac362 20.76 KiB 426.11 KiB 405.34 KiB
443723a 20.76 KiB 414.44 KiB 393.68 KiB
005bb4c 20.76 KiB 419.70 KiB 398.94 KiB
ea73af6 20.76 KiB 425.76 KiB 404.99 KiB
0dfdaaa 20.76 KiB 434.56 KiB 413.79 KiB

Previous results on branch: armcknight/fix/profile-id-reuse

Startup times

Revision Plain With Sentry Diff
5a52ddf 1196.26 ms 1226.28 ms 30.02 ms

App size

Revision Plain With Sentry Diff
5a52ddf 22.85 KiB 407.65 KiB 384.80 KiB

@@ -211,7 +211,7 @@
@"model" : isEmulated ? sentry_getSimulatorDeviceModel() : sentry_getDeviceModel()
};

payload[@"profile_id"] = profileID.sentryIdString;
Copy link
Member

Choose a reason for hiding this comment

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

💯

@armcknight armcknight merged commit 7ce3cf6 into main Sep 18, 2023
41 of 42 checks passed
@armcknight armcknight deleted the armcknight/fix/profile-id-reuse branch September 18, 2023 18:08
@github-actions
Copy link

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- decouple dual-purpose profiling IDs ([#3282](https://github.com/getsentry/sentry-cocoa/pull/3282))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 8078b9e

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

2 participants