Skip to content

Commit

Permalink
Fix route handling on OTEL httpmetrics (#199)
Browse files Browse the repository at this point in the history
## Rationale 

The route field wasn't resolved properly. Here we consider the entire list of used routes, similarly how it's done with `New`.

## Changes
* Consider all route patterns to set the route attribute
* Add more tests

## Meta
[W-13781478](https://gus.lightning.force.com/a07EE00001WSFXiYAP)
  • Loading branch information
ypaq authored Jul 20, 2023
1 parent 8431ac4 commit 8918beb
Show file tree
Hide file tree
Showing 3 changed files with 168 additions and 91 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package httpmetrics
import (
"net/http"
"strconv"
"strings"
"time"

"github.com/heroku/x/go-kit/metrics"
Expand Down Expand Up @@ -51,8 +52,7 @@ func NewOTEL(p metrics.Provider) func(http.Handler) http.Handler {
if ctx.Value(chi.RouteCtxKey) != nil {
rtCtx := chi.RouteContext(ctx)
if len(rtCtx.RoutePatterns) > 0 {
// pick last route pattern as it is the one chi used
route := rtCtx.RoutePatterns[len(rtCtx.RoutePatterns)-1]
route := getRouteAsString(rtCtx.RoutePatterns)
kv := []string{routeKey, route}
labels = append(labels, kv...)
}
Expand All @@ -70,3 +70,11 @@ func NewOTEL(p metrics.Provider) func(http.Handler) http.Handler {
})
}
}

func getRouteAsString(patterns []string) string {
var result string
for _, pattern := range patterns {
result += pattern
}
return strings.ReplaceAll(result, "/*/", "/")
}
158 changes: 158 additions & 0 deletions hmiddleware/httpmetrics/httpmetrics_otel_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
package httpmetrics

import (
"context"
"fmt"
"io"
"net/http"
"net/http/httptest"
"testing"

"github.com/go-chi/chi"

"github.com/heroku/x/go-kit/metrics/testmetrics"
)

func TestOTELMiddleware(t *testing.T) {
p := testmetrics.NewProvider(t)

next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
})

r := httptest.NewRequest("GET", "http://example.org/foo/bar", nil)
w := httptest.NewRecorder()

hand := NewOTEL(p)(next)
hand.ServeHTTP(w, r)

p.CheckObservationCount("http.server.duration.http.request.method:GET:url.scheme:http:server.address:example.org", 1)
}

func TestOTELResponseStatus(t *testing.T) {
p := testmetrics.NewProvider(t)

next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(502)
})

r := httptest.NewRequest("GET", "http://example.org/foo/bar", nil)
w := httptest.NewRecorder()

hand := NewOTEL(p)(next)
hand.ServeHTTP(w, r)

p.CheckObservationCount("http.server.duration.http.request.method:GET:http.response.status_code:502:url.scheme:http:server.address:example.org", 1)
}

func TestOTELChi(t *testing.T) {
p := testmetrics.NewProvider(t)

next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
})

r := httptest.NewRequest("GET", "http://example.org/foo/bar", nil)

rctx := chi.NewRouteContext()
rctx.RoutePatterns = []string{"/*", "/apps/{foo_id}/bars/{bar_id}"}
r = r.WithContext(context.WithValue(r.Context(), chi.RouteCtxKey, rctx))

w := httptest.NewRecorder()

hand := NewOTEL(p)(next)
hand.ServeHTTP(w, r)

p.CheckObservationCount("http.server.duration.http.request.method:GET:http.route:/apps/{foo_id}/bars/{bar_id}:url.scheme:http:server.address:example.org", 1)

}

func TestOTELNestedChiRouters(tt *testing.T) {

cases := []struct {
name string
url string
outerRoute string
innerRouter func(t *testing.T) *chi.Mux
observation string
}{
{
name: "inner outer",
url: "http://example.org/hello/world",
outerRoute: "/",
innerRouter: func(t *testing.T) *chi.Mux {
r := chi.NewRouter()
r.Get("/hello/{name}", func(w http.ResponseWriter, r *http.Request) {
name := chi.URLParam(r, "id")
if _, err := io.WriteString(w, fmt.Sprintf("Hello %s!", name)); err != nil {
t.Fatal("unexpected error", err)
}
})
return r
},
observation: "http.server.duration.http.request.method:GET:http.response.status_code:200:http.route:/hello/{name}:url.scheme:http:server.address:example.org",
},
{
name: "outer inner",
url: "http://example.org/hello/world",
outerRoute: "/hello/{name}",
innerRouter: func(t *testing.T) *chi.Mux {
r := chi.NewRouter()
r.Get("/", func(w http.ResponseWriter, r *http.Request) {
name := chi.URLParam(r, "id")
if _, err := io.WriteString(w, fmt.Sprintf("Hello %s!", name)); err != nil {
t.Fatal("unexpected error", err)
}
})
return r
},
observation: "http.server.duration.http.request.method:GET:http.response.status_code:200:http.route:/hello/{name}/:url.scheme:http:server.address:example.org",
},
{
name: "inner outer star",
url: "http://example.org/hello/world/1",
outerRoute: "/",
innerRouter: func(t *testing.T) *chi.Mux {
r := chi.NewRouter()
r.Get("/hello/{name}/*", func(w http.ResponseWriter, r *http.Request) {
name := chi.URLParam(r, "id")
if _, err := io.WriteString(w, fmt.Sprintf("Hello %s!", name)); err != nil {
t.Fatal("unexpected error", err)
}
})
return r
},
observation: "http.server.duration.http.request.method:GET:http.response.status_code:200:http.route:/hello/{name}/*:url.scheme:http:server.address:example.org",
},
{
name: "slash slash",
url: "http://example.org/",
outerRoute: "/",
innerRouter: func(t *testing.T) *chi.Mux {
r := chi.NewRouter()
r.Get("/", func(w http.ResponseWriter, r *http.Request) {
name := chi.URLParam(r, "id")
if _, err := io.WriteString(w, fmt.Sprintf("Hello %s!", name)); err != nil {
t.Fatal("unexpected error", err)
}
})
return r
},
observation: "http.server.duration.http.request.method:GET:http.response.status_code:200:http.route:/:url.scheme:http:server.address:example.org",
},
}

for _, test := range cases {
test := test
tt.Run(test.name, func(t *testing.T) {
p := testmetrics.NewProvider(t)
outer := chi.NewRouter()
outer.Use(NewOTEL(p))
outer.Mount(test.outerRoute, test.innerRouter(t))

r := httptest.NewRequest("GET", test.url, nil)
w := httptest.NewRecorder()
outer.ServeHTTP(w, r)

p.CheckObservationCount(test.observation, 1)
})
}
}
89 changes: 0 additions & 89 deletions hmiddleware/httpmetrics/httpmetrics_v2_test.go

This file was deleted.

0 comments on commit 8918beb

Please sign in to comment.