Mark Enabled API for synchronous instruments stable #4219
Conversation
This marks the API stable. Fixes open-telemetry#4215 Signed-off-by: Alex Boten <[email protected]>
| @@ -483,7 +483,7 @@ All instruments SHOULD provide functions to: | |||
|
|
|||
| #### Enabled | |||
|
|
|||
| **Status**: [Development](../document-status.md) | |||
| **Status**: [Stable](../document-status.md) | |||
There was a problem hiding this comment.
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 Enabled should 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:
// Returns true or false indicating whether the instrument is enabled. If the final application has not enabled
// OpenTelemetry SDK, then this would be false. This can also be false when the
// opentelemetry sdk is configured (via Views or other means) to Drop
// measurements from this instrument.
There was a problem hiding this comment.
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.
Is there any reason to not stabilize the corresponding SDK capabilities?
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.
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.
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.
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.
| @@ -483,7 +483,7 @@ All instruments SHOULD provide functions to: | |||
|
|
|||
| #### Enabled | |||
|
|
|||
| **Status**: [Development](../document-status.md) | |||
| **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:
There are currently no required parameters for this API. Parameters can be
added in the future, therefore, the API MUST be structured in a way for
parameters to be added.
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.
A few lines below this is:
There are currently no required parameters for this API. Parameters can be
added in the future, therefore, the API MUST be structured in a way for
parameters to be added.Is this something we're comfortable stabilizing with?
Yes to me.
Depending on the actual programming language, this could be done in many different ways. Here are some examples:
- Change an existing API by allowing optional parameters that have default values (Python).
- Add a new API which takes more parameters (Java).
- Add a new type which inherits from the existing type and extends the functionality (e.g. https://learn.microsoft.com/dotnet/framework/unmanaged-api/profiling/icorprofilerinfo4-interface for C/C++).
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.
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?
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.
Have any implementations/prototypes shows the need for additional parameters?
The implementations/prototypes barely exist, hence my comment here:
- Java has a prototype, which I built because I proposed the feature
- .NET has a prototype which predated this feature
- Go's prototype is a PR
There's very little data / feedback to go off of.
Yes to me.
@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
|
cc @jmacd |
There was a problem hiding this comment.
I believe we don't currently meet the prototyping requirement.
Requesting changes to ensure this doesn't accidentally get merged. If indeed there are prototypes in languages besides java and .NET, I'll remove the block.
| There are currently no required parameters for this API. Parameters can be | ||
| added in the future, therefore, the API MUST be structured in a way for | ||
| parameters to be added. | ||
| There are currently no required parameters for this API. |
There was a problem hiding this comment.
I think we should add Context as a parameter similarly to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/bridge-api.md#enabled.
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Fixes #4215
Changes
This marks the API stable. The prototypes are discussed in the issue.
CHANGELOG.mdfile updated for non-trivial changesspec-compliance-matrix.mdupdated if necessary