Update MetricPoint for Histograms#2657
Conversation
src/OpenTelemetry.Exporter.Prometheus/Implementation/PrometheusSerializerExt.cs
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #2657 +/- ##
==========================================
- Coverage 81.33% 81.32% -0.02%
==========================================
Files 246 247 +1
Lines 8659 8670 +11
==========================================
+ Hits 7043 7051 +8
- Misses 1616 1619 +3
|
joaopgrassi
left a comment
joaopgrassi
left a comment
There was a problem hiding this comment.
Nice! This looks so much better 😀. Just left some minor comments.
| } | ||
| else | ||
| { | ||
| throw new InvalidOperationException($"{nameof(this.GetBucketCounts)} is not supported for this metric type."); |
There was a problem hiding this comment.
Should we really throw here? 🤔
There was a problem hiding this comment.
If we do want to throw, NotSupportedException is probably more appropriate.
There was a problem hiding this comment.
I think we should throw here. If we don't there would be no way for exporters to know if they are working correctly as they would just get some default values which is not the actual metric value.
| Assert.Null(histogramPoint.ExplicitBounds); | ||
| Assert.Equal(7, count); | ||
|
|
||
| Assert.Throws<InvalidOperationException>(() => histogramPoint.GetBucketCounts()); |
There was a problem hiding this comment.
I think we should really evaluate if we want to throw in these getters, as this might be unexpected. Specially since now the behavior is changed.
| } | ||
| else | ||
| { | ||
| throw new InvalidOperationException($"{nameof(this.GetHistogramSum)} is not supported for this metric type."); |
There was a problem hiding this comment.
Whatever we define (throw or just a log), I think it would be good to log the aggType or the metric type?
There was a problem hiding this comment.
AggType is private. We can create a mapping of AggType to MetricType and add that here. This could be done in a follow-up PR.
| { | ||
| if (this.aggType == AggregationType.Histogram || this.aggType == AggregationType.HistogramSumCount) | ||
| { | ||
| return this.histogramMeasurements.Count; |
There was a problem hiding this comment.
we can simply use the long and double values from MetricPoint itself. No need to add them to HistogramMeasurements.
There was a problem hiding this comment.
@CodeBlanch was planning to see if we can custom layout to overlap the long/double field. That'd mean we'd be able to still need one of them inside HistogramMeasurement.
There was a problem hiding this comment.
can be addressed as a follow up.
| OpenTelemetry.Metrics.MetricPoint.DoubleValue.get -> double | ||
| OpenTelemetry.Metrics.MetricPoint.EndTime.get -> System.DateTimeOffset | ||
| OpenTelemetry.Metrics.MetricPoint.ExplicitBounds.get -> double[] | ||
| OpenTelemetry.Metrics.MetricPoint.GetBucketCounts() -> long[] |
There was a problem hiding this comment.
as a follow up we need to do something similar to whats done for Tags. (readonly something)
| } | ||
|
|
||
| // Histogram sum | ||
| var count = metricPoint.GetHistogramCount(); |
There was a problem hiding this comment.
Do we need to declare these variables here, or just use them inline?
reyang
left a comment
reyang
left a comment
There was a problem hiding this comment.
LGTM with minor suggestions/comments.
5 participants
Partially fixes #2633
Changes
HistogramMeasurementsfor Hisotgram related dataGetBucketCounts,GetExplicitBounds,GetHistogramCount, andGetHistogramSumPlease 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