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

Add chain ID attribute #228

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

lameiraatt
Copy link

This OTEP proposes the addition of a chain ID attribute to spans and logs that supports data processing/analysis and can be implemented as part of an extension to the default OTel SDK.

Feedback welcome and appreciated! 😊

@lameiraatt lameiraatt requested a review from a team March 22, 2023 11:14
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 22, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@lameiraatt lameiraatt changed the title OTEP: Add chain ID attribute Add chain ID attribute Mar 27, 2023
3. Sum.

Without chain ID, we'd have to first create the tree, using span ID and parent span ID information.
When processing big data sets, with millions of data points, generated by thousands of processes, this can have a considerable performance impact.
Copy link

@cartermp cartermp Apr 10, 2023

Choose a reason for hiding this comment

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

this can have a considerable performance impact.

I'd be curious about the impact based on some non-contrived examples. In my work we definitely have some big traces from some customers, but nothing to the point where this significantly taxed the tracing backend. Is this like a "google has this problem" kinda thing, or do others? It's clear that this can help performance for a tracing backend, but I'm just not clear if this is something that backends will benefit from broadly or only when working with google-sized data.

s/google/some-other-enormous-tech-system-that-requires-bespoke-tooling

Copy link
Member

Choose a reason for hiding this comment

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

"millions of data points" is not necessarily a Google-scale problem. I've seen smaller companies processing tracing volume comparable to hyper-scalers, the difference is that hyper-scalers just have to sample more.

And to me, the challenge with processing mentioned here is not necessarily with the data volume, but with expressiveness of the query languages. Aside from TempoDB QL I am not aware of any QL for traces that allows to express queries against the graph (you can do one-level parent/child in SQL, anything more becomes too cumbersome to express). The chain-id largely solves this issue by allowing to query into tabular data set without any aggregation of events into traces.

Copy link

@electron0zero electron0zero Apr 13, 2023

Choose a reason for hiding this comment

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

Thank you Yuri. @cartermp, see TraceQL Design Proposal for more details.

@bputt-e
Copy link

bputt-e commented Apr 21, 2023

Does this proposal help with linked spans? Let's say I have 500 traces that get 'merged' into a new trace....What would that new trace look like, would it have any 'historical' knowledge to add any chain info or would it be considered a new trace and have nothing starting out.

@joe-elliott
Copy link

fwiw, Tempo achieves similar functionality by computing the nested set model for the tree upon storage. if this were adopted by OTEL it would make calculating the nested set quite a bit easier.

the UUID passed seems to be used to track the root process instance. imo it would be better to make this separate from the concept of the hierarchy. perhaps two different attributes that could be independently controlled:

Perhaps something like:
chain.hierarchy
chain.root

i'm also a bit concerned about traces with extreme depth creating enormous chain ids. perhaps a configuration option for max depth recorded would be worthwhile?

@tedsuo
Copy link
Contributor

tedsuo commented Jul 31, 2023

@lameiraatt we are cleaning up stale OTEP PRs. If there is no further action at this time, we will close this PR in one week. Feel free to open it again when it is time to pick it back up.

@tedsuo tedsuo added the stale This issue or PR is stale and will be closed soon unless it is resurrected by the author. label Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p2 stale This issue or PR is stale and will be closed soon unless it is resurrected by the author. triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants