Skip to content
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

Mark Enabled API for synchronous instruments stable #4219

Closed
wants to merge 10 commits into from

Conversation

codeboten
Copy link
Contributor

Fixes #4215

Changes

This marks the API stable. The prototypes are discussed in the issue.

This marks the API stable.

Fixes open-telemetry#4215

Signed-off-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming the markdownlint failure will be fixed.

@@ -483,7 +483,7 @@ All instruments SHOULD provide functions to:

#### Enabled

**Status**: [Development](../document-status.md)
**Status**: [Stable](../document-status.md)
Copy link
Member

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 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.

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@pellared pellared Sep 24, 2024

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.

Copy link
Member

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)

Copy link
Member

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.

CHANGELOG.md Outdated Show resolved Hide resolved
pellared
pellared previously approved these changes Sep 24, 2024
@@ -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)
Copy link
Member

@jack-berg jack-berg Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Change an existing API by allowing optional parameters that have default values (Python).
  2. Add a new API which takes more parameters (Java).
  3. 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the approach taken in #4221 and have removed the paragraph about supporting additional parameters in the future. Please review

@carlosalberto
Copy link
Contributor

cc @jmacd

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #4256

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocked by #4256

Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 19, 2024
Copy link

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stabilize Metrics Instrument Enabled
10 participants