[exporter/awsemf] Drop metrics with NaN values #26344
Conversation
| func TestCalculateDeltaDatapoints_SummaryDataPointSlice(t *testing.T) { | ||
| emfCalcs := setupEmfCalculators() | ||
| defer require.NoError(t, shutdownEmfCalculators(emfCalcs)) | ||
| for _, retainInitialValueOfDeltaMetric := range []bool{true, false} { | ||
| deltaMetricMetadata := generateDeltaMetricMetadata(true, "foo", retainInitialValueOfDeltaMetric) | ||
| dmd := generateDeltaMetricMetadata(true, "foo", retainInitialValueOfDeltaMetric) |
There was a problem hiding this comment.
renamed many of these variables to avoid shadowing which caused linting fails.
| func init() { | ||
| os.Setenv("AWS_ACCESS_KEY_ID", "test") | ||
| os.Setenv("AWS_SECRET_ACCESS_KEY", "test") | ||
| } | ||
|
|
There was a problem hiding this comment.
These weren't required anymore and were causing warnings for unchecked error. Opted to remove dead code.
| // Drop stale or NaN metric values | ||
| if dps.IsStaleOrNaN(i) { | ||
| if config != nil && config.logger != nil { | ||
| config.logger.Debug("dropped metric with nan value", zap.String("metric.name", pmd.Name())) |
There was a problem hiding this comment.
Should this include the metric attributes? The name alone is not identifying.
There was a problem hiding this comment.
Is there a consistent way to log this without being too verbose? Or should we just log all metric attributes along with the name?
There was a problem hiding this comment.
I've updated the IsStaleOrNaN function signature to also return the attributes for each data point. I don't love this but it seemed like the cleanest way to ensure only one log line had to be added. I chose this over adding the logger as an function parameter to IsStaleOrNaN.
| if metric.Flags().NoRecordedValue() { | ||
| return true, metric.Attributes() | ||
| } | ||
| if math.IsNaN(metric.Max()) || math.IsNaN(metric.Sum()) || math.IsNaN(metric.Min()) { |
There was a problem hiding this comment.
q: do we also need to check on count as well?
There was a problem hiding this comment.
Count should be an integer and thus we don't need to check it. https://github.com/open-telemetry/opentelemetry-proto/blob/main/opentelemetry/proto/metrics/v1/metrics.proto#L412-L415
**Description:** Metrics with NaN values for float types would cause the EMF Exporter to error out during JSON Marshaling. This PR introduces a change to drop metric values that contain NaN. **Link to tracking Issue:** Fixes open-telemetry#26267 **Testing:** Added unit tests at several different points with varying levels of specificity. Unit tests are quite verbose in this example but I have followed the format of existing tests while doing very little refactoring. **Documentation:** Update README
Description: Metrics with NaN values for float types would cause the EMF Exporter to error out during JSON Marshaling. This PR introduces a change to drop metric values that contain NaN.
Link to tracking Issue: Fixes #26267
Testing: Added unit tests at several different points with varying levels of specificity. Unit tests are quite verbose in this example but I have followed the format of existing tests while doing very little refactoring.
Documentation: Update README