From 1b2b8e5de997f3bb22c0a89eb2ea4a80955a6941 Mon Sep 17 00:00:00 2001 From: Tim Mwangi Date: Mon, 30 Jan 2023 11:48:28 -0800 Subject: [PATCH 1/4] feat: refactor http operation metrics to handle multiple frameworks This is to be able to send the correct operation name from the various frameworks including gorilla mux. Also, remove the otel metrics specific code from the sdk. --- instrumentation/hypertrace/metrics.go | 7 ++ .../hypertrace/net/hyperhttp/handler.go | 4 +- instrumentation/opencensus/metrics.go | 20 ++++++ .../opencensus/net/hyperhttp/handler.go | 4 +- .../github.com/gin-gonic/hypergin/gin.go | 4 +- .../github.com/gorilla/hypermux/mux.go | 23 +++++- instrumentation/opentelemetry/init.go | 6 +- instrumentation/opentelemetry/metrics.go | 70 +++++++++++++++++++ .../opentelemetry/net/hyperhttp/handler.go | 4 +- sdk/instrumentation/net/http/handler.go | 60 ++++++++-------- sdk/instrumentation/net/http/handler_test.go | 27 ++++--- sdk/metrics.go | 10 +++ 12 files changed, 190 insertions(+), 49 deletions(-) create mode 100644 instrumentation/hypertrace/metrics.go create mode 100644 instrumentation/opencensus/metrics.go create mode 100644 instrumentation/opentelemetry/metrics.go create mode 100644 sdk/metrics.go diff --git a/instrumentation/hypertrace/metrics.go b/instrumentation/hypertrace/metrics.go new file mode 100644 index 0000000..1e27821 --- /dev/null +++ b/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 diff --git a/instrumentation/hypertrace/net/hyperhttp/handler.go b/instrumentation/hypertrace/net/hyperhttp/handler.go index 2b95a4f..734fd94 100644 --- a/instrumentation/hypertrace/net/hyperhttp/handler.go +++ b/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, ) } diff --git a/instrumentation/opencensus/metrics.go b/instrumentation/opencensus/metrics.go new file mode 100644 index 0000000..9ea5318 --- /dev/null +++ b/instrumentation/opencensus/metrics.go @@ -0,0 +1,20 @@ +package opencensus // import "github.com/hypertrace/goagent/instrumentation/opencensus" + +import ( + "net/http" + + "github.com/hypertrace/goagent/sdk" +) + +type httpOperationMetricsHandler struct { +} + +func NewHttpOperationMetricsHandler() sdk.HttpOperationMetricsHandler { + return &httpOperationMetricsHandler{} +} + +func (mh *httpOperationMetricsHandler) CreateRequestCount() { +} + +func (mh *httpOperationMetricsHandler) AddToRequestCount(n int64, r *http.Request) { +} diff --git a/instrumentation/opencensus/net/hyperhttp/handler.go b/instrumentation/opencensus/net/hyperhttp/handler.go index fb7428e..474364a 100644 --- a/instrumentation/opencensus/net/hyperhttp/handler.go +++ b/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()) } diff --git a/instrumentation/opentelemetry/github.com/gin-gonic/hypergin/gin.go b/instrumentation/opentelemetry/github.com/gin-gonic/hypergin/gin.go index 438cf65..2a61a7c 100644 --- a/instrumentation/opentelemetry/github.com/gin-gonic/hypergin/gin.go +++ b/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), ) diff --git a/instrumentation/opentelemetry/github.com/gorilla/hypermux/mux.go b/instrumentation/opentelemetry/github.com/gorilla/hypermux/mux.go index e27c772..a79bca4 100644 --- a/instrumentation/opentelemetry/github.com/gorilla/hypermux/mux.go +++ b/instrumentation/opentelemetry/github.com/gorilla/hypermux/mux.go @@ -28,12 +28,31 @@ func spanNameFormatter(operation string, r *http.Request) (spanName string) { return } +func getOperationNameFromRoute(r *http.Request) string { + route := mux.CurrentRoute(r) + spanName := "" + if route != nil { + var err error + spanName, err = route.GetPathTemplate() + if err != nil { + spanName, _ = route.GetPathRegexp() + } + } + + if spanName == "" { + // if somehow retrieving the path template or path regexp fails, we still + // want to use the method as fallback. + spanName = r.Method + } + 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), ) diff --git a/instrumentation/opentelemetry/init.go b/instrumentation/opentelemetry/init.go index eeddebd..aa2f100 100644 --- a/instrumentation/opentelemetry/init.go +++ b/instrumentation/opentelemetry/init.go @@ -245,6 +245,9 @@ func InitWithSpanProcessorWrapper(cfg *config.AgentConfig, wrapper SpanProcessor } } + // Initialize metrics + metricsShutdownFn := initializeMetrics(cfg) + exporterFactory = makeExporterFactory(cfg) exporter, err := exporterFactory() @@ -275,9 +278,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 diff --git a/instrumentation/opentelemetry/metrics.go b/instrumentation/opentelemetry/metrics.go new file mode 100644 index 0000000..696160b --- /dev/null +++ b/instrumentation/opentelemetry/metrics.go @@ -0,0 +1,70 @@ +package opentelemetry // import "github.com/hypertrace/goagent/instrumentation/opentelemetry" + +import ( + // "bytes" + // "encoding/base64" + // "io" + // "io/ioutil" + //"context" + "net/http" + + // config "github.com/hypertrace/agent-config/gen/go/v1" + "github.com/hypertrace/goagent/sdk" + // "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/attribute" + "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 { + // operationName string + operationNameGetter func(*http.Request) string + // Some metrics in here. + counters map[string]syncint64.Counter +} + +var _ sdk.HttpOperationMetricsHandler = (*HttpOperationMetricsHandler)(nil) + +// TODO: modify to return interface +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())) + // counters := make(map[string]syncint64.Counter) + + 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) { + 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...) +} diff --git a/instrumentation/opentelemetry/net/hyperhttp/handler.go b/instrumentation/opentelemetry/net/hyperhttp/handler.go index c241add..afce2b9 100644 --- a/instrumentation/opentelemetry/net/hyperhttp/handler.go +++ b/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) } diff --git a/sdk/instrumentation/net/http/handler.go b/sdk/instrumentation/net/http/handler.go index d449a9a..5cf4eef 100644 --- a/sdk/instrumentation/net/http/handler.go +++ b/sdk/instrumentation/net/http/handler.go @@ -12,31 +12,31 @@ 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" + // "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 -) +// 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 + mh sdk.HttpOperationMetricsHandler // Some metrics in here. - counters map[string]syncint64.Counter + // counters map[string]syncint64.Counter } // Options for HTTP handler instrumentation @@ -46,7 +46,7 @@ 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 +59,31 @@ 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) + // 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) - } + // requestCountCounter, err := meter.SyncInt64().Counter(RequestCount) + // if err != nil { + // otel.Handle(err) + // } - counters[RequestCount] = requestCountCounter + // counters[RequestCount] = requestCountCounter + 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()) + // labeler, _ := otelhttp.LabelerFromContext(ctx) + // attributes := append(labeler.Get(), semconv.HTTPServerMetricAttributesFromHTTPRequest(span.Name(), r)...) + // h.counters[RequestCount].Add(ctx, 1, attributes...) + h.mh.AddToRequestCount(1, r) if span.IsNoop() { // isNoop means either the span is not sampled or there was no span diff --git a/sdk/instrumentation/net/http/handler_test.go b/sdk/instrumentation/net/http/handler_test.go index 5ea3fda..4085ddc 100644 --- a/sdk/instrumentation/net/http/handler_test.go +++ b/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)) diff --git a/sdk/metrics.go b/sdk/metrics.go new file mode 100644 index 0000000..5d73737 --- /dev/null +++ b/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) +} From e3a0af0776c4bdd35fe5832d260ca091a2161098 Mon Sep 17 00:00:00 2001 From: Tim Mwangi Date: Mon, 30 Jan 2023 13:12:34 -0800 Subject: [PATCH 2/4] cleanup --- instrumentation/opencensus/metrics.go | 1 + .../github.com/gorilla/hypermux/mux.go | 19 ++--------- instrumentation/opentelemetry/metrics.go | 17 ++-------- sdk/instrumentation/net/http/handler.go | 32 +------------------ 4 files changed, 6 insertions(+), 63 deletions(-) diff --git a/instrumentation/opencensus/metrics.go b/instrumentation/opencensus/metrics.go index 9ea5318..fa1c7e9 100644 --- a/instrumentation/opencensus/metrics.go +++ b/instrumentation/opencensus/metrics.go @@ -6,6 +6,7 @@ import ( "github.com/hypertrace/goagent/sdk" ) +// Will not support metrics for OC instrumentation type httpOperationMetricsHandler struct { } diff --git a/instrumentation/opentelemetry/github.com/gorilla/hypermux/mux.go b/instrumentation/opentelemetry/github.com/gorilla/hypermux/mux.go index a79bca4..0e2dd02 100644 --- a/instrumentation/opentelemetry/github.com/gorilla/hypermux/mux.go +++ b/instrumentation/opentelemetry/github.com/gorilla/hypermux/mux.go @@ -9,23 +9,8 @@ import ( "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" ) -func spanNameFormatter(operation string, r *http.Request) (spanName string) { - route := mux.CurrentRoute(r) - if route != nil { - var err error - spanName, err = route.GetPathTemplate() - if err != nil { - spanName, _ = route.GetPathRegexp() - } - } - - if spanName == "" { - // if somehow retrieving the path template or path regexp fails, we still - // want to use the method as fallback. - spanName = r.Method - } - - return +func spanNameFormatter(operation string, r *http.Request) string { + return getOperationNameFromRoute(r) } func getOperationNameFromRoute(r *http.Request) string { diff --git a/instrumentation/opentelemetry/metrics.go b/instrumentation/opentelemetry/metrics.go index 696160b..6f81d50 100644 --- a/instrumentation/opentelemetry/metrics.go +++ b/instrumentation/opentelemetry/metrics.go @@ -1,21 +1,11 @@ package opentelemetry // import "github.com/hypertrace/goagent/instrumentation/opentelemetry" import ( - // "bytes" - // "encoding/base64" - // "io" - // "io/ioutil" - //"context" "net/http" - // config "github.com/hypertrace/agent-config/gen/go/v1" "github.com/hypertrace/goagent/sdk" - // "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/attribute" "go.opentelemetry.io/otel/metric" "go.opentelemetry.io/otel/metric/global" "go.opentelemetry.io/otel/metric/instrument/syncint64" @@ -31,15 +21,12 @@ const ( ) type HttpOperationMetricsHandler struct { - // operationName string operationNameGetter func(*http.Request) string - // Some metrics in here. - counters map[string]syncint64.Counter + counters map[string]syncint64.Counter } var _ sdk.HttpOperationMetricsHandler = (*HttpOperationMetricsHandler)(nil) -// TODO: modify to return interface func NewHttpOperationMetricsHandler(nameGetter func(*http.Request) string) sdk.HttpOperationMetricsHandler { return &HttpOperationMetricsHandler{ operationNameGetter: nameGetter, @@ -51,7 +38,6 @@ func (mh *HttpOperationMetricsHandler) CreateRequestCount() { 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 { @@ -62,6 +48,7 @@ func (mh *HttpOperationMetricsHandler) CreateRequestCount() { } 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) diff --git a/sdk/instrumentation/net/http/handler.go b/sdk/instrumentation/net/http/handler.go index 5cf4eef..1de2ae6 100644 --- a/sdk/instrumentation/net/http/handler.go +++ b/sdk/instrumentation/net/http/handler.go @@ -12,22 +12,8 @@ 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 defaultAttributes map[string]string @@ -35,8 +21,6 @@ type handler struct { dataCaptureConfig *config.DataCapture filter filter.Filter mh sdk.HttpOperationMetricsHandler - // Some metrics in here. - // counters map[string]syncint64.Counter } // Options for HTTP handler instrumentation @@ -59,30 +43,16 @@ func WrapHandler(delegate http.Handler, spanFromContext sdk.SpanFromContext, opt 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, 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() span := h.spanFromContextRetriever(ctx) - // labeler, _ := otelhttp.LabelerFromContext(ctx) - // attributes := append(labeler.Get(), semconv.HTTPServerMetricAttributesFromHTTPRequest(span.Name(), r)...) - // h.counters[RequestCount].Add(ctx, 1, attributes...) h.mh.AddToRequestCount(1, r) if span.IsNoop() { From f132a7a64526537191f9d3997e8a46b007c3c633 Mon Sep 17 00:00:00 2001 From: Tim Mwangi Date: Mon, 30 Jan 2023 13:16:03 -0800 Subject: [PATCH 3/4] fix lint --- sdk/instrumentation/net/http/handler.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sdk/instrumentation/net/http/handler.go b/sdk/instrumentation/net/http/handler.go index 1de2ae6..997feb6 100644 --- a/sdk/instrumentation/net/http/handler.go +++ b/sdk/instrumentation/net/http/handler.go @@ -30,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, spanFromContext sdk.SpanFromContext, options *Options, spanAttributes map[string]string, mh sdk.HttpOperationMetricsHandler) 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 From e66a39a3c6a2eeb9a39edae49cbe3ec50c92a0bc Mon Sep 17 00:00:00 2001 From: Tim Mwangi Date: Tue, 31 Jan 2023 07:59:14 -0800 Subject: [PATCH 4/4] fix unhandled error --- instrumentation/opentelemetry/init.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/instrumentation/opentelemetry/init.go b/instrumentation/opentelemetry/init.go index 6c92c9b..2c4252d 100644 --- a/instrumentation/opentelemetry/init.go +++ b/instrumentation/opentelemetry/init.go @@ -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) + } } }