Mark Enabled API for synchronous instruments stable #4219
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -477,15 +477,13 @@ All instruments SHOULD provide functions to: | |
|
|
||
| #### Enabled | ||
|
|
||
| **Status**: [Stable](../document-status.md) | ||
|
|
||
| To help users avoid performing computationally expensive operations when | ||
| recording measurements, [synchronous Instruments](#synchronous-instrument-api) | ||
|
There was a problem hiding this comment. A few lines below this is:
Is this something we're comfortable stabilizing with? In today's spec SIG, I heard that structuring APIs in go to allow for additional future parameters has substantial performance impact. Obviously with the metrics enabled operation, performance is paramount. Does this sentence encourage API designs which are not performant? There was a problem hiding this comment. Have any implementations/prototypes shows the need for additional parameters? Is the best way to move this effort forward to implement prototypes in additional languages to validate there isn't a need for additional parameters? There was a problem hiding this comment.
Yes to me. Depending on the actual programming language, this could be done in many different ways. Here are some examples:
There are ways to avoid perf penalty in the programming languages that I've used. It is hard to tell if all programming languages share the design philosophy/principle "you only pay for what you need", there could be outliners that encourage bad performance. I don't think we should be too worried about such cases, because if folks care about perf, then it is a problem that the language/runtime must solve; if folks don't care about perf, then they won't complain about perf.
I won't interpret it in such way. However, I know that we do have language implementation SIGs which developed low performing APIs that later got fixed, so I think it'll be helpful to explicitly call out some performance recommendations. Doesn't seem to be a blocker though. There was a problem hiding this comment.
The implementations/prototypes barely exist, hence my comment here:
There's very little data / feedback to go off of.
@reyang deleting this paragraph wouldn't prohibit us from adding additional parameters in the future - that's always allowed. Its the difference between allowing and encouraging. See #4221 and this comment for a similar conversation for the logger isEnabled method. Maybe we should follow suit with the metrics isEnabled language? And to reiterate another comment I made in the logger isEnabled discussion, while its allowable to add additional parameters, doing so isn't free - the API ergonomics are worse with overloads as it becomes less clear which overload should be called, and some languages have to jump through a lot of hoops to add additional parameters. We ought to try hard to get the parameters right up front. With that said, I think the current spec (i.e. no parameters) is likely to be correct in the long term. There was a problem hiding this comment. I followed the approach taken in #4221 and have removed the paragraph about supporting additional parameters in the future. Please review |
||
| SHOULD provide this `Enabled` API. | ||
|
|
||
| There are currently no required parameters for this API. | ||
|
There was a problem hiding this comment. I think we should add |
||
|
|
||
| This API MUST return a language idiomatic boolean type. A returned value of | ||
| `true` means the instrument is enabled for the provided arguments, and a returned | ||
|
|
||
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 corresponding SDK part of the spec is not marked stable with this. I guess that this is intentional?
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#instrument-enabled
(Logs are also facing similar question - should the API part gets stabilized before the SDK part? I guess it can be, given Metric API spec was stabilized before SDK spec.)
If we are going with stabilizing the API part alone, would it makes sense to clarify that the
Enabledshould always return false when API alone is used, as without an SDK, all instruments are no-op anyway?An end user documentation would be something to the tune of:
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.
Stabilizing the API without the corresponding SDK exposes us to a future possibility where there is a stable API and the corresponding SDK capability gets deleted. We should try to avoid this.
Is there any reason to not stabilize the corresponding SDK capabilities?
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 could mark the SDK stable as well, I didn't know if we would want to mark it stable if it's dependent on MeterConfig. It wasn't clear to me how much work was needed to stabilize MeterConfig
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.
Not stabilizing the MeterConfig portion gives more time to explore other potential ways that the SDK can implement the API. It gives us more flexibility if the users would find some issues with MeterConfig design that does not affect the API.
I find it safer to mark only API as stable and them mark SDK as stable e.g. after a few months.
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.
per the discussion in Spec meeting, only Java has the sdk side implemented.
.NET has implemented SDK side partially (it only returns false based on View's return Drop. MeterConfig part is not implemented)
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 won't block on my comment about API / SDK stabilizing together, but I don't plan on approving this PR as the feature doesn't seem to be widely implemented.