Skip to content

Commit 15f8967

Browse files
deansheatherdeansheather
authored
feat: tracing improvements (coder#4988)
1 parent d402914 commit 15f8967

File tree

10 files changed

+342
-34
lines changed

10 files changed

+342
-34
lines changed

‎coderd/coderd.go

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -204,8 +204,10 @@ func New(options *Options) *API {
204204
})
205205

206206
r.Use(
207-
httpmw.AttachRequestID,
208207
httpmw.Recover(api.Logger),
208+
tracing.StatusWriterMiddleware,
209+
tracing.Middleware(api.TracerProvider),
210+
httpmw.AttachRequestID,
209211
httpmw.ExtractRealIP(api.RealIPConfig),
210212
httpmw.Logger(api.Logger),
211213
httpmw.Prometheus(options.PrometheusRegistry),
@@ -239,7 +241,6 @@ func New(options *Options) *API {
239241

240242
apps := func(r chi.Router) {
241243
r.Use(
242-
tracing.Middleware(api.TracerProvider),
243244
httpmw.RateLimit(options.APIRateLimit, time.Minute),
244245
httpmw.ExtractAPIKey(httpmw.ExtractAPIKeyConfig{
245246
DB: options.Database,
@@ -287,7 +288,6 @@ func New(options *Options) *API {
287288

288289
r.NotFound(func(rw http.ResponseWriter, r *http.Request) { httpapi.RouteNotFound(rw) })
289290
r.Use(
290-
tracing.Middleware(api.TracerProvider),
291291
// Specific routes can specify smaller limits.
292292
httpmw.RateLimit(options.APIRateLimit, time.Minute),
293293
)
@@ -508,14 +508,6 @@ func New(options *Options) *API {
508508
r.Get("/listening-ports", api.workspaceAgentListeningPorts)
509509
r.Get("/connection", api.workspaceAgentConnection)
510510
r.Get("/coordinate", api.workspaceAgentClientCoordinate)
511-
// TODO: This can be removed in October. It allows for a friendly
512-
// error message when transitioning from WebRTC to Tailscale. See:
513-
// https://github.com/coder/coder/issues/4126
514-
r.Get("/dial", func(w http.ResponseWriter, r *http.Request) {
515-
httpapi.Write(r.Context(), w, http.StatusGone, codersdk.Response{
516-
Message: "Your Coder CLI is out of date, and requires v0.8.15+ to connect!",
517-
})
518-
})
519511
})
520512
})
521513
r.Route("/workspaces", func(r chi.Router) {

‎coderd/coderdtest/authorize.go

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,8 +55,6 @@ func AGPLRoutes(a *AuthTester) (map[string]string, map[string]RouteCheck) {
5555
"POST:/api/v2/csp/reports": {NoAuthorize: true},
5656
"POST:/api/v2/authcheck": {NoAuthorize: true},
5757
"GET:/api/v2/applications/host": {NoAuthorize: true},
58-
// This is a dummy endpoint for compatibility with older CLI versions.
59-
"GET:/api/v2/workspaceagents/{workspaceagent}/dial": {NoAuthorize: true},
6058

6159
// Has it's own auth
6260
"GET:/api/v2/users/oauth2/github/callback": {NoAuthorize: true},

‎coderd/httpmw/logger.go

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package httpmw
22

33
import (
4+
"context"
5+
"fmt"
46
"net/http"
57
"time"
68

@@ -11,9 +13,13 @@ import (
1113

1214
func Logger(log slog.Logger) func(next http.Handler) http.Handler {
1315
return func(next http.Handler) http.Handler {
14-
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
16+
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
1517
start := time.Now()
16-
sw := &tracing.StatusWriter{ResponseWriter: w}
18+
19+
sw, ok := rw.(*tracing.StatusWriter)
20+
if !ok {
21+
panic(fmt.Sprintf("ResponseWriter not a *tracing.StatusWriter; got %T", rw))
22+
}
1723

1824
httplog := log.With(
1925
slog.F("host", httpapi.RequestHost(r)),
@@ -51,7 +57,11 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler {
5157
logLevelFn = httplog.Warn
5258
}
5359

54-
logLevelFn(r.Context(), r.Method)
60+
// We already capture most of this information in the span (minus
61+
// the response body which we don't want to capture anyways).
62+
tracing.RunWithoutSpan(r.Context(), func(ctx context.Context) {
63+
logLevelFn(ctx, r.Method)
64+
})
5565
})
5666
}
5767
}

‎coderd/httpmw/patternmatcher/routepatterns.go

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
package patternmatcher
2+
3+
import (
4+
"fmt"
5+
"regexp"
6+
"strings"
7+
8+
"golang.org/x/xerrors"
9+
)
10+
11+
// RoutePatterns provides a method to generate a regex which will match a URL
12+
// path against a collection of patterns. If any of the patterns match the path,
13+
// the regex will return a successful match.
14+
//
15+
// Multiple patterns can be provided and they are matched in order. Example:
16+
// - /api/* matches /api/1 but not /api or /api/1/2
17+
// - /api/*/2 matches /api/1/2 but not /api/2 /api/1
18+
// - /api/** matches /api/1, /api/1/2, /api/1/2/3 but not /api
19+
// - /api/**/3 matches /api/1/2, /api/1/2/3 but not /api, /api/1 or /api/1/2
20+
//
21+
// All patterns support an optional trailing slash.
22+
type RoutePatterns []string
23+
24+
func (rp RoutePatterns) MustCompile() *regexp.Regexp {
25+
re, err := rp.Compile()
26+
if err != nil {
27+
panic(err)
28+
}
29+
return re
30+
}
31+
32+
func (rp RoutePatterns) Compile() (*regexp.Regexp, error) {
33+
patterns := make([]string, len(rp))
34+
for i, p := range rp {
35+
p = strings.ReplaceAll(p, "**", ".+")
36+
p = strings.ReplaceAll(p, "*", "[^/]+")
37+
if !strings.HasSuffix(p, "/") {
38+
p += "/?"
39+
}
40+
patterns[i] = p
41+
}
42+
43+
pattern := fmt.Sprintf("^(%s)$", strings.Join(patterns, "|"))
44+
re, err := regexp.Compile(pattern)
45+
if err != nil {
46+
return nil, xerrors.Errorf("compile regex %q: %w", pattern, err)
47+
}
48+
49+
return re, nil
50+
}

‎coderd/httpmw/patternmatcher/routepatterns_test.go

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
package patternmatcher_test
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/require"
7+
8+
"github.com/coder/coder/coderd/httpmw/patternmatcher"
9+
)
10+
11+
func Test_RoutePatterns(t *testing.T) {
12+
t.Parallel()
13+
14+
cases := []struct {
15+
name string
16+
patterns []string
17+
errContains string
18+
output string
19+
}{
20+
{
21+
name: "Empty",
22+
patterns: []string{},
23+
output: "^()$",
24+
},
25+
{
26+
name: "Single",
27+
patterns: []string{
28+
"/api",
29+
},
30+
output: "^(/api/?)$",
31+
},
32+
{
33+
name: "TrailingSlash",
34+
patterns: []string{
35+
"/api/",
36+
},
37+
output: "^(/api/)$",
38+
},
39+
{
40+
name: "Multiple",
41+
patterns: []string{
42+
"/api",
43+
"/api2",
44+
},
45+
output: "^(/api/?|/api2/?)$",
46+
},
47+
{
48+
name: "Star",
49+
patterns: []string{
50+
"/api/*",
51+
},
52+
output: "^(/api/[^/]+/?)$",
53+
},
54+
{
55+
name: "StarStar",
56+
patterns: []string{
57+
"/api/**",
58+
},
59+
output: "^(/api/.+/?)$",
60+
},
61+
{
62+
name: "TelemetryPatterns",
63+
patterns: []string{
64+
"/api",
65+
"/api/**",
66+
"/@*/*/apps/**",
67+
"/%40*/*/apps/**",
68+
"/gitauth/*/callback",
69+
},
70+
output: "^(/api/?|/api/.+/?|/@[^/]+/[^/]+/apps/.+/?|/%40[^/]+/[^/]+/apps/.+/?|/gitauth/[^/]+/callback/?)$",
71+
},
72+
{
73+
name: "Slash",
74+
patterns: []string{
75+
"/",
76+
},
77+
output: "^(/)$",
78+
},
79+
{
80+
name: "SlashStar",
81+
patterns: []string{
82+
"/*",
83+
},
84+
output: "^(/[^/]+/?)$",
85+
},
86+
{
87+
name: "SlashStarStar",
88+
patterns: []string{
89+
"/**",
90+
},
91+
output: "^(/.+/?)$",
92+
},
93+
{
94+
name: "SlashSlash",
95+
patterns: []string{
96+
"//",
97+
"/api//v1",
98+
},
99+
output: "^(//|/api//v1/?)$",
100+
},
101+
{
102+
name: "Invalid",
103+
patterns: []string{
104+
"/api(",
105+
},
106+
errContains: "compile regex",
107+
},
108+
}
109+
110+
for _, c := range cases {
111+
c := c
112+
t.Run(c.name, func(t *testing.T) {
113+
t.Parallel()
114+
115+
rp := patternmatcher.RoutePatterns(c.patterns)
116+
re, err := rp.Compile()
117+
if c.errContains != "" {
118+
require.Error(t, err)
119+
require.ErrorContains(t, err, c.errContains)
120+
121+
require.Panics(t, func() {
122+
_ = rp.MustCompile()
123+
})
124+
} else {
125+
require.NoError(t, err)
126+
require.Equal(t, c.output, re.String())
127+
128+
require.NotPanics(t, func() {
129+
re := rp.MustCompile()
130+
require.Equal(t, c.output, re.String())
131+
})
132+
}
133+
})
134+
}
135+
}

‎coderd/tracing/httpmw.go

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,17 +10,31 @@ import (
1010
"go.opentelemetry.io/otel/propagation"
1111
semconv "go.opentelemetry.io/otel/semconv/v1.11.0"
1212
"go.opentelemetry.io/otel/trace"
13+
14+
"github.com/coder/coder/coderd/httpmw/patternmatcher"
1315
)
1416

1517
// Middleware adds tracing to http routes.
1618
func Middleware(tracerProvider trace.TracerProvider) func(http.Handler) http.Handler {
19+
// We only want to create spans on the following route patterns, however
20+
// we want the middleware to be very high in the middleware stack so it can
21+
// capture the entire request.
22+
re := patternmatcher.RoutePatterns{
23+
"/api",
24+
"/api/**",
25+
"/@*/*/apps/**",
26+
"/%40*/*/apps/**",
27+
"/gitauth/*/callback",
28+
}.MustCompile()
29+
1730
var tracer trace.Tracer
1831
if tracerProvider != nil {
1932
tracer = tracerProvider.Tracer(TracerName)
2033
}
34+
2135
return func(next http.Handler) http.Handler {
2236
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
23-
if tracer == nil {
37+
if tracer == nil || !re.MatchString(r.URL.Path) {
2438
next.ServeHTTP(rw, r)
2539
return
2640
}

0 commit comments

Comments
 (0)