-
Notifications
You must be signed in to change notification settings - Fork 896
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
And semantic conventions for Display Hints #730
Conversation
I would personally advocate for a more ambitious proposal, something closer to a logging or printf library. If we used e.g., Java's TextMessage syntax as a template in the span name, e.g., Name: "FancyOperation: {variableX}-{variableY}" Display name: "FancyOperation: xyz-123" |
| :------------- | :------------------------------------------------------------------ | | ||
|`display.name` | a custom span name to display instead of the span name | | ||
|`display.attribute` | the key to the attribute which should be used as the display name | | ||
|`display.type` | clarifies the type of operation being performed | |
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 we should specify the expected multiplicity of relation of display.type
to the span name. E.g. from how I read this, I would assume that per span name there is only one display.type, but multiple span names may have the same display type. Also I would assume that per display.name there is only one span name. I.e.:
display.name/display.attribute 0..* --- 1 span name 1..* --- 0..1 display.type
| :------------- | :------------------------------------------------------------------ | | ||
|`display.name` | a custom span name to display instead of the span name | | ||
|`display.attribute` | the key to the attribute which should be used as the display name | | ||
|`display.type` | clarifies the type of operation being performed | |
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.
While this may be useful, I wonder if we should really put it in a display namespace. E.g. samplers may also use it, and we may want to specify a recommended display.type/component in each semantic convention.
I'd vote for discussing this in a separate PR.
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.
That's actually one of my questions: is something like component
a required feature that we want to rely on? If so, I would suggest that the named tracer is the place to put this info. In practice, it was confusing to have a component
tag in OpenTracing at the span level, as stapling it onto every span was a burden. In practice, users tended to only tag the first span, leading to heuristics where backends would have to guess the component
.
I think display.name is certainly useful and I like it's simplicity. I'm also not too sure about display.attribute. It seems like somewhere in between display.name and some more powerful display.template. And then again, if we have a |
@@ -16,6 +16,32 @@ Particular operations may refer to or require some of these attributes. | |||
|
|||
<!-- tocstop --> | |||
|
|||
## Display hints | |||
|
|||
UIs are expected to utilize all available semantic conventions when choosing how to |
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.
Could you specify what kind of UI is meant here? "Tracing backends" or the like would make it more clear.
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.
Sure, will do
|
||
The value of `display.attribute` MUST match another attribute key, with the expectation that | ||
the value of that attribute will be used as the span name in the display. If both | ||
`display.name` and `display.attribute` are set, `display.name` should take precedence. |
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.
It's simple enough to understand easily IMHO but a table with all four combinations of name and attribute being set and span name resulting from that would make a nice example.
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.
Added an example, let me know if it is helpful.
| :------------- | :------------------------------------------------------------------ | | ||
|`display.name` | a custom span name to display instead of the span name | | ||
|`display.attribute` | the key to the attribute which should be used as the display name | | ||
|`display.type` | clarifies the type of operation being performed | |
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 do like the idea of display.name
and display.attribute
but I'm not so sure about display.type
.
Could please you elaborate on the problem this solves, provide examples and explain the implications it is expected to have when set?
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.
sounds like there are questions about type
in general but for reference this is a replacement for the component
tag, which was used to name the library/class/package from which the span originated.
https://github.com/opentracing/specification/blob/master/semantic_conventions.md#span-tags-table
To follow up, it sounds like this is the current feedback:
@jmacd your scheme does look interesting, but no one has requested templating or shown a need for it yet. Given the extra complexity and bikeshedding, if you don't mind I'd like to stick to a simple convention for the time being. |
the value of that attribute will be used as the span name in the display. If both | ||
`display.name` and `display.attribute` are set, `display.name` should take precedence. | ||
|
||
The `display.type` field may match an appropriate namespace (db, http, faas, etc) or be |
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 don't see how this is specific to display
I don't understand the difference between |
@iNikem I added an example to help with this. Basically, when the need for low cardinality results in a span name which is not very helpful to the viewer – like "HTTP GET" – an additional display name can be added for legibility without worry about the impact on grouping or indexing. Backends can of course pick a display name for themselves, but the display hint is still useful, especially in the case of non-standard spans where a backend will have no heuristic for constructing a name on its own. |
Ok, I see the benefit of this when there is no semantic convention for a given span. Ack. |
HTTP GET does come from a semantic convention. |
In this example, the low cardinality span name could be replaced with something more readable: | ||
|
||
* **Span Name**: "HTTP GET" | ||
* **display.name**: "GET /account/123" |
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 is a rather contrived example. The backends already know when the span is for HTTP from other semantic attributes, and can easily change the display name to {method} {url}
. What other examples do you have that do not fall into the same pattern (e.g. the same approach can be used for DB spans, etc.).
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.
Yes, in some of these cases it is possible for a backend to construct a better display name, but will backends actually choose to override the span name by default in these cases? Adding a display hint when the span name is already known to be bad for human comprehension would make life easier for everyone trying to consume our data.
To give other examples, there are two use cases where a backend cannot derive the appropriate name:
- Custom spans, where the backend has no way of knowing the pattern.
- A pattern provided by the user, for their own clarity. For example, by adding hints in a collector pipeline.
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 examples in the specification should showcase real scenarios. An example with HTTP span just doesn't make any sense to me, because the new attributes do not provide anything that's not already available to the backend if the span follows the HTTP semantic conventions. Using it as example in the spec is an example of what not to do, since it just wastes bandwidth. Not to mention that HTTP instrumentation is usually done by reusable middleware, not by the end user code.
I am not opposed to the display.name attribute, but let's show an example where this attribute enables something that's difficult or even impossible otherwise.
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.
Another aspect to think about is that introducing this semantic convention explicitly encourages a pattern of providing data that is clearly static (esp. if using display.template
) and associated with the span name. We never talked about it before, but a lot of span attributes are static for a given span, but the trace API provides no facilities to optimize for that (similar to bound instruments in the metrics API). Booked #734.
Hmm, I think there are two main use cases for the display name:
And also: Of course, for all these reasons, to make the display name actually add value, the additional condition "span name is not good as display name" must also hold, but as shown in #557 this is often the case. |
Please provide real examples that really need that feature. |
|
||
* **Span Name**: "HTTP GET" | ||
* **display.name**: "GET /account/123" | ||
* **display.type**: "HipClient" |
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 don't think this is a clear example of an "operation being performed". Perhaps some other examples would help? What about a server request, message queue, or internal span?
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.
Happy to add more examples. Is there anything in particular that makes this confusing?
I provide some examples here: #730 (comment)
Does this make sense? I should clarify that display hints do not need to be set on every span, only to resolve the extenuating circumstances which are causing issues to be opened by people complaining about their span names. |
Another high-level concern with this proposal: what does "display name" even mean? Especially given the examples, the display name could easily turn into high-cardinality not suitable for aggregations. Which is fine if we only want to use it in the Gantt chart trace view, but suppose the tracing UI has a view of top-10 slowest endpoints for a service. Which name will be used in such view? It can't use the display names because those could be different even for the spans of the same class. @tedsuo said:
This is a similar problem. What happens to the real span name if there is an alternative "display name"? Will the UI need to show both of them, especially because only the real name can be used in the aggregate views? |
@yurishkuro well, I really do not want to get into the business of dictating how backends display information, so I wouldn't say they need to use it anywhere. The purpose is to allow the instrumentation author and the end user a means for communicating descriptive information. How a backend makes use of that information is up to them. @tylerbenson you were fielding a request from the Java SIG for a |
@tedsuo a few come to mind:
As for |
@tylerbenson fwiw, examples like db calls and message queues are still something that can already be done by the backends based on the existing semantic conventions, without requiring instrumentation to worry about the display concerns. |
That's true... from a vendor's perspective. But say I'm trying to write instrumentation for some random RPC framework that my backend doesn't already have support for. I can only really control the instrumentation. |
I see the display name only as a fallback if a backend does not recognize the semantic convention of the span. But I think that's useful. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR is controversial:
Also, there is no support from open source backend for this feature - neither Zipkin nor Jaeger supports it as far as I know. So we cannot demonstrate usability. If there is a strong reason to push for this for GA, please re-open. I'd suggest to split to smaller PRs (even though trivial) as they cause a lot of discussions. |
I think this is an unfortunate choice by .NET, as they are making a choice here that OpenTelemetry spec does not make, and in fact #557 seemed to go into the other direction last time I checked. This is more an argument for instead of against this PR IMHO. |
I'm wondering what should we do for things that were shipped by .Net already - do we have our hands tied there? |
If display name is a conflict we can find different names, descriptive name comes to mind. That being said the tension between high vs low cardinality had come up many times - I'm sad if we table this. In OTel, maintainers only point out problems but never help come up with solutions since there's no real empathy :(
I looked into .Net randomly and saw that the ID generation format may be an enum in the runtime. It didn't seem customizable at first glance but I'm praying our hands aren't tied in being able to change things like that or we will be stuck. |
I suggest sticking to just "Name". Before a final decision on #557 is made, inventing a different name is a gamble.
I feel like for this issue, we don't need a (new) solution but rather "just" a decision. Pros & cons have been laid out, this PR was part of a possible solution, it has been declined/postponed.
I'm not sure what ID generation has to do with this? |
It seems to me that decision has been made, no? PR was declined === decision was made not to implement this proposal. |
Not about this PR specifically sorry, but it's a worry I had in general about the concept of having no recourse for things already shipped by .Net which could tie hands quite strongly.
The decision to drop everything but GA was made in this issue I guess #792 It's just a couple of days ago but this is when the fallout starts. I'm amazed everyone is so happy to say no without any sort of empathy, just cherry picked facts to satisfy their own pride, not the other side - that's not how OSS communities work. |
If you have a deadline to make, you cannot say "yes" to everything. You have to focus and make a release. This means moving a lot of non-breaking changes to later date. Because they CAN be revisited later. Citing closing comment: "If there is a strong reason to push for this for GA, please re-open. I'd suggest to split to smaller PRs (even though trivial) as they cause a lot of discussions." |
I'm sorry you feel this way. I do sympathize to this issue and believe we need to address it. However maintainers and TC is not to force decision, rather to collect requirements and opinions and break ties. As a .NET maintainer I can express opinion. As a TC member I need to look at number of approvals and agreements and account for the release timeline. I will try to split my comments in future into my opinion as maintainer and PR/issue resolution as TC member. |
This PR adds several display hints to resolve issues related to identifying spans in a UI.
display.name
for overriding the span name with a custom name.display.attribute
for overriding the span name with the value of another attribute.display.type
for an additional display grouping, adding back in the functionality formerly provided by thecomponent
tag from OpenTracing.These hints are optional; there is no requirement that instrumentation provide them, or that UIs respect them.
Related issues
#531
#557