feat: refactor http operation metrics to handle multiple frameworks

instrumentation/hypertrace/metrics.go

@@ -0,0 +1,7 @@
+package hypertrace // import "github.com/hypertrace/goagent/instrumentation/hypertrace"
+
+import (
+	"github.com/hypertrace/goagent/instrumentation/opentelemetry"
+)
+
+var NewHttpOperationMetricsHandler = opentelemetry.NewHttpOperationMetricsHandler

instrumentation/hypertrace/net/hyperhttp/handler.go

@@ -15,8 +15,10 @@ func NewHandler(base http.Handler, operation string, opts ...Option) http.Handle
 		opt(o)
 	}
 
+	mh := opentelemetry.NewHttpOperationMetricsHandler(func(_ *http.Request) string { return operation })
+
 	return otelhttp.NewHandler(
-		sdkhttp.WrapHandler(base, operation, opentelemetry.SpanFromContext, o.toSDKOptions(), map[string]string{}),
+		sdkhttp.WrapHandler(base, opentelemetry.SpanFromContext, o.toSDKOptions(), map[string]string{}, mh),
 		operation,
 	)
 }

instrumentation/opencensus/metrics.go

@@ -0,0 +1,21 @@
+package opencensus // import "github.com/hypertrace/goagent/instrumentation/opencensus"
+
+import (
+	"net/http"
+
+	"github.com/hypertrace/goagent/sdk"
+)
+
+// Will not support metrics for OC instrumentation
+type httpOperationMetricsHandler struct {
+}
+
+func NewHttpOperationMetricsHandler() sdk.HttpOperationMetricsHandler {
+	return &httpOperationMetricsHandler{}
+}
+
+func (mh *httpOperationMetricsHandler) CreateRequestCount() {
+}
+
+func (mh *httpOperationMetricsHandler) AddToRequestCount(n int64, r *http.Request) {
+}

instrumentation/opencensus/net/hyperhttp/handler.go

