-
-
Notifications
You must be signed in to change notification settings - Fork 452
Add thread information when a span is created #2998
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
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- Add thread information when a span is created ([#2998](https://github.com/getsentry/sentry-java/pull/2998)) If none of the above apply, you can opt out of this check by adding |
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c7e2fbc | 372.00 ms | 461.71 ms | 89.71 ms |
bc4be3b | 360.40 ms | 435.04 ms | 74.64 ms |
0bf143e | 368.35 ms | 437.47 ms | 69.12 ms |
86f0096 | 368.63 ms | 446.92 ms | 78.29 ms |
99a51e2 | 405.11 ms | 479.65 ms | 74.54 ms |
7ca9895 | 364.31 ms | 460.46 ms | 96.15 ms |
c7e2fbc | 393.98 ms | 478.24 ms | 84.27 ms |
c7e2fbc | 377.85 ms | 426.35 ms | 48.50 ms |
86f0096 | 371.86 ms | 439.78 ms | 67.92 ms |
4bf95dd | 345.96 ms | 414.24 ms | 68.28 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
c7e2fbc | 1.72 MiB | 2.29 MiB | 576.40 KiB |
bc4be3b | 1.72 MiB | 2.29 MiB | 576.53 KiB |
0bf143e | 1.72 MiB | 2.29 MiB | 576.50 KiB |
86f0096 | 1.72 MiB | 2.29 MiB | 576.50 KiB |
99a51e2 | 1.72 MiB | 2.29 MiB | 576.34 KiB |
7ca9895 | 1.72 MiB | 2.29 MiB | 576.51 KiB |
c7e2fbc | 1.72 MiB | 2.29 MiB | 576.40 KiB |
c7e2fbc | 1.72 MiB | 2.29 MiB | 576.40 KiB |
86f0096 | 1.72 MiB | 2.29 MiB | 576.50 KiB |
4bf95dd | 1.72 MiB | 2.29 MiB | 576.40 KiB |
Previous results on branch: feat/starfish-span-mainthread-flag
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
2f17022 | 376.33 ms | 463.83 ms | 87.50 ms |
f9cd2b8 | 382.38 ms | 455.80 ms | 73.42 ms |
2e5b9df | 373.00 ms | 445.30 ms | 72.30 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
2f17022 | 1.72 MiB | 2.29 MiB | 576.71 KiB |
f9cd2b8 | 1.72 MiB | 2.29 MiB | 576.55 KiB |
2e5b9df | 1.72 MiB | 2.29 MiB | 576.55 KiB |
Codecov ReportAll modified and coverable lines are covered by tests ✅
📢 Thoughts on this report? Let us know!. |
@@ -384,6 +385,8 @@ private ISpan createChild( | |||
} | |||
}); | |||
span.setDescription(description); | |||
span.setData( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we agree to use the same blocked_main_thread
key? I'd rather us choose something different, because we don't know for sure if it blocked the main thread, we know that it was just started there? Also, if we pick a different key, the current behavior for File I/O or DB spans will remain the same with blocked_main_thread
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, this wasn't discussed yet - thanks for bringing this up. @shruthilayaj I think Roman has some good points here, would it make sense for you as well to have a separate flag for main thread flagging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it makes sense to have these as separate flags!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit lengthy, but how about started_on_main_thread
? Would also make it clear that the flag is determined on span start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rfc https://github.com/getsentry/rfcs/pull/75/files got merged just a couple of days ago, so we probably should adhere to what is defined there. Here's the develop docs PR which adds the the new keys to the span data conventions getsentry/develop#1056, which means we have to send thread.id
and thread.name
, how does that sounds @shruthilayaj @markushi ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h
: I just wanted to make the same comment ⬆️ . Thanks @romtsn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
just to confirm, sending thread.id
and thread.name
in addition to started_on_main_thread
flag right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah makes sense, and will thread name always be standardized to main
if it started on the main thread regardless of the SDK it's coming from? (just so we know if we just look for main
or need to maintain some sort of map/allow list for extracting this info in relay)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd start with just main
, I guess we can expand it later. We can ensure that it's called main
from the SDKs side
📜 Description
Add thread.id and thread.name to the span context.
💡 Motivation and Context
Fixes #2997
💚 How did you test it?
Add Unit test
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps