-
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
Instrument identity, registration conflicts, multi-callback registration, unregister support #2317
Instrument identity, registration conflicts, multi-callback registration, unregister support #2317
Conversation
…ication into jmacd/dup_instruments
…ication into jmacd/dup_instruments
…ication into jmacd/dup_instruments
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 this is great 👍 👍
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 with one nit. 🙂
I tried to answer this here: #2232 (comment) I changed the PR description to state this is part of #2232 either way, would prefer not to block on the answers to what I think are independent questions about supporting API-level deletion and/or a SDK-level memory preference. |
@bogdandrutu Do you think it's worth holding up this change for an answer to the question above? There are a number of approvals and the PR description has included "unregister support" from the beginning. It seems your question is whether adding unregister support is sufficient to address #2232, which we can debate, but do we still need to debate unregister support? The original request in #2232 is for a way to Close an entire Meter, unregister its instruments and stop reporting its metrics. in the context of a metrics bridge. I've tried to frame this as request for unregistering callbacks and for allowing the SDK to automatically forget stale metrics. @open-telemetry/specs-metrics-approvers Do we know of other reasons to support unregister()? I've tried to argue for this on the principle that without unregister support, users are forced to implement synchronization within callbacks on their own, which is not simple. (Unlike synchronous instruments, which are easy to stop using.) |
…ication into jmacd/dup_instruments
Yesterday I removed support for multi-instrument callbacks from this PR. |
Co-authored-by: Bogdan Drutu <lazy@splunk.com>
You still need to offer that. |
…ion, unregister support (open-telemetry#2317) * allow duplicate instrument reg * comment about single-writer rule and duplicate instruments * comment about duplicate asynchronous observations * refer to supp guidelines as well * first draft update * pre-merge * finish draft * two edits * SDK req * intrinsic properties * from 2270 * typos * add summary * Require a shorter test for probability sampler * Update specification/metrics/sdk.md Co-authored-by: Marc Pichler <marcpi@edu.aau.at> * Update specification/metrics/datamodel.md Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> * add unit to identity for data points * clarify data model uniqueness * clarify how to report semantic errors and when they can be remediated * lint * spelling * spelling * revert mistake unrelated * identical and distinct, respectively * refer to API/datamodel spec in SDK spec for conflict res * try to clarifyt async observation APIs * edits * Update specification/metrics/api.md Co-authored-by: Marc Pichler <marcpi@edu.aau.at> * example unregister in python * let multi-instrument be MAY * Update specification/metrics/api.md Co-authored-by: Aaron Abbott <aaronabbott@google.com> * Update specification/metrics/api.md Co-authored-by: Aaron Abbott <aaronabbott@google.com> * Update specification/metrics/sdk.md Co-authored-by: Aaron Abbott <aaronabbott@google.com> * remove multi-instrument callbacks for now * update examples * example text * remove use of 'context' * move an SDK req * lint * Fix lint * Update specification/metrics/api.md Co-authored-by: Bogdan Drutu <lazy@splunk.com> Co-authored-by: Marc Pichler <marcpi@edu.aau.at> Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com> Co-authored-by: Aaron Abbott <aaronabbott@google.com> Co-authored-by: Bogdan Drutu <bogdandrutu@gmail.com> Co-authored-by: Bogdan Drutu <lazy@splunk.com>
Fixes #2226
Part of #2232
Fixes #2250
Supercedes #2270
Related to, but does not supercede #2281
Part of #2280
Changes
See the discussion in #2226.
Defines "duplicate" and "identical" for Meter and Instrument concepts.
Permits duplicate instrument registration as a first-class feature, including the associated async callbacks
Clarifies that multi-instrument callback idioms are permitted.
Specifies support for unregistering async callbacks.
Specifies a warning for duplicate instrumentation registration conflicts (i.e., same name, not identical)
Specifies that SDKs are permitted to pass-through the semantic conflict.
Specifies that consumers are permitted to reject such data.