eclipse/microprofile-metrics

microprofile-metrics

Eclipse Microprofile Metrics

This specification aims at providing a unified way for Microprofile servers to export Monitoring data ("Telemetry") to management agents and also a unified Java API, that all (application) programmers can use to expose their telemetry data.

• For the current specification see the Metrics Spec document. For the specification at a given release or milestone, download the PDF from the GitHub releases page.

• To chat or ask questions about the spec, join the discussion on Gitter:

• For announcements, visit the Microprofile Google Group and filter by the metrics tag to focus on topics related to this specification.

Contributing

Do you want to contribute to this project? Find out how you can help here.

microprofile-metrics's Issues

Consider removing `type` from Annotations

Annotations currently have a type field

   /**
     * 
     * @return type of the metrics from Metadata, which is a timer for timed
     */
    MetricType type()

But the type is already explicitly given by the name of the annotation. We should perhaps remove the type - other wise users could go like @Gauge(type=COUNTER) int bla;`

Prometheus text format has specific requirements that we don't cover

re Section 2.3.1 of the spec

• prometheus text format - has expectations for how histograms and other objects are supposed to be rendered - need to detail how we are expecting implementers to follow this for each of our metric types
  https://prometheus.io/docs/instrumenting/exposition_formats/#text-format-details

Remove scale + Prometheus related things from MetricUnit

The MetricUnit class is copied from my sample implementation at https://github.com/pilhuhn/mp-metrics/blob/master/src/main/java/de/bsd/mp_metrics/MpMUnit.java and thus also contains logic to convert to the Prometheus base units.

We may consider removing this logic (and also the scaling factors) as they are implementations and could be done by more clever logic.

And then as Prometheus output format is one of the two default formats in MP-Metrics 1.0, we
can also leave this in and only remove the scaling/converting logic, so that each implementation can handle this as it wishes.
Or we only leave the 2 enums (metric + family) and remove all else and leave it to implementations.

See also #4

TCK/REST - ensure tags show up in output from prometheus

Use Astefanutti CDI module as programming interface

Should we add @InterceptorBinding to the metrics annotations?

Since we re-used the annotations from the DropWizard metrics-annotation, they do not have the @InterceptorBinding annotation. This works fine with Antonin's metrics-cdi project, but it is not very CDI'ish to programmatically add the interceptor binds.

TCK/REST - add test for appropriate behavior for options request for both application/json and anything else with OPTIONS verb

What about new or unknown units?

#44 also showed the problem of unit extensible with an enum only approach. Similar to the restriction of JSF with its ProjectStage enum. There is no way to extend the enum. Right now Metainfo only allows to have NO unit or one of the enums, not others, therefore limiting what can be monitored. As the way MetricUnit is used makes not much difference from a string wouldn't that be a more flexible alternative? E.g. using String constants would allow something other than just a known small set of units and none at all.

Remove Dropwizard's reference in Javadoc

Setup a proper maven project

Remove Implementation details of Timer and Histogram

table in section 4.3 of spec doesn't match the code

the table that shows which annotations apply to which things doesn't match the code. The code shows...

    Counted
        @Target({ ElementType.TYPE, ElementType.CONSTRUCTOR, ElementType.METHOD,                                            ElementType.ANNOTATION_TYPE })

    Gauge
        @Target({                                            ElementType.METHOD, ElementType.FIELD,                         ElementType.ANNOTATION_TYPE })

    Metered
        @Target({ ElementType.TYPE, ElementType.CONSTRUCTOR, ElementType.METHOD,                                            ElementType.ANNOTATION_TYPE })

    Timed
        @Target({ ElementType.TYPE, ElementType.CONSTRUCTOR, ElementType.METHOD,                                            ElementType.ANNOTATION_TYPE })

    Metric
        @Target({                                            ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER,  ElementType.ANNOTATION_TYPE })

TCK/API - add tests for use of tags

ensure same keys don't refer to different values
ensure it works with annotations (since you can only pass in a String)
validate that it has key=value format in the array of strings we get from metadata tag

Non-resolvable Parent POM

The current master version of the API does not compile.

[ERROR]     Non-resolvable parent POM for org.eclipse.microprofile.metrics:microprofile-metrics-api:1.0-SNAPSHOT: Could not find artifact org.eclipse.microprofile.metrics:microprofile-metrics-parent:pom:0.1-SNAPSHOT and 'parent.relativePath' points at wrong local POM @ line 20, column 13 -> [Help 2]

Unless all modules are commented out and the POM is built separately the build does not work.

Investigate running vendor-specific tests

Rest-Tests need to know in advance what data they can get from the API.
Vendor specific tests are nothing that all implementations have in common.

We could have some vendor-specific test-methods/classes that are disabled by default and which could be enabled on the command line
mvn test -Dvendor=liberty or mvn test -Dvendor=wf-swarm

MetricUnit for Metered should be seconds, not nanoseconds

need to update api / spec to change this from nanoseconds to seconds

Clarify metric types

I think the following should be gauges:

• ThreadCount
• DaemonThreadCount
• LoadedClassCount (= currentLoadedClassCount )

Clean up Metadata in spec

Metadata in 4.5.1 is still wrong wrt type and unit.

Also I guess we want to remove the @Json* annotations.

RAT plugin prevents builds

The Parent POM won't build according to #51 yet, because the RAT plugin fails due to missing excludes or similar. Simply commenting out the entire RAT plugin works, the Parent POM builds and so does the API module, but that should not be necessary.

Consider not setting default unit in Annotations

As written here: https://gitter.im/eclipse/microprofile-metrics?at=599d5220210ac26920b20f23

@raymondlam I would remove default units for @Gauge etc. Or at least default to NONE. (?)
Actually I feal we should remove them. If does not set them and compilation fails, then it should be quick to add them and thus sanetize what the user does.

TCK - provide tests for the API

Nathan and Fahham working on this

The interceptors don't get triggered for local methods in a servlet class

We noticed that the interceptors for methods within a servlet does not work (Annotations on fields seem to work correctly). Does anyone know what the limitations are when working with CDI + Servlets?

After looking at Antonin's CDI servlet tests, we got an example working by moving the timed method to a bean, @Inject it and calling the instance's method.

@WebServlet("/TestServlet")
public class TestServlet extends HttpServlet {
    private static final long serialVersionUID = 1L;

    @Inject
    MetricRegistry registry;  // This works

    @Inject
    @Metric(name = "test", absolute = true)
    Counter count;  // This works

    public TestServlet() {
        super();
    }

    // This does not work
    @Timed(name = "testTimer")
    @Counted(name = "count2", absolute = false)
    private void doSomething() {
        System.out.println("Timed");
    }

    protected void doGet(HttpServletRequest request, HttpServletResponse response)
            throws ServletException, IOException {
        count.inc();
        doSomething();
        response.getWriter().append("Served at: ").append(request.getContextPath());
    }

    protected void doPost(HttpServletRequest request, HttpServletResponse response)
            throws ServletException, IOException {
        doGet(request, response);
    }

}

Problems with PERCENT as a MetricUnit

Currently we show:

PERCENT("%", Family.RATE)

Using "Family.RATE" seems strange on this as we use the term rate elsewhere to describe how often something happens (ie. per unit of passing time). It seems like PERCENT should probably be unitless - eg. Family.NONE.

TCK/API - put metrics-CDI test bucket into the TCK

this relates to #84

how do we tell the world that they can use DWM 3.2.3 and metrics-cdi 1.4 as the bulk of their implementation?

QUESTION - should we allow standalone MetricRegistry types?

The metrics-CDI test bucket assumes MetricRegistry instances are not shared across tests.
Our current MetricRegistry factory only returns the app/vendor/base instances, which live for the duration of the server.
Adding the ability to have a standalone MetricRegistry would make it possible to run the metrics-CDI bucket of tests, and could be used by developers that have need for a MetricRegistry that isn't tied to one of our 3 pre-defined MetricRegistry instances.

Miscellaneous review comments

2.1.1

• in Required Base Metrics section we should say "Base metrics are exposed under /metrics/base."

2.1.2

• punctuation - "Tags have taken over the role to for example identify an application"

• need to clarify - "Global tags will be appended to the per-metric tags."

2.1.3

• incorrect (counters can go up and down in DWM - for example, to count how many threads are simultaneously executing the same method) - "counter: a monotonically increasing or decreasing numeric value"

• gauge - cite something like temperature of a CPU to help differentiate it from a counter

• meter - also mention that it counts the hits

• broken link - "See also [_supplying_of_tags]."

2.2

• I didn't review this as it has no content

2.3

• typo - "Json"

• typo - "http"

• typo - "404 if an directly"

2.3.1

• clarify - "Metrics will respond to GET requests and are exposed in a tree like fashion with sub-trees for the sub-resources mentioned in the previous section"

• change name to avoid any possible confusion - "count": 45

• need cleaner example - "The duration of foo after each request" (maybe "The average duration of foo requests during last 5 minutes")

2.3.2

• formatting problem - "Dot (.), Space ( ), Dash (-) are translated to underscore (_)."

• typo - "Scope is always starts the metric name"

3.4

• typo - "classloader:totalLoadedClass.count"

• typo - "classloader:totalUnloadedClass.count"

• typo - "on some platform where"

• SystemLoadAverage - should be listed as optional (The load average may be unavailable on some platform where it is expensive to implement this method)

4

• broken link - "see [_scopes] for the description of scopes"

• typo - "No unit is given, to MetricUnit.NONE is used"

• clarify - "It will be possible to use the non-annotations API, but using the annotations is preferred."

• typo - "watch the annotated objects and update its internal data structures"

4.3

• needs more description, and a period at the end - "Denotes a counter"

• needs more description - "Denotes a gauge."

• check - is this right?... "Denotes a meter which counts the invocations of the annotated object."

• table - need to explain what the annotation does if used at each of its possible scopes

• needs more context - say that this is used when @injecting a MetricRegistry - "Qualifies the type of Metric Registry to inject.". do we support injecting base/vendor MetricRegistries?

4.3.1

• needs clarification - "If not explicitly given it is determined from the annotated object and the fully qualified name of the surrounding class, unless absolute is true."

• needs to be more precise - "If true use the given name as an absolute name of the metric. If false, use the given name relative to the annotated class. Default value is false."

4.3.2

• missing classname -
  com.example.redCount
  com.example.blue

4.3.3

• typo - "for each of the constructor and methods " (also repeats later in spec)

4.3.4

• typo - "a method of field"

4.3.5

• needs more detail to differentiate it from a counter - "The meter counts the invocations of the constructor or method."

4.3.6

• clarify - "The metric of type Timer records the time it took to invoke the annotated object."

5.1

• wording - "There exists Jolokia as JMX-HTTP bridge. Using this for application specific metrics requires that those metrics are exposed to JMX first, which are many users not familiar with."

need a way to avoid metric name collisions for /metrics/application when multiple apps exist

Per discussion in MP metrics hangout call on Aug. 29:

In the spec, indicate that implementations that provide the ability to run more than one application can provide configuration for their server to enable an application GUID to be prepended to the application-scoped metric names

Clean up editor formats

Consider removing 'mbean' and 'multi' from Annotations

The annotations are meant for application programmers, which normally should not need to expose mbean attributes (as the application code does not first need to expose data to MBeans and then later declare a metric to read it from mbeans and expose it over mp-metrics).

We should perhaps remove those two fields that apply to mbeans from the annotations

remove JMX bridge

the spec, in section 3, calls for...

Values from the MBean server are encoded with MBean-Name/attribute[#field] name to retrieve a single attribute. E.g. GET /metrics/base/java.lang:type=Memory/ObjectPendingFinalizationCount to only get that count. For MBeans attributes that are of type CompositeData, the #field will return a single item of this composite data.

This was in the spec before we had a list of base metrics and seems out of place now. MP runs on servers that aren't even required to have JMX.

If we do intend to keep it, we need to provide more information in the spec. Today nothing is said about how that should look in JSON / prometheus formats, for example. Also, what happens if the mbean return value isn't a java native type (eg. it's a FooBean)?

recommendation: I don't think this was intended to be a permanent part of the spec. Suggest we remove it.

DECISION: take this out of the spec for v1.0. Not opposed to rediscussing this when Heiko is back to include in 1.1.

Should we support a hierarchy of metrics?

Currently (for 1.0), Metrics exposes a flat list of available metrics.

There is a possibility of supporting a hierarchy instead (as discussed in #26). Is this a good idea and for what use case might we want to implement it?

Mega and Giga are not m and g

At least mbyte or gbyte in the unit enum are not the right abbreviations and symbols for Mega and Giga. See https://en.wikipedia.org/wiki/Milli-, mbyte actually means millibyte.

Where did this come from, is it to satisfy a particular system like Prometheus?

Inconsistent enum constants in MpMUnit

The proposed MBUnits enum has constants like BIT as singular while others like DAYS or SECONDS are plural.
I understand, the inconsistent naming was introduced by Java Concurrency (TimeUnit) some while ago and e.g. JSR 310 later carried on the same inconsistencies (to be "consistent" with other parts of the JDK) but looking at ICU4J which is the de-facto standard for i18n in Eclipse projects, http://icu-project.org/apiref/icu4j/com/ibm/icu/util/MeasureUnit.html has a consistent way of naming all the constants like BIT or SECOND. So does the JSR 363 Unicode extension which is fully compatible with ICU4J and already used by monitoring solutions like Parfait. Metrics 2.0 also sticks to singular names of all units.

QUESTION - what should we do with MetricRegistryFactory?

all impls need a MetricRegistry producer that produces base/app/vendor MetricRegistry instances
should we put this in a new Appendix B?

Enhance Astefanutti Annotations to carry metadata

• units
• description at least.

If that is not provided, use units= none and emit a warning in the logs (on startup), that those should be provided.

clean up names of base metrics

currently we have:

    "base":{
        "availableProcessors":8,
        "committedHeapMemory":425197568,
        "currentLoadedClassCount":12369,
        "daemonThreadCount":15,
        "gc.PSMarkSweep.count":3,
        "gc.PSMarkSweep.time":264,
        "gc.PSScavenge.count":7,
        "gc.PSScavenge.time":80,
        "jvmUptime":18709269,
        "maxHeapMemory":3817865216,
        "peakThreadCount":82,
        "systemLoadAverage":3.20263671875,
        "threadCount":61,
        "totalLoadedClassCount":12397,
        "totalUnloadedClassCount":28,
        "usedHeapMemory":94789312
    }

suggested:
Naming…

        "base":{
            "cpu.availableProcessors":8,
            "cpu.systemLoadAverage":3.20263671875,
            "classloader.currentLoadedClass.count":12369,
            "classloader.totalLoaded.count":12397,
            "classloader.totalUnloaded.count":28,
            "gc.PSMarkSweep.count":3,
            "gc.PSMarkSweep.time":264,
            "gc.PSScavenge.count":7,
            "gc.PSScavenge.time":80,
            "jvm.uptime":18709269,
            "memory.committedHeap":425197568,
            "memory.maxHeap":3817865216,
            "memory.usedHeap":94789312
            "thread.daemon.count":15,
            "thread.max.count":82,                  // changed from peak to max for consistency
            "thread.count":61,
        }

spec needs to describe the use of accept header to pick between json and prometheus formats

The term MetricUnit is misleading

The class MetricUnit suggests it might have something to do with the Metric System, but that's not the case.

update spec to reflect proper use of unit suffixes for prometheus (as discussed in aug 31 meeting)

• counts should be dimensionless (no suffix)
• gauges should have units
• histogram could have units (used in each of the quantile metric names)
• meters
  • count is always dimensionless
  • rates are always per second
• timer
  • count is always dimensionless
  • histogram (always in seconds)
  • rate (always per second)

TCK/REST - RestAssured doesn't like port number supplied in baseURI

I'm working on a fix for this in our MetricsIT class.

bundlise the artifact

add all necessary dependencies to create a proper bundle with the correct version for packages

Spec - update spec to describe what should happen when you use OPTIONS with different accept headers

Rename DWM package to Microprofile one

Add PER_SECOND unit for @Metered Annotation

Since @Metered is a rate, do we need a PER_SECOND unit for the default unit?

Clarification: Alignment of security requirements between health and metrics

Both the health spec and metrics provide HTTP endpoints with mandatory security requirements. I.e in the health spec, we require HTTP Digest at a minimum, but not SSL. With metrics SSL seems to be required. Does it makes sense to align the minimum requirement between the two?

API - review javadoc

in particular, RegistryType needs some javadoc help

broken link to the spec

The url at bottom of https://github.com/eclipse/microprofile-metrics "For the specification see Metrics spec" points to...

• https://github.com/eclipse/microprofile-metrics/blob/master/spec/metrics_spec.adoc

which gives a 404. The spec is actually at...

• https://github.com/eclipse/microprofile-metrics/blob/master/spec/src/main/asciidoc/metrics_spec.adoc

API - Copyright in our api files

do we have the right copyright at top of our source files currently?
Raymond had taken best guess at this, but need to validate that this matches approach of config and other specs

Need to support tags for differentiating metric instances

Spec currently says "The implementation must flag duplicate metrics upon registration and reject the duplicate. A duplicate metric is a metric that has the same scope and name as an existing one."

we need to support tags as being considered part of the metrics id, in future rev, so that prometheus-style tagging is supported. It is common with Prometheus to reuse the same metric name for all instances of a metric, and differentiate the instances using the value of one or more tags.

Examples:

http_response_time_milliseconds_count{method="GET",handler="HomeController.handlerA",status="200",} 3.0

• http_requests_total{code="200",handler="prometheus",method="get"} 119

• Prometheus test code: https://github.com/prometheus/common/blob/master/expfmt/testdata/text

• Prometheus Golang collector: https://github.com/prometheus/client_golang/blob/master/prometheus/process_collector.go