eclipse / microprofile-metrics

microprofile-metrics
Apache License 2.0
100 stars 66 forks source link

MetricRegistry Key based Lookup #548

Closed jbee closed 4 years ago

jbee commented 4 years ago

In response to #543 this PR adds methods to MetricRegistry that allow to favour key based lookup over accessing a map first and resolving the target instance from the returned map.

Having to use a map is considered an anti-pattern as in many cases the returned maps are either views or data structures computed on demand. This either taxes implementation complexity (views) or runtime resources (computed maps) unnecessarily and it is somewhat awkward to use as well.

This PR adds to MetricRegistry:

Notably a Gauge<?> gauge(MetricID metricID, Gauge<?> gauge); method was added that allows to lookup or create a Gauge as an atomic (thread-safe) operation of the registry. No variant with Metadata was added for now.

In addition the existing methods getMetrics() and getMetadata() were marked deprecated.

Tests have been adopted to use the new methods where preferable. This also confirmed that getMetrics() and getMetadata() should not exist. There are no longer used by the TCK.

The same is largely true for methods like getCounters() that were mostly used to do getCounter(MethodID) in form of getCounters().get(MetricID) and so forth.

MetricRegistry has been changed to an interface and default methods have been used to show how the API can benefit from the use without providing implementations for methods that should be implemented based on the internal data organisation. Mapping String name or String name and Tag[] to MetricID can only be done correct in one way. There is no value in duplicating this logic. From an implementation standpoint the same is true for all methods accepting a MetricFilter. As the implementation cannot know the logic of the provided filter there is little room for an implementation to optimise this wherefore it is unlikely that an implementation can do better than the default methods provided. As for the various ways of providing a MetricID the family of methods accepting a MetricFilter is more a convenience feature of the API for the user than an opportunity for an implementation to optimise.

eclipse-microprofile-bot commented 4 years ago

Can one of the admins verify this patch?

jmartisk commented 4 years ago

ok to test

jbee commented 4 years ago

Warnings should be corrected, waiting with further changes for more feedback and a decided path.

jmartisk commented 4 years ago

So @donbourne WDYT? Should we turn MetricRegistry into an interface and introduce a breaking change by that? I'm leaning more and more toward doing a breaking change release. This could also align with the goals of https://github.com/eclipse/microprofile-metrics/issues/506 because we could potentially also remove the helper methods name and append, and if an implementation needs it then it will have to provide them itself.

And to fix the main part of #506, which is instantiating MetricID in its constructor, that would be a breaking change as well. Fixing https://github.com/eclipse/microprofile-metrics/issues/524 will be a breaking change too. I think it's becoming quite clear that the next release will be 3.0

donbourne commented 4 years ago

sorry for lack of communication on this -- I am quite interested, but likely won't get time to page this in until tomorrow.

jmartisk commented 4 years ago

Could you also update the changelog (spec/src/main/asciidoc/changelog.adoc) with a brief description of the changes? The section is now "Changes in 2.4", I think you can rename that to "Changes in 3.0" I think this will mostly go into API/SPI Changes part

update: I sent a PR which does the 2.4->3.0 move here https://github.com/eclipse/microprofile-metrics/pull/554 so you don't have to change the section title

jbee commented 4 years ago

@jmartisk Did changes as requested. If there is something that was discussed earlier that should be changed in one or the other way (e.g. the default methods or deprecation) let me know how this should be changed and I make the update.

jmartisk commented 4 years ago

@jbee Thanks! I think the current consensus is that we can keep the deprecations and default methods

jmartisk commented 4 years ago

I took the liberty to rebase, fix a few conflicts and apply some minor fixes. TCK is passing for me now

jmartisk commented 4 years ago

I'm basically fine with merging now, just want @donbourne / @Channyboy 's blessing

Channyboy commented 4 years ago

Tested and its good. Will need to rebase.

jmartisk commented 4 years ago

Yeah seems we're doing too many things in parallel, leading to conflicts ;) We should try to merge things quickly so they don't stay around for too long. @jbee would you mind to have a look and rebase please? I think that it's ready to merge except for that

jbee commented 4 years ago

@jmartisk First missed conflicts in the javadoc of MetricRegistry but I hope I now got everything rebased correctly.

jmartisk commented 4 years ago

@jbee , The build is now failing because enforcer plugin complains about unused imports: https://ci.eclipse.org/microprofile/job/Metrics%20pull-requests/257/org.eclipse.microprofile.metrics$microprofile-metrics-api-tck/console

jbee commented 4 years ago

Done. Still unfamiliar with the things to run :D - Should be fine now.

jmartisk commented 4 years ago

I'll merge it now, hoping it won't cause any issues that we might have missed :) Thanks @jbee !!