-
Notifications
You must be signed in to change notification settings - Fork 95
Add support for Summary type and value. #110
Add support for Summary type and value. #110
Conversation
c492e59
to
7dc940f
Compare
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. Can you fix the build failures?
// percentiles directly. | ||
// | ||
// See also: https://prometheus.io/docs/concepts/metric_types/#summary | ||
// A summary value. |
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.
Maybe worth repeating here that it's not recommended.
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.
done.
|
||
// The start_timestamp only applies to the count and sum in the SummaryValue. | ||
message SummaryValue { | ||
// The total number of recorded values from the beginning. Optional since |
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.
from the beginning -> since start_time
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.
done.
google.protobuf.UInt64Value count = 1; | ||
|
||
// The sum of values in the snapshot. Optional since some systems don't | ||
// expose this. If count is zero then this field must be zero or not set |
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.
How about "This field must be unset if the sum is not available."
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.
done.
|
||
message Quantile { | ||
// Must be in the interval (0.0, 1.0] | ||
required double quantile = 1; |
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.
Remove "required" this is proto3 ;)
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.
done.
} | ||
} | ||
|
||
// Values calculated from over a sliding time window. |
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.
How about "Values calculated over an arbitrary time window."
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.
done.
7dc940f
to
0ca2a0e
Compare
// (usually things like request durations and response sizes). While it | ||
// also provides a total count of observations and a sum of all observed | ||
// values, it calculates configurable quantiles over a sliding time window. | ||
// This is not recommended, since cannot be aggregated. |
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.
nit: s/since cannot/since it cannot/
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.
done.
// percentiles directly. | ||
// | ||
// See also: https://prometheus.io/docs/concepts/metric_types/#summary | ||
// A summary value. This is not recommended, since cannot be aggregated. |
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.
ditto
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.
done.
@@ -91,6 +92,13 @@ message MetricDescriptor { | |||
// Distribution cumulative measurement. The count can only go up, if resets | |||
// then the start_time should also be reset. | |||
CUMULATIVE_DISTRIBUTION = 5; | |||
|
|||
// Old libraries implemented Histograms as a summary samples observations |
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.
as a summary samples observations -> as a summary of observations? measurements?
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.
done.
@@ -91,6 +92,13 @@ message MetricDescriptor { | |||
// Distribution cumulative measurement. The count can only go up, if resets | |||
// then the start_time should also be reset. | |||
CUMULATIVE_DISTRIBUTION = 5; | |||
|
|||
// Old libraries implemented Histograms as a summary samples observations |
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.
Old libraries implemented -> some frameworks implement ?
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.
done.
0ca2a0e
to
1673321
Compare
// also provides a total count of observations and a sum of all observed | ||
// values, it calculates configurable quantiles over a sliding time window. | ||
// This is not recommended, since it cannot be aggregated. | ||
SUMMARY = 7; |
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.
I think this should be number 6.
This value better supports dropwizard histograms.