Update MetricPoint HistogramMeasurements#2664
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2664 +/- ##
==========================================
+ Coverage 82.24% 82.28% +0.03%
==========================================
Files 248 249 +1
Lines 8692 8703 +11
==========================================
+ Hits 7149 7161 +12
+ Misses 1543 1542 -1
|
|
|
||
| namespace OpenTelemetry.Metrics | ||
| { | ||
| public readonly struct HistogramMeasurement |
There was a problem hiding this comment.
the term "measurement" is misleading as it refers to the raw data point reported through the API. https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#measurement
| namespace OpenTelemetry.Metrics | ||
| { | ||
| internal class HistogramMeasurements | ||
| public class HistogramMeasurements |
There was a problem hiding this comment.
| public class HistogramMeasurements | |
| public class HistogramMetricPoint |
There was a problem hiding this comment.
Is this a HistogramMetricPoint or it is a HistogramMetricPointBucket?
If it is a HistogramMetricPoint, maybe it should inherit from MetricPoint?
There was a problem hiding this comment.
HistogramMetricPointBucket is correct.
There was a problem hiding this comment.
HistogramBuckets - this sounds better name.
| private readonly bool isHistogramSumCount; | ||
| private readonly int numberOfBuckets; | ||
| private readonly int numberofExplicitBounds; |
There was a problem hiding this comment.
If you look at the original one I did, I had a Count on the parent class which I used in MoveNext. The reason for that is it is better for perf to keep structs as small as possible. What I don't know is if that size-reduction provides more perf than caching these values in the struct to reference in each MoveNext invocation. Might be worth measuring and possibly adjusting (could be a follow-up).
At the least, I think we can remove bool isHistogramSumCount because this.numberOfBuckets will be 0 in that case so MoveNext will know to exit. Basically...
public bool MoveNext()
{
if (this.index < this.numberOfBuckets)...should be enough (I think)?
There was a problem hiding this comment.
I have removed isHistogramSumCount. I will do the benchmarking for size-reduction vs caching later on.
| throw new NotSupportedException($"{nameof(this.GetBucketCounts)} is not supported for this metric type."); | ||
| } | ||
| } | ||
| public readonly HistogramMeasurements HistogramMeasurements { get; } |
There was a problem hiding this comment.
Just typing my understanding:
MetricPoint.GetHistogramSum, GetHistogramCount is used to retrieve sum/count of Histogram. (always expected for a Histogram)
For Buckets (optional), it'll be retrieved from the HistogramBuckets.
There was a problem hiding this comment.
I think that might change after the struct layout stuff I'm looking at. I think leave it like this for now and then we can revisit?
|
|
||
| namespace OpenTelemetry.Metrics | ||
| { | ||
| public class HistogramBuckets |
There was a problem hiding this comment.
since this class holds more than buckets (sum,count) , this name is probably not correct as well.
Lets iterate on it a bit more, after @CodeBlanch does custom struct layout as well.
Also from the datamodel, (
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/datamodel.md#histogram), the term bucket seem to indicate a "triplet" with lower bound, upper bound, and the count. So might need another name.
Not blocking this PR, but needs addressing before stable.
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
have some comments, to be addressed as follow ups.
4 participants
The objective of this PR is provide a way to abstract away the implementation detail of
BucketCountsandExplicitBoundsand prevent the user from updating values forBucketCountsorExplicitBounds.This PR achieves it by providing an enumerator for
MetricPoint.HistogramMeasurementswhich returns a newstructcalledHistogramMeasurementstruct which has anExplicitBoundandBucketCountpair. The last pair hasdouble.PositiveInfinityas the explicit bound.This is built on @CodeBlanch 's idea: https://github.com/open-telemetry/opentelemetry-dotnet/compare/main...CodeBlanch:histogram-measurements?expand=1
Changes
HistogramMeasurementstruct which has anExplicitBoundandBucketCountpairGetHistogramBucketCountsandGetHistogramExplicitBoundsPlease provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes