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

Allow registering multiple callbacks in async metrics instruments #4051

Closed
mateuszrzeszutek opened this issue Jan 5, 2022 · 5 comments
Closed
Labels
Feature Request Suggest an idea for this project

Comments

@mateuszrzeszutek
Copy link
Member

mateuszrzeszutek commented Jan 5, 2022

Is your feature request related to a problem? Please describe.
I think we should allow registering more than one callback for async instruments (like a gauge). The current state pretty much forces the user (or the instrumentation author) to implement composite mutable callbacks and manage their state by hand if there's a need to have an async instrument that records measurements originating from various places. For example, see the AsyncInstrumentRegistry from my micrometer bridge instrumentation - micrometer meters treat attributes/tags as part of their identifier, so it's possible to have several instruments with the same name, but different tags.
Another example: the Splunk distro currently contains several database connection pool metric instrumentations, that use same instrument names and different attributes/tags to identify different connection pool instance metrics. We're using micrometer right now so that works, but we'll want to upstream these instrumentations soon -- and the single callback restriction will be rather problematic if you want to use same metric names for different connection pools/connection pool implementations.

Describe the solution you'd like
It should be possible to register multiple callbacks in async metrics instruments. The callbacks should be cancellable/removable independently of each other too -- imagine you have multiple connection pools in your app, each one using a separate callback: you probably don't want to remove the whole instrument if only one of these pools closes.

Describe alternatives you've considered
Implementing an AsyncInstrumentRegistry in the instrumentation-api? That's an option I guess, although I believe the SDK should just support this.

Additional context
Current "log a warning" approach: #4005
The latest AsyncInstrumentRegistry version: https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/5017/files#diff-cfef47d826dc6102c0d5c11299ea412d8e66e5f90d70ccc877770733df6bc5c0

@mateuszrzeszutek mateuszrzeszutek added the Feature Request Suggest an idea for this project label Jan 5, 2022
@jack-berg
Copy link
Member

This feels like it needs to be resolved at the spec level. I can't find specification on what to do when multiple async callbacks are registered for the same instrument. The closest is the following blurb:

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

For synchronous instruments, we compare the instrument type, instrument value type, name, unit and description. If there are any differences, we log an error. If all are the same, we return the same instrument. We can do the same comparison checks on async instruments, but its not clear whether their callbacks should be composed if all the checks pass.

@anuraaga
Copy link
Contributor

anuraaga commented Jan 6, 2022

and the single callback restriction will be rather problematic if you want to use same metric names for different connection pools/connection pool implementations.

Can you inject the instrument into the pools instead of creating them together? I guess if there were a connection pool instrumenter, it would hold needed instruments and pass to logic and that could be idiomatic.

@jack-berg
Copy link
Member

Opened spec Issue #2249 to try to get clarification on this.

@mateuszrzeszutek
Copy link
Member Author

Can you inject the instrument into the pools instead of creating them together? I guess if there were a connection pool instrumenter, it would hold needed instruments and pass to logic and that could be idiomatic.

Hmm, I don't really get that - could you elaborate?

Pretty much all JDBC connection pools that we instrument expose getter methods for metrics/statistics, e.g. like getIdleConnectionsCount() - currently we're creating a new micrometer gauge for each of those methods, each pool separately.

@jack-berg
Copy link
Member

Resolved in #4143.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Suggest an idea for this project
Projects
None yet
Development

No branches or pull requests

3 participants