Skip to content

Commit

Permalink
[exporter/awsemf] Improve summary metric type NaN checks (open-teleme…
Browse files Browse the repository at this point in the history 
…try#28894)

**Description:** I have observed some behavior on a personal collector
deployment where the EMF Exporter is still returning errors for `NaN`
json marshalling. This was in a prometheus -> emf exporter metrics
pipeline.

I could not find the specific NaN value in the metrics when
troubleshooting the error. I curled the `/metrics` endpoint and also
tried using the logging exporter to try to get more information. I could
not find where the NaN value was coming from so I took another look into
the unit tests and found some possible code paths in which NaNs could
slip though.

**Link to tracking Issue:** Original issue
open-telemetry#26267

**Testing:** Added more unit tests. The summary unit tests got a slight
refactor for two reasons. So I could get ride of the unnecessary
typecasting and so that we could more easily test out different
combinations of quantile values.

I have also added a few more histogram unit tests to just verify that
all combinations of NaN values are being checked on their own.
  • Loading branch information
@bryan-aguilar @RoryCrispin
bryan-aguilar authored and RoryCrispin committed Nov 24, 2023
1 parent 8bf14e7 commit f0060e9
Show file tree
Hide file tree
Showing 3 changed files with 238 additions and 22 deletions.
27 changes: 27 additions & 0 deletions .chloggen/awsemf_SummaryNanCheck.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: enhancement

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: awsemfexporter

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: Improve NaN value checking for Summary metric types.

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [28894]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: []
9 changes: 9 additions & 0 deletions exporter/awsemfexporter/datapoint.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,15 @@ func (dps summaryDataPointSlice) IsStaleOrNaN(i int) (bool, pcommon.Map) {
if math.IsNaN(metric.Sum()) {
return true, metric.Attributes()
}

values := metric.QuantileValues()
for i := 0; i < values.Len(); i++ {
quantile := values.At(i)
if math.IsNaN(quantile.Value()) || math.IsNaN(quantile.Quantile()) {
return true, metric.Attributes()
}
}

return false, metric.Attributes()
}

Expand Down
224 changes: 202 additions & 22 deletions exporter/awsemfexporter/datapoint_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,11 +223,11 @@ func generateTestSummaryMetricWithNaN(name string) pmetric.Metrics {
summaryDatapoint.SetCount(uint64(5 * i))
summaryDatapoint.SetSum(math.NaN())
firstQuantile := summaryDatapoint.QuantileValues().AppendEmpty()
firstQuantile.SetQuantile(0.0)
firstQuantile.SetValue(1)
firstQuantile.SetQuantile(math.NaN())
firstQuantile.SetValue(math.NaN())
secondQuantile := summaryDatapoint.QuantileValues().AppendEmpty()
secondQuantile.SetQuantile(100.0)
secondQuantile.SetValue(5)
secondQuantile.SetQuantile(math.NaN())
secondQuantile.SetValue(math.NaN())
}

return otelMetrics
Expand Down Expand Up @@ -543,7 +543,7 @@ func TestIsStaleOrNaN_HistogramDataPointSlice(t *testing.T) {
setFlagsFunc func(point pmetric.HistogramDataPoint) pmetric.HistogramDataPoint
}{
{
name: "Histogram with NaNs",
name: "Histogram with all NaNs",
histogramDPS: func() pmetric.HistogramDataPointSlice {
histogramDPS := pmetric.NewHistogramDataPointSlice()
histogramDP := histogramDPS.AppendEmpty()
Expand All @@ -556,6 +556,48 @@ func TestIsStaleOrNaN_HistogramDataPointSlice(t *testing.T) {
}(),
boolAssertFunc: assert.True,
},
{
name: "Histogram with NaN Sum",
histogramDPS: func() pmetric.HistogramDataPointSlice {
histogramDPS := pmetric.NewHistogramDataPointSlice()
histogramDP := histogramDPS.AppendEmpty()
histogramDP.SetCount(uint64(17))
histogramDP.SetSum(math.NaN())
histogramDP.SetMin(1234)
histogramDP.SetMax(1234)
histogramDP.Attributes().PutStr("label1", "value1")
return histogramDPS
}(),
boolAssertFunc: assert.True,
},
{
name: "Histogram with NaN Min",
histogramDPS: func() pmetric.HistogramDataPointSlice {
histogramDPS := pmetric.NewHistogramDataPointSlice()
histogramDP := histogramDPS.AppendEmpty()
histogramDP.SetCount(uint64(17))
histogramDP.SetSum(123)
histogramDP.SetMin(math.NaN())
histogramDP.SetMax(123)
histogramDP.Attributes().PutStr("label1", "value1")
return histogramDPS
}(),
boolAssertFunc: assert.True,
},
{
name: "Histogram with nan Max",
histogramDPS: func() pmetric.HistogramDataPointSlice {
histogramDPS := pmetric.NewHistogramDataPointSlice()
histogramDP := histogramDPS.AppendEmpty()
histogramDP.SetCount(uint64(17))
histogramDP.SetSum(123)
histogramDP.SetMin(123)
histogramDP.SetMax(math.NaN())
histogramDP.Attributes().PutStr("label1", "value1")
return histogramDPS
}(),
boolAssertFunc: assert.True,
},
{
name: "Histogram with min and max",
histogramDPS: func() pmetric.HistogramDataPointSlice {
Expand Down Expand Up @@ -727,6 +769,62 @@ func TestIsStaleOrNaN_ExponentialHistogramDataPointSlice(t *testing.T) {
}(),
boolAssertFunc: assert.False,
},
{
name: "Exponential histogram with all possible NaN",
histogramDPS: func() pmetric.ExponentialHistogramDataPointSlice {
histogramDPS := pmetric.NewExponentialHistogramDataPointSlice()
histogramDP := histogramDPS.AppendEmpty()
histogramDP.SetCount(uint64(17))
histogramDP.SetSum(math.NaN())
histogramDP.SetMin(math.NaN())
histogramDP.SetMax(math.NaN())
histogramDP.Attributes().PutStr("label1", "value1")
return histogramDPS
}(),
boolAssertFunc: assert.True,
},
{
name: "Exponential histogram with NaN Sum",
histogramDPS: func() pmetric.ExponentialHistogramDataPointSlice {
histogramDPS := pmetric.NewExponentialHistogramDataPointSlice()
histogramDP := histogramDPS.AppendEmpty()
histogramDP.SetCount(uint64(17))
histogramDP.SetSum(math.NaN())
histogramDP.SetMin(1245)
histogramDP.SetMax(1234556)
histogramDP.Attributes().PutStr("label1", "value1")
return histogramDPS
}(),
boolAssertFunc: assert.True,
},
{
name: "Exponential histogram with NaN Min",
histogramDPS: func() pmetric.ExponentialHistogramDataPointSlice {
histogramDPS := pmetric.NewExponentialHistogramDataPointSlice()
histogramDP := histogramDPS.AppendEmpty()
histogramDP.SetCount(uint64(17))
histogramDP.SetSum(1255)
histogramDP.SetMin(math.NaN())
histogramDP.SetMax(12545)
histogramDP.Attributes().PutStr("label1", "value1")
return histogramDPS
}(),
boolAssertFunc: assert.True,
},
{
name: "Exponential histogram with NaN Max",
histogramDPS: func() pmetric.ExponentialHistogramDataPointSlice {
histogramDPS := pmetric.NewExponentialHistogramDataPointSlice()
histogramDP := histogramDPS.AppendEmpty()
histogramDP.SetCount(uint64(17))
histogramDP.SetSum(512444)
histogramDP.SetMin(123)
histogramDP.SetMax(math.NaN())
histogramDP.Attributes().PutStr("label1", "value1")
return histogramDPS
}(),
boolAssertFunc: assert.True,
},
{
name: "Exponential histogram with NaNs",
histogramDPS: func() pmetric.ExponentialHistogramDataPointSlice {
Expand Down Expand Up @@ -862,49 +960,131 @@ func TestCalculateDeltaDatapoints_SummaryDataPointSlice(t *testing.T) {
}

func TestIsStaleOrNaN_SummaryDataPointSlice(t *testing.T) {
type qMetricObject struct {
value float64
quantile float64
}
type quantileTestObj struct {
sum float64
count uint64
qMetrics []qMetricObject
}
testCases := []struct {
name string
summaryMetricValue map[string]any
summaryMetricValue quantileTestObj
expectedBoolAssert assert.BoolAssertionFunc
setFlagsFunc func(point pmetric.SummaryDataPoint) pmetric.SummaryDataPoint
}{
{
name: "summary with no nan values",
summaryMetricValue: map[string]any{"sum": float64(17.3), "count": uint64(17), "firstQuantile": float64(1), "secondQuantile": float64(5)},
name: "summary with no nan values",
summaryMetricValue: quantileTestObj{
sum: 17.3,
count: 17,
qMetrics: []qMetricObject{
{
value: 1,
quantile: 0.5,
},
{
value: 5,
quantile: 2.0,
},
},
},
expectedBoolAssert: assert.False,
},
{
name: "Summary with nan values",
summaryMetricValue: map[string]any{"sum": math.NaN(), "count": uint64(25), "firstQuantile": math.NaN(), "secondQuantile": math.NaN()},
name: "Summary with nan sum",
summaryMetricValue: quantileTestObj{
sum: math.NaN(),
count: 17,
qMetrics: []qMetricObject{
{
value: 1,
quantile: 0.5,
},
{
value: 5,
quantile: 2.0,
},
},
},
expectedBoolAssert: assert.True,
},
{
name: "Summary with set flag func",
summaryMetricValue: map[string]any{"sum": math.NaN(), "count": uint64(25), "firstQuantile": math.NaN(), "secondQuantile": math.NaN()},
name: "Summary with no recorded value flag set to true",
summaryMetricValue: quantileTestObj{
sum: 1245.65,
count: 17,
qMetrics: []qMetricObject{
{
value: 1,
quantile: 0.5,
},
{
value: 5,
quantile: 2.0,
},
},
},
expectedBoolAssert: assert.True,
setFlagsFunc: func(point pmetric.SummaryDataPoint) pmetric.SummaryDataPoint {
point.SetFlags(pmetric.DefaultDataPointFlags.WithNoRecordedValue(true))
return point
},
},
{
name: "Summary with nan quantile value",
summaryMetricValue: quantileTestObj{
sum: 1245.65,
count: 17,
qMetrics: []qMetricObject{
{
value: 1,
quantile: 0.5,
},
{
value: math.NaN(),
quantile: 2.0,
},
},
},
expectedBoolAssert: assert.True,
},
{
name: "Summary with nan quantile",
summaryMetricValue: quantileTestObj{
sum: 1245.65,
count: 17,
qMetrics: []qMetricObject{
{
value: 1,
quantile: 0.5,
},
{
value: 7.8,
quantile: math.NaN(),
},
},
},
expectedBoolAssert: assert.True,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
// Given the summary datapoints with quantile 0, quantile 100, sum and count
summaryDPS := pmetric.NewSummaryDataPointSlice()
summaryDP := summaryDPS.AppendEmpty()
summaryDP.SetSum(tc.summaryMetricValue["sum"].(float64))
summaryDP.SetCount(tc.summaryMetricValue["count"].(uint64))
summaryDP.SetSum(tc.summaryMetricValue.sum)
summaryDP.SetCount(tc.summaryMetricValue.count)
summaryDP.Attributes().PutStr("label1", "value1")

summaryDP.QuantileValues().EnsureCapacity(2)
firstQuantileValue := summaryDP.QuantileValues().AppendEmpty()
firstQuantileValue.SetQuantile(0)
firstQuantileValue.SetValue(tc.summaryMetricValue["firstQuantile"].(float64))
secondQuantileValue := summaryDP.QuantileValues().AppendEmpty()
secondQuantileValue.SetQuantile(100)
secondQuantileValue.SetValue(tc.summaryMetricValue["secondQuantile"].(float64))
summaryDP.QuantileValues().EnsureCapacity(len(tc.summaryMetricValue.qMetrics))
for _, qMetric := range tc.summaryMetricValue.qMetrics {
newQ := summaryDP.QuantileValues().AppendEmpty()
newQ.SetValue(qMetric.value)
newQ.SetQuantile(qMetric.quantile)
}

summaryDatapointSlice := summaryDataPointSlice{deltaMetricMetadata{}, summaryDPS}
if tc.setFlagsFunc != nil {
Expand Down

0 comments on commit f0060e9

Please sign in to comment.