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 <[email protected]>

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

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

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
+	})
+}

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 {

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"))
 		}

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()
+}