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

Remove misleading comments for Span in trace.proto #408

Merged

Conversation

arminru
Copy link
Member

@arminru arminru commented Jun 22, 2022

Resolves #403.

Copy link

@b-viguier b-viguier left a comment

Choose a reason for hiding this comment

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

Much clearer 👍 🙇

@yurishkuro
Copy link
Member

Why not just delete this whole paragraph? The conceptual model of what trace is should be defined in the spec. The description here is so dumbed down that it's almost meaningless. It doesn't use any of the right words like causality or happens-before.

@arminru
Copy link
Member Author

arminru commented Jun 22, 2022

Why not just delete this whole paragraph? The conceptual model of what trace is should be defined in the spec. The description here is so dumbed down that it's almost meaningless. It doesn't use any of the right words like causality or happens-before.

@yurishkuro This would be reasonable as well but most if not all of the other fields have a similarly basic description and we should either remove all of them or leave them as-is. This PR would at least remove the potential for confusion by the current wording.

@yurishkuro
Copy link
Member

I don't mind basic description. This one is not, it pulls together some random aspects of the model, without a nuanced discussion required to understand them.

For example:

Span represents a single operation within a trace.

Fine, albeit recursive. "A single operation performed by a single component of the system" is more precise. Would be fine as the only description for Span type.

Spans can be nested to form a trace tree.

False. Nesting traditionally refers to semantic scope that implies a strong causality that once you exit the child scope you're back into the parent scope. That is neither the guarantee nor (often) the assumption implied by the parent-child relationship between spans (e.g. in async communications).

Spans may also be linked to other spans from the same or different trace and form graphs.

Factually true, but devoid of any semantic meaning of "linked", so does not improve understanding.

Often, a trace contains a root span that describes the end-to-end latency

There is always a root span, given the structure of the model (span.parentId), so why "often"?

A trace can also contain multiple root spans, or none at all.

Because of poor language above, this now mixes together the span links and parent-child relationships. They are not equivalent, and if we only go by parent-child, then multiple roots are not possible - unless we count partially collected traces (but I wouldn't confuse completeness with multi-roots). If we extend causality capture to links as well, then the effect is much broader than multi-roots, they allow modeling fork-join within a single trace just as well.

Spans do not need to be contiguous - there may be gaps or overlaps between spans in a trace.

A very random comment.

@arminru
Copy link
Member Author

arminru commented Jun 27, 2022

@yurishkuro Thanks for dissecting the individual statements. I agree that having a basic but correct description is better than a more extensive but misleading one. I don't think (hope) that people would read the proto file first to understand what traces and spans are about but would rather, for example, read the OTel docs and/or spec on it first. I'll cut down the description to the very first statement as you suggested.

@arminru arminru changed the title Clarify "multiple root spans" comment in trace.proto Remove misleading comments for Span in trace.proto Jun 27, 2022
@arminru arminru merged commit 95cf8f4 into open-telemetry:main Jun 28, 2022
@arminru arminru deleted the multiple-root-spans branch June 28, 2022 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Doc] Number of root spans per trace
5 participants