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

Update spec to include Process CPU Time #450

Merged
merged 3 commits into from Sep 27, 2019

Conversation

@Yushan-Lin
Copy link
Contributor

Yushan-Lin commented Sep 25, 2019

Fixes #442

Signed-off-by: Yushan Lin <yushan.lin@ibm.com>
@genie-microprofile

This comment has been minimized.

Copy link
Contributor

genie-microprofile commented Sep 25, 2019

Can one of the admins verify this patch?

1 similar comment
@genie-microprofile

This comment has been minimized.

Copy link
Contributor

genie-microprofile commented Sep 25, 2019

Can one of the admins verify this patch?

@Yushan-Lin Yushan-Lin force-pushed the Yushan-Lin:442-ProcessCPUTime branch from 38d8f34 to 2c9d69d Sep 25, 2019
@donbourne

This comment has been minimized.

Copy link
Member

donbourne commented Sep 25, 2019

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

This comment has been minimized.

Copy link
Member

donbourne commented Sep 25, 2019

ok to test

@jmartisk

This comment has been minimized.

Copy link
Contributor

jmartisk commented Sep 26, 2019

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 :) )

Copy link
Contributor

jmartisk left a comment

Please update also the changelog with this addition

@donbourne

This comment has been minimized.

Copy link
Member

donbourne commented Sep 26, 2019

@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?

Yushan-Lin added 2 commits Sep 26, 2019
Signed-off-by: Yushan Lin <yushan.lin@ibm.com>
Signed-off-by: Yushan Lin <yushan.lin@ibm.com>
@Yushan-Lin Yushan-Lin requested a review from jmartisk Sep 26, 2019
Copy link
Member

donbourne left a comment

LGTM

@jmartisk

This comment has been minimized.

Copy link
Contributor

jmartisk commented Sep 27, 2019

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.

@jmartisk jmartisk merged commit 53ad297 into eclipse:master Sep 27, 2019
2 checks passed
2 checks passed
default Build finished.
Details
eclipsefdn/eca The author(s) of the pull request is covered by necessary legal agreements in order to proceed!
Details
@donbourne

This comment has been minimized.

Copy link
Member

donbourne commented Sep 28, 2019

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.