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: Add thread id and name to span data #3359

Merged
merged 2 commits into from
Oct 27, 2023
Merged

Conversation

philipphofmann
Copy link
Member

📜 Description

Add current thread name and thread id to every span when the SDK initializes the span.

💡 Motivation and Context

Fixes GH-3355

💚 How did you test it?

Unit tests and with the sample 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.

🔮 Next steps

Add current thread name and thread id to every span
when the SDK initializes the span.

Fixes GH-3355
@github-actions
Copy link

github-actions bot commented Oct 25, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against f862cf3

@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Merging #3359 (f862cf3) into main (ae9c51b) will increase coverage by 0.008%.
The diff coverage is 96.629%.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3359       +/-   ##
=============================================
+ Coverage   89.243%   89.252%   +0.008%     
=============================================
  Files          500       500               
  Lines        54592     54644       +52     
  Branches     19592     19610       +18     
=============================================
+ Hits         48720     48771       +51     
+ Misses        5005      5004        -1     
- Partials       867       869        +2     
Files Coverage Δ
Sources/Sentry/SentryCoreDataTracker.m 94.482% <100.000%> (ø)
Sources/Sentry/SentryDependencyContainer.m 95.000% <100.000%> (+0.240%) ⬆️
Sources/Sentry/SentryNSDataTracker.m 92.000% <100.000%> (ø)
Sources/Sentry/SentrySpan.m 95.652% <100.000%> (+0.318%) ⬆️
Sources/Sentry/include/SentryInternalDefines.h 42.105% <100.000%> (+6.811%) ⬆️
...ts/SentryTests/Transaction/SentryTracerTests.swift 99.183% <100.000%> (+<0.001%) ⬆️
...ntryTests/Transaction/SentryTransactionTests.swift 96.407% <100.000%> (ø)
...ests/SentryTests/Transaction/SentrySpanTests.swift 99.253% <94.545%> (-0.747%) ⬇️

... 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 ae9c51b...f862cf3. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Oct 25, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1243.06 ms 1260.88 ms 17.82 ms
Size 22.85 KiB 411.69 KiB 388.84 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
6f4d20c 1228.67 ms 1238.88 ms 10.21 ms
dc0db9e 1246.06 ms 1260.46 ms 14.40 ms
1db04d8 1250.20 ms 1258.12 ms 7.92 ms
4053ee9 1248.04 ms 1250.82 ms 2.78 ms
b2f82fa 1236.94 ms 1262.86 ms 25.92 ms
ea73af6 1230.96 ms 1244.98 ms 14.02 ms
326b7eb 1204.76 ms 1234.96 ms 30.20 ms
596ccc1 1214.98 ms 1239.36 ms 24.38 ms
98a8c16 1243.33 ms 1257.86 ms 14.53 ms
7cd187e 1239.02 ms 1261.42 ms 22.40 ms

App size

Revision Plain With Sentry Diff
6f4d20c 20.76 KiB 431.71 KiB 410.95 KiB
dc0db9e 20.76 KiB 419.62 KiB 398.86 KiB
1db04d8 20.76 KiB 435.50 KiB 414.74 KiB
4053ee9 20.76 KiB 435.22 KiB 414.46 KiB
b2f82fa 20.76 KiB 419.62 KiB 398.86 KiB
ea73af6 20.76 KiB 425.75 KiB 404.99 KiB
326b7eb 20.76 KiB 432.31 KiB 411.55 KiB
596ccc1 22.84 KiB 401.44 KiB 378.60 KiB
98a8c16 20.76 KiB 431.00 KiB 410.24 KiB
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB

Previous results on branch: feat/span-thread-info

Startup times

Revision Plain With Sentry Diff
093b6a9 1264.65 ms 1281.02 ms 16.37 ms

App size

Revision Plain With Sentry Diff
093b6a9 22.85 KiB 411.72 KiB 388.87 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.

Looks good, one question on architecture but nothing blocking!

Comment on lines +40 to +51
SentryCrashThread currentThread = sentrycrashthread_self();
_data[SPAN_DATA_THREAD_ID] = @(currentThread);

if ([NSThread isMainThread]) {
_data[SPAN_DATA_THREAD_NAME] = @"main";
} else {
NSString *threadName = [SentryDependencyContainer.sharedInstance.threadInspector
getThreadName:currentThread];
if (threadName.length > 0) {
_data[SPAN_DATA_THREAD_NAME] = threadName;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we couldn't put this block of logic into SentryThreadInspector, so that the code here would just read as

_data[SPAN_DATA_THREAD_NAME] = SentryDependencyContainer.sharedInstance.threadInspector.threadName;

Copy link
Member Author

Choose a reason for hiding this comment

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

ThreadInspector threadName returns an empty string when the thread name is nil, see

NSString *threadName = [NSString stringWithCString:pBuffer encoding:NSUTF8StringEncoding];
if (nil == threadName) {
threadName = @"";
}
return threadName;

I will fix that in another PR.

Good catch, thanks @armcknight 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened a PR #3361

@philipphofmann philipphofmann merged commit 2124551 into main Oct 27, 2023
71 checks passed
@philipphofmann philipphofmann deleted the feat/span-thread-info branch October 27, 2023 08:18
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.

Tag all spans with thread info
2 participants