search

eclipse / microprofile-metrics

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

Update spec to include Process CPU Time #450

Closed Yushan-Lin closed 5 years ago

Yushan-Lin commented 5 years ago

Fixes #442

eclipse-microprofile-bot commented 5 years ago

Can one of the admins verify this patch?

eclipse-microprofile-bot commented 5 years ago

Can one of the admins verify this patch?

donbourne commented 5 years ago

Should we have the unit be nanoseconds or milliseconds? All of the other metrics with time units are in milliseconds so far. Is it useful to know the CPU time in more granularity than that?

@pilhuhn @jmartisk , WDYT?

donbourne commented 5 years ago

ok to test

jmartisk commented 5 years ago

OpenMetrics export will convert this to the base unit, which is seconds anyway :( and I'd say that's what metrics are mostly used with. For JSON export I don't really have a strong opinion, but I guess nanoseconds are fine. Quarkus currently exposes nanoseconds, so with them we will have less of a breaking change (we still will move it from vendor to base scope, that's the breaking change :) )

donbourne commented 5 years ago

@jmartisk given this new metric is optional, you might want to delay adding it to Quarkus until the appropriate time for you to make a backwards-incompatible change?

jmartisk commented 5 years ago

Yeah I'll see with the Quarkus guys. Before 1.0 release we don't really care about breaking changes, but this likely won't get into Quarkus before 1.0.

donbourne commented 5 years ago

@Yushan-Lin , I think we need to also update the base_metrics.xml file to include this new optional metric, so that the testOptionalBaseMetrics() test in MpMetricTest.java will validate that anyone using that metric is using the right type and units.