diff --git a/CHANGELOG.md b/CHANGELOG.md index b11178cb..7979d77d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lightstep/sdk/metric/aggregator/aggregator.go b/lightstep/sdk/metric/aggregator/aggregator.go index 45293948..7a407a3e 100644 --- a/lightstep/sdk/metric/aggregator/aggregator.go +++ b/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 } } diff --git a/lightstep/sdk/metric/internal/asyncstate/async_test.go b/lightstep/sdk/metric/internal/asyncstate/async_test.go index 1b20c83c..3fe51a93 100644 --- a/lightstep/sdk/metric/internal/asyncstate/async_test.go +++ b/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) } diff --git a/lightstep/sdk/metric/internal/syncstate/sync_test.go b/lightstep/sdk/metric/internal/syncstate/sync_test.go index 7e69ad1c..18d2f59b 100644 --- a/lightstep/sdk/metric/internal/syncstate/sync_test.go +++ b/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) {