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

And semantic conventions for Display Hints #730

Closed
wants to merge 6 commits into from

Conversation

tedsuo
Copy link
Contributor

@tedsuo tedsuo commented Jul 23, 2020

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 the component tag from OpenTracing.

These hints are optional; there is no requirement that instrumentation provide them, or that UIs respect them.

Related issues

#531
#557

@jmacd
Copy link
Contributor

jmacd commented Jul 23, 2020

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}"
Labels:
variableX: xyz
variableY: 123

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 |
Copy link
Member

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 |
Copy link
Member

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.

Copy link
Contributor Author

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.

@Oberon00
Copy link
Member

@jmacd

I would personally advocate for a more ambitious proposal

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 display.type, you might configure a per-type template on the backend instead of sending the same string with every span.

@@ -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
Copy link
Member

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.

Copy link
Contributor Author

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.
Copy link
Member

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.

Copy link
Contributor Author

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 |
Copy link
Member

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?

Copy link
Contributor Author

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

@tedsuo
Copy link
Contributor Author

tedsuo commented Jul 23, 2020

To follow up, it sounds like this is the current feedback:

display.name is uncontroversial.

display.attribute is a little fuzzier. To clarify the value, this is for situations where the value of the attribute may not be known or easily accessible by the code which would like to change the display name. One way to think about it is thatdisplay.name is a value, and display.attribute is a pointer to a value. There were requests for this in the past; if it is overkill I can remove it for now.

display.type is controversial. This is meant to replace OpenTracing component tag, but in an optional manner. I do see one suggestion that we may want to rely something like component for other purposes. If that is the case, I don't beleive a span attribute is the correct way to handle component identification. I would suggest that the named tracer is the place to put this info. In practice, it was confusing in OpenTracing to have a component span tag, as stapling that tag onto every single 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. If this debate needs to continue, I can remove display.type for now.

@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
Copy link
Contributor

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

@iNikem
Copy link
Contributor

iNikem commented Jul 23, 2020

I don't understand the difference between span.name and display.name, when they are both set by instrumented library. I think that I still fail to see the reason why display name and grouping key should be different.

@tedsuo
Copy link
Contributor Author

tedsuo commented Jul 23, 2020

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

@iNikem
Copy link
Contributor

iNikem commented Jul 23, 2020

Ok, I see the benefit of this when there is no semantic convention for a given span. Ack.

@Oberon00
Copy link
Member

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"
Copy link
Member

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

Copy link
Contributor Author

@tedsuo tedsuo Jul 23, 2020

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.

Copy link
Member

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.

Copy link
Member

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.

@Oberon00
Copy link
Member

@iNikem

when there is no semantic convention for a given span

@yurishkuro

The backends already know when the span is for HTTP from other semantic attributes, and can easily change the display name

Hmm, I think there are two main use cases for the display name:

  1. When there really is no semantic convention to which the span belongs, as @iNikem said
  2. When the backend does not know the semantic convention

And also:
3. When the information on the span is lacking attributes the backend would "require" to format a display name

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.

@yurishkuro
Copy link
Member

When there really is no semantic convention to which the span belongs

Please provide real examples that really need that feature.


* **Span Name**: "HTTP GET"
* **display.name**: "GET /account/123"
* **display.type**: "HipClient"
Copy link
Member

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?

Copy link
Contributor Author

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?

@tedsuo
Copy link
Contributor Author

tedsuo commented Jul 23, 2020

I provide some examples here: #730 (comment)

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.

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.

@carlosalberto carlosalberto added spec:trace Related to the specification/trace directory area:api Cross language API specification issue area:semantic-conventions Related to semantic conventions labels Jul 24, 2020
@yurishkuro
Copy link
Member

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:

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?

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?

@tedsuo
Copy link
Contributor Author

tedsuo commented Jul 24, 2020

@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 description or details field. Do you have examples from instrumentation where you would like to use this feature?

@tylerbenson
Copy link
Member

@tedsuo a few come to mind:

  • database calls: we might consider including some/all of a query in the details, but would be high cardinality for the name.
  • message queues: it's possible that the queue name could be a temporary name.
  • url for both http client/server calls (already covered above)

As for display.type, I might ask why you might want to distinguish http client calls using framework Foo, vs Bar. This is why I was suggesting instead consider something like http-client.request. I likely don't care so much which client is making the call.

@yurishkuro
Copy link
Member

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

@tylerbenson
Copy link
Member

tylerbenson commented Jul 28, 2020

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.

@Oberon00
Copy link
Member

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.

@github-actions
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@SergeyKanzhelev
Copy link
Member

This PR is controversial:

  • display.name will conflict with the .NET DisplayName which is what Span.Name means. So if .NET to support this concept - it will be very confusing. Secondly, feedback in the comments that it's unclear how exactly libraries will set it up.
  • dsplay.attribute confirmed to have fuzzy meaning and un-proven by PR author.
  • display.type caused many questions and doesn't seem to be agreed on.

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.

@Oberon00
Copy link
Member

Oberon00 commented Aug 14, 2020

@SergeyKanzhelev

display.name will conflict with the .NET DisplayName which is what Span.Name means

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.

@carlosalberto
Copy link
Contributor

I'm wondering what should we do for things that were shipped by .Net already - do we have our hands tied there?

@anuraaga
Copy link
Contributor

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'm wondering what should we do for things that were shipped by .Net already - do we have our hands tied there

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.

@Oberon00
Copy link
Member

If display name is a conflict we can find different names, descriptive name comes to mind.

I suggest sticking to just "Name". Before a final decision on #557 is made, inventing a different name is a gamble.

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

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 looked into .Net randomly and saw that the ID generation format may be an enum in the runtime.

I'm not sure what ID generation has to do with this?

@iNikem
Copy link
Contributor

iNikem commented Aug 14, 2020

we don't need a (new) solution but rather "just" a decision

It seems to me that decision has been made, no? PR was declined === decision was made not to implement this proposal.

@anuraaga
Copy link
Contributor

I'm not sure what ID generation has to do with this?

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.

Pros & cons have been laid out, this PR was part of a possible solution, it has been declined/postponed.

decision was made not to implement this proposal.

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.

https://opensource.guide/

@iNikem
Copy link
Contributor

iNikem commented Aug 14, 2020

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

@SergeyKanzhelev
Copy link
Member

SergeyKanzhelev commented Aug 14, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants