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

Define what required means for a semantic convention #653

Closed
bogdandrutu opened this issue Jun 12, 2020 · 24 comments
Closed

Define what required means for a semantic convention #653

bogdandrutu opened this issue Jun 12, 2020 · 24 comments
Assignees
Labels
area:semantic-conventions Related to semantic conventions priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:resource Related to the specification/resource directory spec:trace Related to the specification/trace directory

Comments

@bogdandrutu
Copy link
Member

bogdandrutu commented Jun 12, 2020

We started to mark fields as required in the semantic conventions but we have not defined very well what does it mean:

  1. Without this field the Span data are not usable?
  2. Without this field the Span will not be identified correctly?
  3. They are more important than non-required, but everything will still work?

There are multiple ways to define required and we need to define it before we add required to a lot of the semantic conventions that we add.

@bogdandrutu bogdandrutu added area:semantic-conventions Related to semantic conventions spec:resource Related to the specification/resource directory spec:trace Related to the specification/trace directory labels Jun 12, 2020
@Oberon00
Copy link
Member

Oberon00 commented Jun 12, 2020

About "we started to mark fields as required": I think apart from the Resource conventions, every semantic convention has been using required from the start.

About the issue itself: 👍 We should really define this. My definition would be: If a backend supports this semantic convention at all, it must be able to recognize the span as conforming to it and do any basic special handling for such spans if all required fields are set.

@bogdandrutu
Copy link
Member Author

@Oberon00 Please try to rephrase your definition from the user perspective. What is the problem if I don't set a required field? May the backend reject it? May the backend analyze it wrong?

@anuraaga
Copy link
Contributor

My 2c is the opposite I think, the required / optional should apply to instrumentation, not backends / collectors. So, an OpenTelemetry SDK instrumentation is required to set a field that is marked such in the semantic conventions. This provides good guidelines for instrumentation authors on what is really important when writing instrumentation, even if it requires going through some hoops as the case may be. And if even then for some reason an instrumentation can't provide a required field, they can loop back to the community for advice and understanding the upstream impact. The key point is there's nothing a backend can do if instrumentation is not providing the information we expect it to.

Having backends / collectors use this information, for example to drop spans, seems unhelpful. Even missing some required semantic convention, presumably the span exists because it provides some sort of information and just losing it seems to be less useful to user than keeping it, with potential issues when rendering in a UI or whatever. But this could also be left to the backend in deciding how important the semantic convention is to their UX.

While perhaps the same expectation as instrumentation can be applied to backend, it seems too restrictive to me. For vendors supporting OpenTelemetry, we don't currently [mention](
https://github.com/open-telemetry/opentelemetry-specification/blob/df484976656c8fc9e8575011dd08e01b3b40615e/specification/vendors.md#supports-opentelemetry} semantic conventions. This seems ok to me since what UX the backend eventually exposes to a user is still generally going to be up to them, and having requirements on the instrumentation side means backends will be able to confidently create their tailored flows, or not if they choose to.

So I guess out of the three options in the original post, I'd lean towards 3). Though not "everything" but "somethings" will still work.

@carlosalberto
Copy link
Contributor

So, an OpenTelemetry SDK instrumentation is required to set a field that is marked such in the semantic conventions. This provides good guidelines for instrumentation authors

I think this is already the current expectation.

My two cents: I think Spans should not be dropped, and the required attributes should be a highly encouraged piece of information, and its absence might decrease the functionality/information offered in the backend, but not considered an error in itself.

I'd also lean towards option 3)

@arminru
Copy link
Member

arminru commented Jun 16, 2020

I don't see how "3) They are more important than non-required, but everything will still work" should work in practice.
If you don't set any of the required http.* attributes on a span representing an HTTP request, how should the backend figure out that this is an HTTP request and what the target of it is? If you have a database span but no information about which database is being called, how helpful is this information for detecting bottlenecks or outages?
I see required attributes as a minimal set of attributes required to properly detect the kind and target of requests, to distinguish it from other requests and to be able to get reasonable information out of it for analyzing and reasoning about it.

@Oberon00
Copy link
Member

Repeating my definition for further explanation:

If a backend supports this semantic convention at all, it must be able to recognize the span as conforming to it and do any basic special handling for such spans if all required attributes are set.

I think this is a more specific variant of "3) They are more important than non-required, but everything will still work?"

Of course "everything" cannot work, e.g. you can't see the HTTP method if you don't set it. And the backend might display the span as a generic "unknown server-side span" or maybe, if "net.*" attributes are set, it might fall back to some more generic handling, e.g. classify the span as "incoming TCP request" insted of "incoming HTTP".

@bogdandrutu You asked me to reword my definition from the users perspective, but I don't know which user you had in mind there.

From the instrumentation writer's perspective, you should set all required attributes if at all possible. But it is not a fatal error if you miss some, it might still be better than not creating the span at all. But this is very vague. After all, the attributes are meant to be processed by the backend to be then displayed to some end-user, or transformed into nice graphs or alerts.

From the end-users perspective who is looking at the backend's UI, a slight rephrasing to some user-manual-like style could be:

If a span is not displayed in the way you expect it (for example a HTTP span is not found under the route it belongs to, or is diplayed with the wrong icon), this is probably because of missing required attributes. Your backend might be able to provide debugging help (e.g. display which attributes would be missing for a certain type/class of spans).

My definition was not meant to say that the backend should reject the span actively if it does not conform to the semantic conventions. I expect all backends to operate with best-effort. But if a backend wants to claim that it supports OpenTelemetry semantic conventions, it must at least be able to do any special handling for Spans with semantic conventions if only the required attributes are set.

@yurishkuro
Copy link
Member

yurishkuro commented Jun 17, 2020

