Avoid frequent logging of invalid-values (Inf, NaN, Negatives) (#272)

CHANGELOG.md

@@ -18,8 +18,13 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
   [host](https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/instrumentation/host)
   metrics instrumentation will be reported automatically.
   [#265](https://github.com/lightstep/otel-launcher-go/pull/265)
-- Proposed replacement for go-contrib instrumentation/runtime added as lightstep/instrumentation/runtime. 
+- Proposed replacement for go-contrib instrumentation/runtime added as lightstep/instrumentation/runtime.
   [#267](https://github.com/lightstep/otel-launcher-go/pull/267)
+- Avoid repetitive calls to `otel.Handle()` with error conditions
+  caused by out-of-range metrics input values, including NaN, Inf, and
+  in some cases negative values.  These will be handled no more than
+  once per 30 seconds per condition.
+  [#272](https://github.com/lightstep/otel-launcher-go/pull/272)
 
 ## [1.10.1](https://github.com/lightstep/otel-launcher-go/releases/tag/v1.10.1) - 2022-08-29
 

lightstep/sdk/metric/aggregator/aggregator.go

@@ -16,9 +16,11 @@ package aggregator // import "github.com/lightstep/otel-launcher-go/lightstep/sd
 
 import (
 	"fmt"
+	"time"
 
 	"github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/aggregation"
 	histostruct "github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/histogram/structure"
+	"github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/internal/doevery"
 	"github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/number"
 	"github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/sdkinstrument"
 	"go.opentelemetry.io/otel"
@@ -39,12 +41,16 @@ func RangeTest[N number.Any, Traits number.Traits[N]](num N, desc sdkinstrument.
 	var traits Traits
 
 	if traits.IsInf(num) {
-		otel.Handle(fmt.Errorf("%s: %w", desc.Name, ErrInfInput))
+		doevery.TimePeriod(30*time.Second, func() {
+			otel.Handle(fmt.Errorf("%s: %w", desc.Name, ErrInfInput))
+		})
 		return false
 	}
 
 	if traits.IsNaN(num) {
-		otel.Handle(fmt.Errorf("%s: %w", desc.Name, ErrNaNInput))
+		doevery.TimePeriod(30*time.Second, func() {
+			otel.Handle(fmt.Errorf("%s: %w", desc.Name, ErrNaNInput))
+		})
 		return false
 	}
 
@@ -53,7 +59,9 @@ func RangeTest[N number.Any, Traits number.Traits[N]](num N, desc sdkinstrument.
 	case sdkinstrument.SyncCounter,
 		sdkinstrument.SyncHistogram:
 		if num < 0 {
-			otel.Handle(fmt.Errorf("%s: %w", desc.Name, ErrNegativeInput))
+			doevery.TimePeriod(30*time.Second, func() {
+				otel.Handle(fmt.Errorf("%s: %w", desc.Name, ErrNegativeInput))
+			})
 			return false
 		}
 	}

lightstep/sdk/metric/internal/asyncstate/async_test.go

@@ -362,13 +362,10 @@ func TestOutOfRangeValues(t *testing.T) {
 	}, tt, func(ctx context.Context) {
 		c.Observe(ctx, math.NaN())
 		c.Observe(ctx, math.Inf(+1))
-		c.Observe(ctx, math.Inf(-1))
 		u.Observe(ctx, math.NaN())
 		u.Observe(ctx, math.Inf(+1))
-		u.Observe(ctx, math.Inf(-1))
 		g.Observe(ctx, math.NaN())
 		g.Observe(ctx, math.Inf(+1))
-		g.Observe(ctx, math.Inf(-1))
 	})
 
 	runFor := func(num int) {
@@ -403,15 +400,23 @@ func TestOutOfRangeValues(t *testing.T) {
 		)
 	}
 
-	// 2 readers x 3 error conditions x 3 instruments
-	require.Equal(t, 2*3*3, len(*otelErrs))
+	// Errors are rate limited, but this is the only test in this
+	// package that uses invalid values.  We should have at least
+	// one per class.
+	require.LessOrEqual(t, 2, len(*otelErrs))
 
+	haveNaN := false
+	haveInf := false
 	for _, err := range *otelErrs {
 		isNaN := errors.Is(err, aggregator.ErrNaNInput)
 		isInf := errors.Is(err, aggregator.ErrInfInput)
-		isNeg := errors.Is(err, aggregator.ErrNegativeInput)
 
-		require.True(t, isNaN || isInf || isNeg)
+		require.True(t, isNaN || isInf)
 		require.True(t, strings.HasPrefix(err.Error(), "testPattern"))
+
+		haveNaN = haveNaN || isNaN
+		haveInf = haveInf || isInf
 	}
+	require.True(t, haveNaN)
+	require.True(t, haveInf)
 }

lightstep/sdk/metric/internal/syncstate/sync_test.go

@@ -16,12 +16,14 @@ package syncstate
 
 import (
 	"context"
+	"errors"
 	"math"
 	"math/rand"
 	"sync"
 	"testing"
 	"time"
 
+	"github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator"
 	"github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/aggregation"
 	"github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/gauge"
 	"github.com/lightstep/otel-launcher-go/lightstep/sdk/metric/aggregator/histogram"
@@ -329,6 +331,8 @@ func TestSyncStateFullNoopInstrument(t *testing.T) {
 }
 
 func TestOutOfRangeValues(t *testing.T) {
+	otelErrs := test.OTelErrors()
+
 	for _, desc := range []sdkinstrument.Descriptor{
 		test.Descriptor("cf", sdkinstrument.SyncCounter, number.Float64Kind),
 		test.Descriptor("uf", sdkinstrument.SyncUpDownCounter, number.Float64Kind),
@@ -392,6 +396,29 @@ func TestOutOfRangeValues(t *testing.T) {
 			),
 		)
 	}
+
+	// Errors are rate limited, but this is the only test in this
+	// package that uses invalid values.  We should have at least
+	// one per class.
+	require.LessOrEqual(t, 3, len(*otelErrs))
+
+	haveNaN := false
+	haveInf := false
+	haveNeg := false
+	for _, err := range *otelErrs {
+		isNaN := errors.Is(err, aggregator.ErrNaNInput)
+		isInf := errors.Is(err, aggregator.ErrInfInput)
+		isNeg := errors.Is(err, aggregator.ErrNegativeInput)
+
+		require.True(t, isNaN || isInf || isNeg)
+
+		haveNaN = haveNaN || isNaN
+		haveInf = haveInf || isInf
+		haveNeg = haveNeg || isNeg
+	}
+	require.True(t, haveNaN)
+	require.True(t, haveInf)
+	require.True(t, haveNeg)
 }
 
 func TestSyncGaugeDeltaInstrument(t *testing.T) {