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

impr: Stop sending empty thread names #3361

Merged
merged 2 commits into from
Oct 30, 2023

Conversation

philipphofmann
Copy link
Member

📜 Description

Set the thread name to nil when retrieving the thread name from the machine context wrapper in case it fails or the thread name is empty. This slightly reduces the payload size.

💡 Motivation and Context

Came up here #3359 (comment).

💚 How did you test it?

Unit test and running the sample app on a simulator

📝 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

Set the thread name to nil when retrieving the thread name
from the machine context wrapper in case it fails or the
thread name is empty. This slightly reduces the payload size.
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #3361 (191adbe) into main (2124551) will decrease coverage by 0.032%.
Report is 1 commits behind head on main.
The diff coverage is 100.000%.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3361       +/-   ##
=============================================
- Coverage   89.251%   89.219%   -0.032%     
=============================================
  Files          500       500               
  Lines        54657     54690       +33     
  Branches     19618     19625        +7     
=============================================
+ Hits         48782     48794       +12     
- Misses        5004      5026       +22     
+ Partials       871       870        -1     
Files Coverage Δ
...s/Sentry/SentryCrashDefaultMachineContextWrapper.m 100.000% <100.000%> (ø)
Sources/Sentry/SentrySpan.m 95.569% <100.000%> (-0.083%) ⬇️
Sources/Sentry/SentryThreadInspector.m 98.400% <100.000%> (+0.066%) ⬆️
Tests/SentryTests/Protocol/SentryThreadTests.swift 100.000% <100.000%> (ø)
...Tests/SentryCrash/SentryThreadInspectorTests.swift 82.558% <100.000%> (+1.230%) ⬆️

... and 17 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 2124551...191adbe. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Oct 27, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1213.80 ms 1239.57 ms 25.78 ms
Size 22.85 KiB 411.66 KiB 388.81 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f715499 1234.26 ms 1259.40 ms 25.14 ms
66922ca 1221.68 ms 1235.98 ms 14.30 ms
3cba0e8 1236.29 ms 1271.21 ms 34.92 ms
8f397a7 1230.10 ms 1253.88 ms 23.77 ms
2e88e64 1233.64 ms 1241.53 ms 7.89 ms
24b4df1 1240.98 ms 1253.98 ms 13.00 ms
7bb0873 1226.18 ms 1247.30 ms 21.12 ms
2124551 1201.23 ms 1225.34 ms 24.11 ms
25bcc50 1240.47 ms 1254.70 ms 14.23 ms
11ccc16 1203.82 ms 1237.06 ms 33.24 ms

App size

Revision Plain With Sentry Diff
f715499 20.76 KiB 427.23 KiB 406.47 KiB
66922ca 20.76 KiB 425.80 KiB 405.04 KiB
3cba0e8 22.84 KiB 403.19 KiB 380.34 KiB
8f397a7 20.76 KiB 420.55 KiB 399.79 KiB
2e88e64 22.85 KiB 408.88 KiB 386.03 KiB
24b4df1 20.76 KiB 425.77 KiB 405.01 KiB
7bb0873 22.85 KiB 407.09 KiB 384.24 KiB
2124551 22.85 KiB 411.69 KiB 388.84 KiB
25bcc50 20.76 KiB 427.22 KiB 406.46 KiB
11ccc16 20.76 KiB 431.71 KiB 410.95 KiB

Previous results on branch: improvement/emtpy-thread-names

Startup times

Revision Plain With Sentry Diff
1aa71f7 1229.76 ms 1240.24 ms 10.48 ms

App size

Revision Plain With Sentry Diff
1aa71f7 22.85 KiB 411.71 KiB 388.86 KiB

Copy link
Member

@armcknight armcknight left a comment

Choose a reason for hiding this comment

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

Code looks fine, but does the server expect something to be in this field? What happens if the field isn't present?

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, I would just check what @armcknight pointed out, about the server being ok with this.

Sources/Sentry/SentryCrashDefaultMachineContextWrapper.m Outdated Show resolved Hide resolved
@philipphofmann
Copy link
Member Author

philipphofmann commented Oct 30, 2023

Great question, @armcknight 👍

thread.name is part of span data. You don't have to send it. You can omit it. This is when you skip it for spans.

CleanShot 2023-10-30 at 14 28 18@2x

And when you send it
CleanShot 2023-10-30 at 14 28 59@2x

For threads on events, you can omit the thread name. It's an optional field see https://develop.sentry.dev/sdk/event-payloads/threads/#attributes.

@philipphofmann philipphofmann merged commit 4aea556 into main Oct 30, 2023
67 of 70 checks passed
@philipphofmann philipphofmann deleted the improvement/emtpy-thread-names branch October 30, 2023 13:40
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