-
Notifications
You must be signed in to change notification settings - Fork 889
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
Comments
About "we started to mark fields as 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. |
@Oberon00 Please try to rephrase your definition from the user perspective. What is the problem if I don't set a |
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]( 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. |
I think this is already the current expectation. My two cents: I think I'd also lean towards option 3) |
I don't see how "3) They are more important than non-required, but everything will still work" should work in practice. |
Repeating my definition for further explanation:
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:
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. |
I think this is indeed the current interpretation, and 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. |
@yurishkuro great minds think alike :) #604 (comment) |
That was exactly my point, we need to define this from the user perspective. |
@bogdandrutu sorry, I should've attributed the "recommended" term, comment updated. |
@bogdandrutu Please define which user you mean. The one who looks at the Web UI of the backend, or the "user" that codes instrumentations? |
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. |
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. |
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. |
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 |
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. |
If that is the case we should reconsider what are the required semantics, because right now there are a lot of required fields |
@bogdandrutu talked about this at the maintainers mtg today, labelling as |
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. |
from the issue triage meeting today with TC, allowed for GA for editorial change only |
Should we consider it done with #2522 ? |
Since nobody objected, I guess this issue can be closed? |
Done in #2522. |
We started to mark fields as
required
in the semantic conventions but we have not defined very well what does it mean:Span
data are not usable?Span
will not be identified correctly?There are multiple ways to define
required
and we need to define it before we addrequired
to a lot of the semantic conventions that we add.The text was updated successfully, but these errors were encountered: