-
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
Clarify/relax required resource attributes. #1294
Clarify/relax required resource attributes. #1294
Conversation
specification/resource/sdk.md
Outdated
Certain **required** `Resource` attributes MUST be set to a default value if they were not specified when | ||
the Resource is associate with a `TracerProvider` or `MeterProvider` | ||
(this will result in a new `Resource` being associated, as the original `Resource` is immutable). |
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.
The implication is that each provider would be responsible for setting those required attributes to default values. Should there be some framework for enriching resources that providers can share, to avoid duplication?
Also, do we document default fallback values for all required attributes? Service name alone too a number of iterations.
Something was weird here with the auto-requested reviews (there seem to have been none?) so I manually requested reviews now. |
This is a good clarification. I think it should also be highlighted in the referenced document [ |
Since I have no approvals yet anyway, I reworked this again to not always force the attributes into the Resource. If this is too lenient, I can go back to requiring setting the attributes at time of association with TracerProvider/MeterProvider (see state at f65800a). |
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.
thanks! much cleaner/clearer than what was previously specified.
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.
Thanks!
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.
Thanks! Please don't forget about the changelog 🙂
Co-authored-by: Armin Ruech <[email protected]>
Done. BTW the new/updated distinction seems to be lost currently. |
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'm not sure this is the right place for this question but why this property is called service.name
? Should not this be a little more general, e.g.: application.name
(e.g.: in case of a thick client)?
Not a strong feeling, but I'd vow for 3a as it's the simplest one (and should work just fine). |
@jonatan-ivanov In order to not lose focus here, would you mind opening an issue with this question? Or even better, come to the Spec SIG meeting on Tuesday, 8AM Pacific ;) |
@open-telemetry/specs-approvers Please review the latest iteration. Once we get 1-2 more reviews we are good to go (usually we need less reviews, but this being a tricky one, I'd rather have more eyes on it). Else, we will merge in the next days ;) |
The SDK MUST provide access to a Resource with at least the attributes listed at | ||
[Semantic Attributes with SDK-provided Default Value](semantic_conventions/README.md#semantic-attributes-with-sdk-provided-default-value). | ||
This resource MUST be associated with a `TracerProvider` or `MeterProvider` | ||
if another resource was not explicitly specified. |
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 not clear to me.
The SDK MUST provide access to a Resource
provide access to whom? because later it says
This resource MUST be associated with a
TracerProvider
orMeterProvider
associated by whom?
Are we saying that SDK must expose an API like getDefaultResource()
that the user MUST call and then pass to provider? What if another resource is passed, not the default one, and thus the required attributes are still not available?
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.
The SDK MUST provide access to a Resource
provide access to whom? because later it says
As a public API.
This resource MUST be associated with a TracerProvider or MeterProvider
associated by whom?
By the Tracer/MeterProvider implemenentation. For that, a public API would not be necessary, but it does not hurt that case and is very useful if exporters want to access it, e.g. to get a fallback service.name
.
Are we saying that SDK must expose an API like getDefaultResource()
Yes
that the user MUST call and then pass to provider?
No, we do not place MUST requirements on the user here. See my above answer in this comment.
What if another resource is passed, not the default one, and thus the required attributes are still not available?
Then whoever needs these attributes needs to fall back to the default resource. This could be done e.g. by explicitlyPassedResource.Merge(defaultResource).get(theAttributeNeeded)
.
I don't think this addresses the underlying cause of always having service name, given current wording
As discussed in the spec meeting: If we change the order of arguments of Merge (i.e. last value wins like everywhere else in OTel), this PR would offer better UX. |
|
@open-telemetry/specs-approvers I will merge this by EOD, FYI ;) (unless there's some late feedback against, etc) |
@Oberon00 Please rebase so we can finally merge this. |
Fixes #1346.
Depends on #1345 ✔️ .
Changes
Clarify that it is enough to add required resource attributes
when the resource is associated with somethingto the default resource. We do not need to somehow patch the attribute on every (temporary) resource object created by the user.Related Java issues (see linked PRs there): open-telemetry/opentelemetry-java#2298
EDIT on 2020-12-19: I forgot to update this PR description to match the changes from 1698fd0 (2020-12-17). Done now.