-
Notifications
You must be signed in to change notification settings - Fork 66
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 Histogram/Timer's Snapshot #712
Conversation
public double get999thPercentile() { | ||
return getValue(0.999); | ||
} | ||
public abstract long size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use long
instead of int
to match up with micrometer.
|
||
/** | ||
* Returns the highest value in the snapshot. | ||
* | ||
* @return the highest value | ||
*/ | ||
public abstract long getMax(); | ||
public abstract double getMax(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use double
to match up with micrometer
@@ -140,34 +141,72 @@ public void testSnapshotValues() throws Exception { | |||
SAMPLE_LONG_DATA, histogramLong.getSnapshot().getValues()); | |||
} | |||
|
|||
@Test | |||
public void testSnapshotPercentileValuesPresent() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we really need to test this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems reasonable for this release, given those are the expected percentiles
@@ -154,56 +155,63 @@ public void testSnapshotValues() throws Exception { | |||
SAMPLE_LONG_DATA, globalTimer.getSnapshot().getValues()); | |||
} | |||
|
|||
@Test | |||
public void testSnapshotPercentileValuesPresent() throws Exception { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we really need to test this?
bc40f0c
to
d5550ad
Compare
- Removed getMedian, getXXXPercentile, getMin, getStdDev, getValues from Snapshot - Added inner static class PercentileValue to Snapshot, percentileValues() method to Snapshot - Spec updates - TCK updates
d5550ad
to
2a493ab
Compare
@@ -140,34 +141,72 @@ public void testSnapshotValues() throws Exception { | |||
SAMPLE_LONG_DATA, histogramLong.getSnapshot().getValues()); | |||
} | |||
|
|||
@Test | |||
public void testSnapshotPercentileValuesPresent() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems reasonable for this release, given those are the expected percentiles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tck/api/src/main/java/org/eclipse/microprofile/metrics/tck/metrics/HistogramTest.java
Show resolved
Hide resolved
tck/api/src/main/java/org/eclipse/microprofile/metrics/tck/metrics/HistogramTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Update Snapshot used by Histogram and Timer
^methods removed were those that can not be supported by micrometer
fixes #695
fixes #696