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

Clarify whether multiple callbacks are allowed for async instruments #2249

Closed
jack-berg opened this issue Jan 6, 2022 · 3 comments
Closed
Assignees
Labels
area:api Cross language API specification issue enhancement New feature or request needs discussion Need more information before all suitable labels can be applied spec:metrics Related to the specification/metrics directory

Comments

@jack-berg
Copy link
Member

jack-berg commented Jan 6, 2022

What are you trying to achieve?

Clarify behavior of metrics API / SDK when multiple callbacks are registered to the same instrument. The api spec says:

Meter implementations MUST return an error when multiple Instruments are registered under the same Meter instance using the same name.

In java, we interpret that as meaning that you can't register conflicting instruments under the same name. However, if the instrument type, name, unit, and description match, we'll return the previously registered instrument.

For example, in the following snippet, no error is generated and counter1 and counter2 are the instrument.

    LongCounter counter1 = meter.counterBuilder("foo").build();
    LongCounter counter2 = meter.counterBuilder("foo").build();

This makes sense for synchronous instruments, but its not clear what to do in the async case. Currently, if multiple callbacks are registered for the same instrument, we silently ignore registrations after the first. This isn't great, and minimally we should log an error when subsequent callbacks are registered. However, there's also an argument to made that registering multiple callbacks should be allowed and should result in each being invoked.

This has been raised in Issue #4051 and we believe it needs raising in the spec for resolution.

In particular, we want to make sure that our plans to publish a stable version of the metrics API would not be impacted by the spec resolution of this issue.

Additional context.

Issue #4005 aims to log a warning when multiple callbacks are registered, and is implemented in Issue #4020.

@jack-berg jack-berg added the spec:metrics Related to the specification/metrics directory label Jan 6, 2022
@arminru arminru assigned reyang and unassigned arminru Jan 10, 2022
@reyang reyang added enhancement New feature or request area:api Cross language API specification issue labels Jan 10, 2022
@reyang reyang added this to the Metrics Future Release milestone Jan 10, 2022
@reyang
Copy link
Member

reyang commented Jan 11, 2022

Meter implementations MUST return an error when multiple Instruments are registered under the same Meter instance using the same name.

It seems this statement in the spec is clear, do we expect to "have more clarification" or "change the behavior"?

In java, we interpret that as meaning that you can't register conflicting instruments under the same name. However, if the instrument type, name, unit, and description match, we'll return the previously registered instrument.

I can see problems, the API spec allows optional parameters to be added in the future, which might break Java's implementation.

@jsuereth
Copy link
Contributor

Meter implementations MUST return an error when multiple Instruments are registered under the same Meter instance using the same name.

It seems this statement in the spec is clear, do we expect to "have more clarification" or "change the behavior"?

I actually was the one who misinterpreted this (when first approving the PR and then when implementing). I think this is far too restrictive as written and really diverges from getTracer and getLogger conventions. I suspect this limitation will dramatically hurt instrumentation in open-world systems. I'd argue we should consider this a major bug/user friction issue we should address.

In java, we interpret that as meaning that you can't register conflicting instruments under the same name. However, if the instrument type, name, unit, and description match, we'll return the previously registered instrument.

I can see problems, the API spec allows optional parameters to be added in the future, which might break Java's implementation.

We need to define instrument identity. If we add new parameters that alter instrument identity, we're likely causing user-friction/breakages. I'd like to understand what sorts of parameters we'd add (that alter metric identity) that are non-breaking for other implementations.

@jack-berg
Copy link
Member Author

Resolved in #2317.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue enhancement New feature or request needs discussion Need more information before all suitable labels can be applied spec:metrics Related to the specification/metrics directory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants