rfc(feature): Span thread information #75

JonasBa · 2023-02-15T14:57:18Z

TODO. Rendered RFC

text/0075-span-thread-id.md

k-fish

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?)

JonasBa · 2023-02-17T14:01:59Z

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

JonasBa · 2023-03-07T13:27:39Z

@AbhiPrasad @k-fish thanks for the feedback, I updated the RFC to be more precise and I added a couple of benchmarks/estimates

Zylphrex

One comment here is do we care about the thread name as well? Or is that too much information to be collecting?

JonasBa · 2023-03-07T15:43:53Z

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

philipphofmann

This functionality can be helpful for multiple things. I like the idea.

philipphofmann · 2023-03-15T10:59:11Z

text/0075-span-thread-id.md

+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


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.

@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?

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.

philipphofmann · 2023-03-15T11:03:47Z

text/0075-span-thread-id.md

+
+
+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.


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

All span data documented here: https://develop.sentry.dev/sdk/performance/span-data-conventions/

AbhiPrasad

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.

sl0thentr0py · 2023-03-16T14:11:39Z

The Hub is already isolated to threads, so the hub.scope.span reference is always thread local. How can one thread start a span and another end it? I don't know how this applies in mobile where there's only global Hub but this is the behaviour in all web SDKs.
cc @AbhiPrasad

AbhiPrasad · 2023-03-16T14:25:42Z

I think this is mostly for mobile right? Where one thread can start and another can finish?

philipphofmann · 2023-03-20T10:55:10Z

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.

philipphofmann · 2023-10-16T08:09:58Z

@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 😃.

JonasBa · 2023-10-16T13:57:11Z

@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?

AbhiPrasad · 2023-10-16T14:02:01Z

Let's please use span.data and the same naming scheme that opentelemetry does. I'm working on #116 which should hopefully make it clearer to future contributors where to place this kind of data.

JonasBa · 2023-10-16T14:19:40Z

@AbhiPrasad Yes, lets go with future compatibility here. I would propose we standardize on the flattened thread.id: string and thread.name: string on span data.

JonasBa · 2023-10-17T18:12:51Z

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.

text/0043-frame-module.md

AbhiPrasad

If you could make a PR to https://develop.sentry.dev/sdk/performance/span-data-conventions/ would be awesome!

text/0075-span-thread-id.md

JonasBa force-pushed the rfc/span-thread-id branch from 46e9828 to 52a352b Compare February 15, 2023 14:57

Zylphrex reviewed Feb 15, 2023

View reviewed changes

text/0075-span-thread-id.md Show resolved Hide resolved

text/0075-span-thread-id.md Outdated Show resolved Hide resolved

JonasBa marked this pull request as ready for review February 16, 2023 13:41

AbhiPrasad reviewed Feb 16, 2023

View reviewed changes

text/0075-span-thread-id.md Outdated Show resolved Hide resolved

AbhiPrasad reviewed Feb 16, 2023

View reviewed changes

text/0075-span-thread-id.md Outdated Show resolved Hide resolved

k-fish reviewed Feb 16, 2023

View reviewed changes

Zylphrex reviewed Mar 7, 2023

View reviewed changes

philipphofmann reviewed Mar 15, 2023

View reviewed changes

JonasBa changed the title ~~rfc(feature): Span thread id~~ rfc(feature): Span thread information Mar 15, 2023

AbhiPrasad approved these changes Mar 16, 2023

View reviewed changes

sl0thentr0py approved these changes Mar 16, 2023

View reviewed changes

AbhiPrasad mentioned this pull request Aug 2, 2023

[Starfish] Collect more data on mobile spans to support drill-downs on starfish getsentry/team-sdks#22

Open

JonasBa added 5 commits October 17, 2023 13:35

rfc(feature): Span thread id

99b6104

spans: add initial pass on thread_id rfc

c03422b

update spans poc

8fda4b8

small update

c6ee922

rfc(0075): add conclusion

6eac0fc

JonasBa force-pushed the rfc/span-thread-id branch from ec804ab to 6eac0fc Compare October 17, 2023 17:59

AbhiPrasad reviewed Oct 17, 2023

View reviewed changes

text/0043-frame-module.md Outdated Show resolved Hide resolved

rfc(0075): update conclusion

361d4fe

JonasBa force-pushed the rfc/span-thread-id branch from 6ce8d28 to 361d4fe Compare October 17, 2023 20:14

AbhiPrasad approved these changes Oct 17, 2023

View reviewed changes

text/0075-span-thread-id.md Outdated Show resolved Hide resolved

rfc(0075): remove unresolved question

5853fcf

JonasBa merged commit 177c8b3 into main Oct 18, 2023

JonasBa deleted the rfc/span-thread-id branch October 18, 2023 22:13

Zylphrex mentioned this pull request Mar 18, 2024

feat(profiling): Add thread data to spans getsentry/sentry-python#2843

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rfc(feature): Span thread information #75

rfc(feature): Span thread information #75

JonasBa commented Feb 15, 2023 •

edited

Loading

k-fish left a comment

JonasBa commented Feb 17, 2023

JonasBa commented Mar 7, 2023 •

edited

Loading

Zylphrex left a comment

JonasBa commented Mar 7, 2023

philipphofmann left a comment

philipphofmann Mar 15, 2023

JonasBa Mar 15, 2023

philipphofmann Mar 20, 2023

philipphofmann Mar 15, 2023

AbhiPrasad Mar 15, 2023

AbhiPrasad left a comment •

edited

Loading

sl0thentr0py commented Mar 16, 2023

AbhiPrasad commented Mar 16, 2023

philipphofmann commented Mar 20, 2023

philipphofmann commented Oct 16, 2023

JonasBa commented Oct 16, 2023

AbhiPrasad commented Oct 16, 2023

JonasBa commented Oct 16, 2023

JonasBa commented Oct 17, 2023

AbhiPrasad left a comment



		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.

rfc(feature): Span thread information #75

rfc(feature): Span thread information #75

Conversation

JonasBa commented Feb 15, 2023 • edited Loading

k-fish left a comment

Choose a reason for hiding this comment

JonasBa commented Feb 17, 2023

JonasBa commented Mar 7, 2023 • edited Loading

Zylphrex left a comment

Choose a reason for hiding this comment

JonasBa commented Mar 7, 2023

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann Mar 15, 2023

Choose a reason for hiding this comment

JonasBa Mar 15, 2023

Choose a reason for hiding this comment

philipphofmann Mar 20, 2023

Choose a reason for hiding this comment

philipphofmann Mar 15, 2023

Choose a reason for hiding this comment

AbhiPrasad Mar 15, 2023

Choose a reason for hiding this comment

AbhiPrasad left a comment • edited Loading

Choose a reason for hiding this comment

sl0thentr0py commented Mar 16, 2023

AbhiPrasad commented Mar 16, 2023

philipphofmann commented Mar 20, 2023

philipphofmann commented Oct 16, 2023

JonasBa commented Oct 16, 2023

AbhiPrasad commented Oct 16, 2023

JonasBa commented Oct 16, 2023

JonasBa commented Oct 17, 2023

AbhiPrasad left a comment

Choose a reason for hiding this comment

JonasBa commented Feb 15, 2023 •

edited

Loading

JonasBa commented Mar 7, 2023 •

edited

Loading

AbhiPrasad left a comment •

edited

Loading