-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
rfc(feature): Span thread information #75
Conversation
46e9828
to
52a352b
Compare
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.
Two things from me:
- Spans adding data will increase the size of transaction payload a little bit, you should probably ballpark it's frequency / size impact based on the data type (int I'm guessing)
- w.r.t the thread id collection, is the thread id only present w/ a profile for those spans? are we always collecting it? what's the overhead of fetching the thread id on each span for non-profiled transactions (or would we even want to?)
Yes, I 100% agree. I wanted to keep this open so folks from different teams can contribute with their thoughts before I move forward |
@AbhiPrasad @k-fish thanks for the feedback, I updated the RFC to be more precise and I added a couple of benchmarks/estimates |
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.
One comment here is do we care about the thread name as well? Or is that too much information to be collecting?
@k-fish brought this up in #75 (comment), I wanted to defer that decision to each product (we definitely need it for profiling), but it seems like it can be added later on. Collecting thread ids will likely determine how we collect thread names in the future, I dont see any strong reason why we would deviate from that tbh |
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 functionality can be helpful for multiple things. I like the idea.
The common data type for a thread identifier is uint64 (also the one we currently use in profiling) and one that we likely should be using. | ||
|
||
Suggestion: | ||
- collect thread.id on spans regardless if profiling is enabled or not |
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.
With that approach, we could highlight in the transaction detail view which spans started on the main thread and which ones did not. Especially for mobile, this is useful. In order to make this happen, we would also need to send the main thread id.
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.
@philipphofmann I think what we do now in profiling is that we send both thread name and tid and rely on the naming to indicate a "main thread". I briefly touched on that in the spec, but would it make sense for you to just set tid and name then as well or would you still want to send tid and main tid?
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 guess tid and name are more flexible. If users set thread names, tid and name would work int that case. tid and main tid would not.
|
||
|
||
Where do we store the data? | ||
There are two places where we could store thread information - either on the span data dict and using setData or by adding new fields to the span object. The former has the benefit of a prexisting API, we would only have to ensure the data is properly set while the later seems more involved and might have infrastructure implications. Since the data dict is public, we would probably want to ensure that users do not override the thread information or discard it if they do - it sounds like acceptable risk to me. |
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.
For file I/O on the main thread we use setData
. We agreed on the key blocked_main_thread
, see https://github.com/getsentry/develop/blob/1c1b528a3442405dad51eedef6e69dbbec296d24/src/docs/sdk/performance/span-data-conventions.mdx?plain=1#L24
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.
All span data documented here: https://develop.sentry.dev/sdk/performance/span-data-conventions/
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 think this change makes a lot of sense to add.
This will be harder to adopt in the sdks since we don’t have span start hooks outside of JS atm, so if an SDK wants to add this they may have to add hooks first to implement (then an thread integration for ex. would always add a thread ID on span start).
Given this is an implementation detail I'm in favor of adding merging this RFC (and updating the develop docs) and then tackling that afterwards with the respective SDK teams.
The Hub is already isolated to threads, so the |
I think this is mostly for mobile right? Where one thread can start and another can finish? |
Yes, exactly. On mobile, it's common to have some UI event on the main thread, then do some work on a background thread as fetching data via HTTP, and then update the UI again on the main thread. |
@JonasBa, what's missing for merging this RFC? We also need span thread information for Mobile Starfish. It would be great to get it merged 😃. |
@philipphofmann we haven agreed where we should store this data on the span model - it seems like span.data could be the right place, any thoughts on that? |
Let's please use |
@AbhiPrasad Yes, lets go with future compatibility here. I would propose we standardize on the flattened |
ec804ab
to
6eac0fc
Compare
Updated the RFC with the conclusion. In case someone has any final comments/thoughts please comment them, else I'll mark this as approved and merge it. |
6ce8d28
to
361d4fe
Compare
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.
If you could make a PR to https://develop.sentry.dev/sdk/performance/span-data-conventions/ would be awesome!
TODO. Rendered RFC