-
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
Remove the concept of supported temporality, keep preferred #2154
Conversation
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.
LGTM. Need to update the changelog.
0894181
to
ee9a8f6
Compare
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
ee9a8f6
to
7b272de
Compare
preferred temporality is explicitly specified then the SDK SHOULD respect that, | ||
otherwise use Cumulative. |
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.
We should strengthen the normative requirements here to ensure compatibility and keep error behavior explicitly to avoid ambiguity.
preferred temporality is explicitly specified then the SDK SHOULD respect that, | |
otherwise use Cumulative. | |
preferred temporality is explicitly specified then the SDK MUST provide data using that temporality if it can. | |
If the SDK is not capable of providing data with the explicitly specified temporality it MUST be treated as an error. | |
Otherwise, if no preferred temporality is specified, the SDK SHOULD provide data with Cumulative temporality. |
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.
MUST provide data using that temporality if it can.
MUST and "if it can" are kind of opposite, is it a MUST or not.
The reason not to use must is because 20 lines below we have:
OpenTelemetry SDK
authors MAY choose the best idiomatic design for their language:
- Whether to treat the temporality settings as recommendation or enforcement.
For example, if the temporality is set to Delta, would the SDK want to perform
Cumulative->Delta conversion for an Asynchronous
Counter, or downgrade it to a
Gauge, or keep consuming it as Cumulative due to the
consideration of memory
efficiency?
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.
MUST provide data using that temporality if it can.
MUST and "if it can" are kind of opposite, is it a MUST or not.
The reason not to use must is because 20 lines below we have:
OpenTelemetry SDK
authors MAY choose the best idiomatic design for their language:
- Whether to treat the temporality settings as recommendation or enforcement.
For example, if the temporality is set to Delta, would the SDK want to perform
Cumulative->Delta conversion for an Asynchronous
Counter, or downgrade it to a
Gauge, or keep consuming it as Cumulative due to the
consideration of memory
efficiency?
Gotcha 👍 . Thoughts on being explicit with the errors response?
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 most (all?) implementations currently do the cumulative -> delta conversion, right? This would not be an error state. So since everything is allowed, do you mean that there should be a standard way of letting users know what is happening?
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.
golang does not, and probably other languages may not do as well (not sure).
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.
Thoughts on being explicit with the errors response?
Wait, how was this resolved?
…emetry#2154) Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
Signed-off-by: Bogdan Drutu bogdandrutu@gmail.com
Fixes #2130