-
Notifications
You must be signed in to change notification settings - Fork 272
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
Remove misleading comments for Span in trace.proto #408
Conversation
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.
Much clearer 👍 🙇
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. |
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:
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.
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).
Factually true, but devoid of any semantic meaning of "linked", so does not improve understanding.
There is always a root span, given the structure of the model (
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.
A very random comment. |
@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. |
Resolves #403.