@@ -10,6 +10,6 @@ import (
 // WrapHandler returns a new http.Handler that should be passed to
 // the *ochttp.Handler
 func WrapHandler(delegate http.Handler, options *sdkhttp.Options) http.Handler {
-	// TODO: If I am doing this then I might have the metrics code in the wrong place.
-	return sdkhttp.WrapHandler(delegate, "", opencensus.SpanFromContext, options, map[string]string{})
+	return sdkhttp.WrapHandler(delegate, opencensus.SpanFromContext, options, map[string]string{},
+		opencensus.NewHttpOperationMetricsHandler())
 }

instrumentation/opentelemetry/github.com/gin-gonic/hypergin/gin.go

@@ -91,8 +91,10 @@ func Middleware(options *sdkhttp.Options) gin.HandlerFunc {
 			ctx := context.WithValue(rc, hyperGinKey, ginRoute{route: ginOperationName})
 			wrappedHandler.c.Request = wrappedHandler.c.Request.WithContext(ctx)
 		}
+
+		mh := opentelemetry.NewHttpOperationMetricsHandler(func(_ *http.Request) string { return ginOperationName })
 		return otelhttp.NewHandler(
-			sdkhttp.WrapHandler(delegate, ginOperationName, opentelemetry.SpanFromContext, options, map[string]string{}),
+			sdkhttp.WrapHandler(delegate, opentelemetry.SpanFromContext, options, map[string]string{}, mh),
 			"",
 			otelhttp.WithSpanNameFormatter(spanNameFormatter),
 		)

instrumentation/opentelemetry/github.com/gorilla/hypermux/mux.go

@@ -9,8 +9,13 @@ import (
 	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
 )
 
-func spanNameFormatter(operation string, r *http.Request) (spanName string) {
+func spanNameFormatter(operation string, r *http.Request) string {
+	return getOperationNameFromRoute(r)
+}
+
+func getOperationNameFromRoute(r *http.Request) string {
 	route := mux.CurrentRoute(r)
+	spanName := ""
 	if route != nil {
 		var err error
 		spanName, err = route.GetPathTemplate()
@@ -24,16 +29,15 @@ func spanNameFormatter(operation string, r *http.Request) (spanName string) {
 		// want to use the method as fallback.
 		spanName = r.Method
 	}
-
-	return
+	return spanName
 }
 
 // NewMiddleware sets up a handler to start tracing the incoming requests.
 func NewMiddleware(options *sdkhttp.Options) mux.MiddlewareFunc {
-	// TODO: Get a proper operation name for http gorilla mux
+	mh := opentelemetry.NewHttpOperationMetricsHandler(getOperationNameFromRoute)
 	return func(delegate http.Handler) http.Handler {
 		return otelhttp.NewHandler(
-			sdkhttp.WrapHandler(delegate, "", opentelemetry.SpanFromContext, options, map[string]string{}),
+			sdkhttp.WrapHandler(delegate, opentelemetry.SpanFromContext, options, map[string]string{}, mh),
 			"",
 			otelhttp.WithSpanNameFormatter(spanNameFormatter),
 		)

instrumentation/opentelemetry/init.go

@@ -246,6 +246,9 @@ func InitWithSpanProcessorWrapper(cfg *config.AgentConfig, wrapper SpanProcessor
 		}
 	}
 
+	// Initialize metrics
+	metricsShutdownFn := initializeMetrics(cfg)
+
 	exporterFactory = makeExporterFactory(cfg)
 
 	exporter, err := exporterFactory()
@@ -279,9 +282,6 @@ func InitWithSpanProcessorWrapper(cfg *config.AgentConfig, wrapper SpanProcessor
 
 	otel.SetTextMapPropagator(makePropagator(cfg.PropagationFormats))
 
-	// Initialize metrics
-	metricsShutdownFn := initializeMetrics(cfg)
-
 	traceProviders = make(map[string]*sdktrace.TracerProvider)
 	globalSampler = sampler
 	initialized = true
@@ -421,7 +421,10 @@ func initializeMetrics(cfg *config.AgentConfig) func() {
 	otelmetricglobal.SetMeterProvider(metricsPusher)
 
 	return func() {
-		metricsPusher.Stop(context.Background())
+		err := metricsPusher.Stop(context.Background())
+		if err != nil {
+			log.Printf("an error while calling metrics pusher stop: %v", err)
+		}
 	}
 }
 

instrumentation/opentelemetry/metrics.go

@@ -0,0 +1,57 @@
+package opentelemetry // import "github.com/hypertrace/goagent/instrumentation/opentelemetry"
+
+import (
+	"net/http"
+
+	"github.com/hypertrace/goagent/sdk"
+	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
+	"go.opentelemetry.io/otel"
+	"go.opentelemetry.io/otel/metric"
+	"go.opentelemetry.io/otel/metric/global"
+	"go.opentelemetry.io/otel/metric/instrument/syncint64"
+	semconv "go.opentelemetry.io/otel/semconv/v1.12.0"
+)
+
+// Server HTTP metrics.
+const (
+	// Pseudo of go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp#RequestCount since a metric is not
+	// created for that one for some reason.(annotated with hypertrace to avoid a duplicate if otel go ever implement
+	// their own)
+	RequestCount = "hypertrace.http.server.request_count" // Incoming request count total
+)
+
+type HttpOperationMetricsHandler struct {
+	operationNameGetter func(*http.Request) string
+	counters            map[string]syncint64.Counter
+}
+
+var _ sdk.HttpOperationMetricsHandler = (*HttpOperationMetricsHandler)(nil)
+
+func NewHttpOperationMetricsHandler(nameGetter func(*http.Request) string) sdk.HttpOperationMetricsHandler {
+	return &HttpOperationMetricsHandler{
+		operationNameGetter: nameGetter,
+		counters:            make(map[string]syncint64.Counter, 1),
+	}
+}
+
+func (mh *HttpOperationMetricsHandler) CreateRequestCount() {
+	mp := global.MeterProvider()
+	meter := mp.Meter("go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",
+		metric.WithInstrumentationVersion(otelhttp.SemVersion()))
+
+	requestCountCounter, err := meter.SyncInt64().Counter(RequestCount)
+	if err != nil {
+		otel.Handle(err)
+	}
+
+	mh.counters[RequestCount] = requestCountCounter
+}
+
+func (mh *HttpOperationMetricsHandler) AddToRequestCount(n int64, r *http.Request) {
+	// Add metrics using the same logic in go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp#handler.go
+	ctx := r.Context()
+	labeler, _ := otelhttp.LabelerFromContext(ctx)
+	operationName := mh.operationNameGetter(r)
+	attributes := append(labeler.Get(), semconv.HTTPServerMetricAttributesFromHTTPRequest(operationName, r)...)
+	mh.counters[RequestCount].Add(ctx, n, attributes...)
+}

instrumentation/opentelemetry/net/hyperhttp/handler.go

@@ -10,6 +10,6 @@ import (
 // WrapHandler returns a new round tripper instrumented that relies on the
 // needs to be used with OTel instrumentation.
 func WrapHandler(delegate http.Handler, options *sdkhttp.Options) http.Handler {
-	// TODO: Find another way to get the operation name
-	return sdkhttp.WrapHandler(delegate, "", opentelemetry.SpanFromContext, options, map[string]string{})
+	mh := opentelemetry.NewHttpOperationMetricsHandler(func(_ *http.Request) string { return "" })
+	return sdkhttp.WrapHandler(delegate, opentelemetry.SpanFromContext, options, map[string]string{}, mh)
 }

sdk/instrumentation/net/http/handler.go

@@ -12,31 +12,15 @@ import (
 	"github.com/hypertrace/goagent/sdk/filter"
 	internalconfig "github.com/hypertrace/goagent/sdk/internal/config"
 	"github.com/hypertrace/goagent/sdk/internal/container"
-	"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
-	"go.opentelemetry.io/otel"
-	"go.opentelemetry.io/otel/metric"
-	"go.opentelemetry.io/otel/metric/global"
-	"go.opentelemetry.io/otel/metric/instrument/syncint64"
-	semconv "go.opentelemetry.io/otel/semconv/v1.12.0"
-)
-
-// Server HTTP metrics.
-const (
-	// Pseudo of go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp#RequestCount since a metric is not
-	// created for that one for some reason.(annotated with hypertrace to avoid a duplicate if otel go ever implement
-	// their own)
-	RequestCount = "hypertrace.http.server.request_count" // Incoming request count total
 )
 
 type handler struct {
 	delegate                 http.Handler
-	operation                string
 	defaultAttributes        map[string]string
 	spanFromContextRetriever sdk.SpanFromContext
 	dataCaptureConfig        *config.DataCapture
 	filter                   filter.Filter
-	// Some metrics in here.
-	counters map[string]syncint64.Counter
+	mh                       sdk.HttpOperationMetricsHandler
 }
 
 // Options for HTTP handler instrumentation
@@ -46,7 +30,8 @@ type Options struct {
 
 // WrapHandler wraps an uninstrumented handler (e.g. a handleFunc) and returns a new one
 // that should be used as base to an instrumented handler
-func WrapHandler(delegate http.Handler, operation string, spanFromContext sdk.SpanFromContext, options *Options, spanAttributes map[string]string) http.Handler {
+func WrapHandler(delegate http.Handler, spanFromContext sdk.SpanFromContext, options *Options, spanAttributes map[string]string,
+	mh sdk.HttpOperationMetricsHandler) http.Handler {
 	defaultAttributes := make(map[string]string)
 	for k, v := range spanAttributes {
 		defaultAttributes[k] = v
@@ -59,29 +44,17 @@ func WrapHandler(delegate http.Handler, operation string, spanFromContext sdk.Sp
 		f = options.Filter
 	}
 
-	mp := global.MeterProvider()
-	meter := mp.Meter("go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp",
-		metric.WithInstrumentationVersion(otelhttp.SemVersion()))
-	counters := make(map[string]syncint64.Counter)
-
-	requestCountCounter, err := meter.SyncInt64().Counter(RequestCount)
-	if err != nil {
-		otel.Handle(err)
-	}
-
-	counters[RequestCount] = requestCountCounter
+	// Create request count metric
+	mh.CreateRequestCount()
 
-	return &handler{delegate, operation, defaultAttributes, spanFromContext, internalconfig.GetConfig().GetDataCapture(), f, counters}
+	return &handler{delegate, defaultAttributes, spanFromContext, internalconfig.GetConfig().GetDataCapture(), f, mh}
 }
 
 func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	// Add metrics using the same logic in go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp#handler.go
 	ctx := r.Context()
-	labeler, _ := otelhttp.LabelerFromContext(ctx)
-	attributes := append(labeler.Get(), semconv.HTTPServerMetricAttributesFromHTTPRequest(h.operation, r)...)
-	h.counters[RequestCount].Add(ctx, 1, attributes...)
+	span := h.spanFromContextRetriever(ctx)
 
-	span := h.spanFromContextRetriever(r.Context())
+	h.mh.AddToRequestCount(1, r)
 
 	if span.IsNoop() {
 		// isNoop means either the span is not sampled or there was no span

sdk/instrumentation/net/http/handler_test.go

@@ -16,7 +16,16 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-const fooOpName string = "/foo"
+// A no-op metrics handler
+type metricsHandler struct {
+}
+
+func (mh *metricsHandler) CreateRequestCount() {
+
+}
+func (mh *metricsHandler) AddToRequestCount(int64, *http.Request) {
+
+}
 
 var emptyTestConfig = &config.DataCapture{
 	HttpHeaders: &config.Message{
@@ -52,7 +61,7 @@ func TestServerRequestWithNilBodyIsntChanged(t *testing.T) {
 		assert.Nil(t, r.Body)
 	})
 
-	wh, _ := WrapHandler(h, fooOpName, mock.SpanFromContext, &Options{}, map[string]string{}).(*handler)
+	wh, _ := WrapHandler(h, mock.SpanFromContext, &Options{}, map[string]string{}, &metricsHandler{}).(*handler)
 	wh.dataCaptureConfig = emptyTestConfig
 
 	ih := &mockHandler{baseHandler: wh}
@@ -76,7 +85,7 @@ func TestServerRequestIsSuccessfullyTraced(t *testing.T) {
 		rw.Write([]byte("ponse_body"))
 	})
 
-	wh, _ := WrapHandler(h, fooOpName, mock.SpanFromContext, &Options{}, map[string]string{"foo": "bar"}).(*handler)
+	wh, _ := WrapHandler(h, mock.SpanFromContext, &Options{}, map[string]string{"foo": "bar"}, &metricsHandler{}).(*handler)
 	wh.dataCaptureConfig = emptyTestConfig
 	ih := &mockHandler{baseHandler: wh}
 
@@ -105,7 +114,7 @@ func TestHostIsSuccessfullyRecorded(t *testing.T) {
 		assert.Nil(t, r.Body)
 	})
 
-	wh, _ := WrapHandler(h, fooOpName, mock.SpanFromContext, &Options{}, map[string]string{}).(*handler)
+	wh, _ := WrapHandler(h, mock.SpanFromContext, &Options{}, map[string]string{}, &metricsHandler{}).(*handler)
 	wh.dataCaptureConfig = emptyTestConfig
 
 	ih := &mockHandler{baseHandler: wh}
@@ -141,7 +150,7 @@ func TestServerRequestHeadersAreSuccessfullyRecorded(t *testing.T) {
 			rw.WriteHeader(202)
 		})
 
-		wh, _ := WrapHandler(h, fooOpName, mock.SpanFromContext, &Options{}, map[string]string{}).(*handler)
+		wh, _ := WrapHandler(h, mock.SpanFromContext, &Options{}, map[string]string{}, &metricsHandler{}).(*handler)
 		ih := &mockHandler{baseHandler: wh}
 		wh.dataCaptureConfig = emptyTestConfig
 		wh.dataCaptureConfig.HttpHeaders = &config.Message{
@@ -287,7 +296,7 @@ func TestServerRecordsRequestAndResponseBodyAccordingly(t *testing.T) {
 				rw.Write([]byte(tCase.responseBody))
 			})
 
-			wh, _ := WrapHandler(h, fooOpName, mock.SpanFromContext, &Options{}, map[string]string{}).(*handler)
+			wh, _ := WrapHandler(h, mock.SpanFromContext, &Options{}, map[string]string{}, &metricsHandler{}).(*handler)
 			wh.dataCaptureConfig = emptyTestConfig
 			wh.dataCaptureConfig.HttpBody = &config.Message{
 				Request:  config.Bool(tCase.captureHTTPBodyConfig),
@@ -465,7 +474,7 @@ func TestServerRequestFilter(t *testing.T) {
 				rw.WriteHeader(http.StatusOK)
 			})
 
-			wh, _ := WrapHandler(h, fooOpName, mock.SpanFromContext, tCase.options, map[string]string{}).(*handler)
+			wh, _ := WrapHandler(h, mock.SpanFromContext, tCase.options, map[string]string{}, &metricsHandler{}).(*handler)
 			ih := &mockHandler{baseHandler: wh}
 			r, _ := http.NewRequest("POST", tCase.url, strings.NewReader(tCase.body))
 			for i := 0; i < len(tCase.headerKeys); i++ {
@@ -493,14 +502,14 @@ func TestProcessingBodyIsTrimmed(t *testing.T) {
 
 	h := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {})
 
-	wh, _ := WrapHandler(h, fooOpName, mock.SpanFromContext, &Options{
+	wh, _ := WrapHandler(h, mock.SpanFromContext, &Options{
 		Filter: mock.Filter{
 			BodyEvaluator: func(span sdk.Span, body []byte, headers map[string][]string) result.FilterResult {
 				assert.Equal(t, "{", string(body)) // body is truncated
 				return result.FilterResult{Block: true, ResponseStatusCode: 403}
 			},
 		},
-	}, map[string]string{}).(*handler)
+	}, map[string]string{}, &metricsHandler{}).(*handler)
 	wh.dataCaptureConfig = emptyTestConfig
 	wh.dataCaptureConfig.HttpBody.Request = config.Bool(true)
 	wh.dataCaptureConfig.BodyMaxProcessingSizeBytes = config.Int32(int32(bodyMaxProcessingSizeBytes))

sdk/metrics.go

@@ -0,0 +1,10 @@
+package sdk // import "github.com/hypertrace/goagent/sdk"
+
+import (
+	"net/http"
+)
+
+type HttpOperationMetricsHandler interface {
+	CreateRequestCount()
+	AddToRequestCount(int64, *http.Request)
+}