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

Decouple internal Metrics SPI from MP Metrics API #8272

Closed
kenfinnigan opened this issue Mar 30, 2020 · 4 comments · Fixed by #10626
Closed

Decouple internal Metrics SPI from MP Metrics API #8272

kenfinnigan opened this issue Mar 30, 2020 · 4 comments · Fixed by #10626
Labels
kind/enhancement New feature or request triage/needs-decision This issue proposes a design change. Needs to be discussed in the ML
Milestone

Comments

@kenfinnigan
Copy link
Member

Description
Currently, the SPI for Extensions to register metrics (https://github.com/quarkusio/quarkus/blob/master/extensions/smallrye-metrics/spi) bleeds the MicroProfile Metrics API into the Extension. We should shield our Extensions from changes to the API that don't impact how metrics are registered for Quarkus.

See https://quarkus.io/guides/writing-extensions#extension-metrics for what Extensions need to implement.

Implementation ideas
One option is to create our own SPI for registering metrics from Extensions, ideally one that can take a Supplier, or the like, that is the function of the metric.

@kenfinnigan kenfinnigan added kind/enhancement New feature or request triage/needs-decision This issue proposes a design change. Needs to be discussed in the ML labels Mar 30, 2020
@jmartisk
Copy link
Contributor

jmartisk commented Apr 3, 2020

Sorry I didn't notice this right away, feel free to @mention me next time so I get a notification.
So to list all things that we use in the SPI and would need changes:

  • The Metadata interface and its builder

    • We could introduce some sort of alternative class representing Metadata. The bad thing is that extensions will not be able to use the fluent MetadataBuilder from the API. So to maintain usability, our abstraction would need to be very similar (offer a similar builder), I guess.
  • The MetricRegistry.Type enum

    • For that we could have our enum that contains the same values?
  • Extensions sometimes provide their own implementation classes of metrics

    • That's a bit more tricky because each metric type has a different set of methods. For that, I think we'd need to provide a set of our own metric interfaces and be able to bridge their implementations into the standard interfaces.

All in all it sounds mostly like copying stuff from the Metrics API and uglifying the code quite a lot. And given how our "alternative" classes will most likely be very similar to the original ones, I'm not sure how well it will really be able to shield us from changes in the API.

For example, one related change that is IMO relatively possible to happen, is that we drop the notion of having multiple metric registries. In that case, we could leave the extensions specify what registry to use for their metrics, but then we would ignore that setting and put everything into one registry; but I don't think this solution would be any easier or nicer than simply updating the SPI to stop accepting that parameter, and updating the extensions' code accordingly.

So, if it's really needed, I don't know, but if we decide that it is, I can do that.

@ebullient
Copy link
Member

This grew somewhat out of my attempt to see if we could get micrometer support to work natively with Quarkus. The extension was easy enough to write. If, however, we wanted to allow that extension to be an alternate provider for metrics, we run into a lot of trouble.

The SPI surfaced to other extensions is very MP-centric: base/api/vendor concepts, for example. I was trying to disentangle things to allow that, but there was too much detail exposed about inner registry workings to make that completely feasible. The types, e.g. would be easy enough to map, but the recommended practice for a counter specifies defining an implementation, rather than (for example) specifying a function that could be attached to a counter.

@jmartisk
Copy link
Contributor

jmartisk commented Apr 6, 2020

But what are we going to do about extensions which don't use this SPI at all, because their underlying libraries use MP Metrics API directly? It does not seem very realistic that we could refactor all such libraries (at the moment I can think of Infinispan and most SmallRye projects, but there will probably be more in the future) to use an abstracted API.

Jaeger is another potentially problematic case, it has its very own custom metric API and we created a bridge between our that API and our MP Metrics-based "presentation" layer.
Vert.x, on the other hand, has the built-in option to produce Micrometer-compatible metrics, and we're yet to design a reasonable bridging mechanism from that to our MP Metrics presentation layer.

Trying to run all metrics through an abstract API that allows bridging to both Micrometer and MP Metrics output would be super cool, but I'm quite skeptical about it being possible.

@ebullient
Copy link
Member

I think we should look more closely at the binder pattern that both micrometer and opentelemetry use. There is a binder for jaeger and micrometer, and a binder for jaeger to opentelemetry metrics. From a compatbility perspective, I'd be willing to make an MP Metrics to micrometer binder.. but I think there are some API hiccups there because implementation is tied in.

My first truly vile attempt was going to be class manipulation to do something like that anyway..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request triage/needs-decision This issue proposes a design change. Needs to be discussed in the ML
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants