From cc367ec12c7c9a56aa77ce88ac75614e63ef8471 Mon Sep 17 00:00:00 2001 From: Nicholas Thompson Date: Thu, 12 Dec 2024 19:20:08 +0200 Subject: [PATCH] Add DisableSkipErrMetrics to disable ErrSkip metrics (#389) DisableSkipErrMetrics will skip the recoding of any SkipErr metric if enabled. Since SkipErr results in a retry, the actual database interaction will still be recorded. Closes #386 --------- Co-authored-by: Sam Xie --- CHANGELOG.md | 4 +++ config.go | 5 ++++ option.go | 8 +++++ option_test.go | 5 ++++ utils.go | 6 +++- utils_test.go | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 108 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47255073..61f439b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ## [Unreleased] +### Added + +- `DisableSkipErrMeasurement` option suppresses `driver.ErrSkip` as an error status in measurements if enabled. (#389) + ## [0.35.0] - 2024-10-11 ### Changed diff --git a/config.go b/config.go index 3b722968..a2e01640 100644 --- a/config.go +++ b/config.go @@ -80,6 +80,11 @@ type config struct { // InstrumentAttributesGetter will be called to produce additional attributes while recording metrics to instruments. // Default returns nil InstrumentAttributesGetter InstrumentAttributesGetter + + // DisableSkipErrMeasurement, if set to true, will suppress driver.ErrSkip as an error status in measurements. + // The measurement will be recorded as status=ok. + // Default is false + DisableSkipErrMeasurement bool } // SpanOptions holds configuration of tracing span to decide diff --git a/option.go b/option.go index 9c2ec26b..1406eca0 100644 --- a/option.go +++ b/option.go @@ -108,3 +108,11 @@ func WithInstrumentAttributesGetter(instrumentAttributesGetter InstrumentAttribu cfg.InstrumentAttributesGetter = instrumentAttributesGetter }) } + +// WittDisableSkipErrMeasurement, if set to true, will suppress driver.ErrSkip as an error status in measurements. +// The measurement will be recorded as status=ok. +func WithDisableSkipErrMeasurement(disable bool) Option { + return OptionFunc(func(cfg *config) { + cfg.DisableSkipErrMeasurement = disable + }) +} diff --git a/option_test.go b/option_test.go index 73347330..89c5e33e 100644 --- a/option_test.go +++ b/option_test.go @@ -84,6 +84,11 @@ func TestOptions(t *testing.T) { option: WithInstrumentAttributesGetter(dummyAttributesGetter), expectedConfig: config{InstrumentAttributesGetter: dummyAttributesGetter}, }, + { + name: "WithDisableSkipErrMeasurement", + option: WithDisableSkipErrMeasurement(true), + expectedConfig: config{DisableSkipErrMeasurement: true}, + }, } for _, tc := range testCases { diff --git a/utils.go b/utils.go index 10c279ae..aba76bf4 100644 --- a/utils.go +++ b/utils.go @@ -70,7 +70,11 @@ func recordMetric( attributes = append(attributes, cfg.InstrumentAttributesGetter(ctx, method, query, args)...) } if err != nil { - attributes = append(attributes, queryStatusKey.String("error")) + if cfg.DisableSkipErrMeasurement && err == driver.ErrSkip { + attributes = append(attributes, queryStatusKey.String("ok")) + } else { + attributes = append(attributes, queryStatusKey.String("error")) + } } else { attributes = append(attributes, queryStatusKey.String("ok")) } diff --git a/utils_test.go b/utils_test.go index 563b07af..15650b0e 100644 --- a/utils_test.go +++ b/utils_test.go @@ -25,6 +25,7 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" + "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/noop" sdktrace "go.opentelemetry.io/otel/sdk/trace" "go.opentelemetry.io/otel/sdk/trace/tracetest" @@ -257,3 +258,83 @@ var omit SpanFilter = func(_ context.Context, _ Method, _ string, _ []driver.Nam var keep SpanFilter = func(_ context.Context, _ Method, _ string, _ []driver.NamedValue) bool { return true } + +func TestRecordMetric(t *testing.T) { + type args struct { + ctx context.Context + cfg config + method Method + query string + args []driver.NamedValue + } + tests := []struct { + name string + args args + recordErr error + expectedStatus string + }{ + { + name: "metric with no error", + args: args{ + cfg: newConfig(), + method: MethodConnQuery, + query: "example query", + }, + recordErr: nil, + expectedStatus: "ok", + }, + { + name: "metric with an error", + args: args{ + cfg: newConfig(), + method: MethodConnQuery, + query: "example query", + }, + recordErr: assert.AnError, + expectedStatus: "error", + }, + { + name: "metric with skip error but not disabled", + args: args{ + cfg: newConfig(), + method: MethodConnQuery, + query: "example query", + }, + recordErr: driver.ErrSkip, + expectedStatus: "error", + }, + { + name: "metric with skip error but disabled", + args: args{ + cfg: newConfig(WithDisableSkipErrMeasurement(true)), + method: MethodConnQuery, + query: "example query", + }, + recordErr: driver.ErrSkip, + expectedStatus: "ok", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockLatency := &float64HistogramMock{} + mockInstruments := &instruments{ + latency: mockLatency, + } + recordFunc := recordMetric(tt.args.ctx, mockInstruments, tt.args.cfg, tt.args.method, tt.args.query, tt.args.args) + recordFunc(tt.recordErr) + assert.Equal(t, tt.expectedStatus, mockLatency.status) + }) + } +} + +type float64HistogramMock struct { + // Add metric.Float64Histogram so we only need to implement the function we care about for the mock + metric.Float64Histogram + status string +} + +func (m *float64HistogramMock) Record(_ context.Context, _ float64, opts ...metric.RecordOption) { + attr := metric.NewRecordConfig(opts).Attributes() + statusVal, _ := attr.Value(queryStatusKey) + m.status = statusVal.AsString() +}