From the instrumentation writer's perspective, you should set all required attributes if at all possible. But it is not a fatal error if you miss some, it might still be better than not creating the span at all.

I think this is indeed the current interpretation, and required is the wrong word for it, because clearly it's NOT required for things to work. I find it very hard to assign a significant weight to distinguish attributes on the optional <= recommended <= required (#604 (comment)) spectrum. Do we even need this distinction? The more attributes the instrumentation provides, the more powerful analysis can be done. It's really a gray scale, not a discrete classification.

For comparison, OpenTracing Semantic Conventions did not use required/optional, and I don't recall hearing much complaints about that. It's much more important to establish what each attribute means and its expected value type than to discuss its optionality.

My proposal is to drop optional/required altogether for v1.

@bogdandrutu
Copy link
Member Author

@yurishkuro great minds think alike :) #604 (comment)

@bogdandrutu
Copy link
Member Author

@anuraaga

My 2c is the opposite I think, the required / optional should apply to instrumentation, not backends / collectors.

That was exactly my point, we need to define this from the user perspective.

@yurishkuro
Copy link
Member

@bogdandrutu sorry, I should've attributed the "recommended" term, comment updated.

@Oberon00
Copy link
Member

Oberon00 commented Jun 17, 2020

@bogdandrutu Please define which user you mean. The one who looks at the Web UI of the backend, or the "user" that codes instrumentations?

@mtwo mtwo added the release:required-for-ga Must be resolved before GA release, or nice to have before GA label Jun 30, 2020
@bogdandrutu bogdandrutu self-assigned this Jul 7, 2020
@andrewhsu andrewhsu added the priority:p1 Highest priority level label Jul 17, 2020
@bogdandrutu
Copy link
Member Author

I will propose for the moment to remove the notion of "required" from all the semantic conventions until we have time to think and design the right solution. This is a required issue for GA and this is the proposed resolution. Please vote.

@Oberon00
Copy link
Member

Oberon00 commented Jul 22, 2020

I'm worried that if we just remove it (instead of coming up with a proper definition now), we will not have much leeway how to define "required" ("important"?) later. If we made anything actually "required" (e.g.: typed API will not compile if you don't supply required attributes), it would be a breaking change. I agree that in the current situation "required" does not add much value though.

@Oberon00
Copy link
Member

Oberon00 commented Jul 22, 2020

Probably what we will end up with is something purely descriptive like "if you don't set one of these attributes, most backends we know will not detect this span as HTTP request, so please keep that in mind". I think that's OK. We could consider having some table in some wiki or something where we add what's required per backend (and which features won't work on that backend if you don't provide that attribute). I assume backends will provide that information in their own documentation too, but having some overview would be very helpful for instrumentation writers.
EDIT: For the typed span API that would mean that we effectively lose the "required" information and thus lose the opportunity to prevent some instrumentation mistakes already at compile time.

@thisthat
Copy link
Member

To see how we implemented typed spans and the checks for missing attributes, please have a look at this PR: open-telemetry/opentelemetry-java-instrumentation#502

@tedsuo
Copy link
Contributor

tedsuo commented Aug 17, 2020

Following up on this. "Required" fields on a semantic convention may not always be enforceable, depending on language (though we should eventually provide helpers in all languages to make it easier to correctly create these standard attributes). Rather than remove the "required" terminology, can we simply say that a convention is "incomplete" if it is missing a required field, and leave it at that? Understanding which attributes are required to complete a convention, and which are optional, is very helpful to users when trying to understand these conventions.

@dyladan
Copy link
Member

dyladan commented Aug 18, 2020

Following up on this. "Required" fields on a semantic convention may not always be enforceable, depending on language (though we should eventually provide helpers in all languages to make it easier to correctly create these standard attributes). Rather than remove the "required" terminology, can we simply say that a convention is "incomplete" if it is missing a required field, and leave it at that? Understanding which attributes are required to complete a convention, and which are optional, is very helpful to users when trying to understand these conventions.

I came here to say basically this. If some library wants to claim that it exports opentelemetry metrics, it would be very helpful when vetting the library to have some minimum set of attributes you know will be exported. I think required attributes are the minimum set of attributes that must be supported by an instrumentation library in order to claim that it supports some version of opentelemetry.

@bogdandrutu
Copy link
Member Author

If that is the case we should reconsider what are the required semantics, because right now there are a lot of required fields

@andrewhsu
Copy link
Member

@bogdandrutu talked about this at the maintainers mtg today, labelling asP2 issue since it is not strictly breaking trace api.

@andrewhsu andrewhsu added priority:p2 Medium priority level and removed priority:p1 Highest priority level labels Aug 31, 2020
@Oberon00
Copy link
Member

Oberon00 commented Sep 1, 2020

Yes, we can declare the first GA version does not have stable semantic conventions yet. Which is a bit of a bummer, but can be fixed in a later 1.x version.

@andrewhsu andrewhsu added release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs and removed release:required-for-ga Must be resolved before GA release, or nice to have before GA labels Sep 25, 2020
@andrewhsu
Copy link
Member

from the issue triage meeting today with TC, allowed for GA for editorial change only

@lmolkova
Copy link
Contributor

Should we consider it done with #2522 ?

@Oberon00
Copy link
Member

Oberon00 commented Sep 8, 2022

Since nobody objected, I guess this issue can be closed?

@arminru
Copy link
Member

arminru commented Sep 8, 2022

Done in #2522.

@arminru arminru closed this as completed Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions priority:p2 Medium priority level release:allowed-for-ga Editorial changes that can still be added before GA since they don't require action by SIGs spec:resource Related to the specification/resource directory spec:trace Related to the specification/trace directory
Projects
None yet
Development

No branches or pull requests