From 3ddc1a3a021244a377cd8b21981f13e161b18634 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 12 Oct 2026 23:16:12 +0000 Subject: [PATCH] Name Sentry transactions after the route they matched (closes #185) sentryhttp names each transaction after the concrete path, which the BeforeSendTransaction hook has to withhold, so with tracing on the transactions would fall into one Sentry group per method, such as POST /(redacted). A middleware installed beside every sentryhttp handler now renames the transaction to the method and chi route pattern once the request is routed, and sets its source to route. The hook keeps a route-sourced name and withholds every other name as before. A path that matches nothing at the top level keeps the withheld name; one that matches no route inside a route group, or that the group's middleware turns away, is named after the group's pattern, such as GET /pages/*. Tracing stays off. Model: opus-5-5 --- README.md | 30 ++++++++--- internal/server/export_test.go | 18 +++++++ internal/server/routes.go | 9 +++- internal/server/sentry.go | 61 +++++++++++++++++++-- internal/server/sentry_test.go | 97 +++++++++++++++++++++++++++++++--- 5 files changed, 194 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index 7a2a892..124fd68 100644 --- a/README.md +++ b/README.md @@ -2551,10 +2551,23 @@ Where the pattern is out of reach — the transaction dispatch, an event capture outside the router, or a request that matched no route — the fallback is never the concrete path. The path becomes the literal `/(redacted)`, so the URL reads `http://host/(redacted)`; a URL the rewrite cannot parse into a scheme is -withheld whole. A transaction event additionally carries the SDK's own -`METHOD /path` name, built from the concrete path as well; it is rewritten on -the same terms, to `POST /h/{uuid}` where the pattern is known and -`POST /(redacted)` where it is not. +withheld whole. + +A transaction event also carries a name, which the SDK builds as `METHOD /path` +from the concrete path and gives the transaction source `url`. Sentry groups +transactions by name, so withholding it would put every request in the service +into one group per method, such as `POST /(redacted)`. Inside each Sentry +handler, a second middleware renames the transaction once the request has been +routed, to the method and the route pattern (`POST /h/{uuid}`), and sets its +source to `route`. The hook keeps a name whose source is `route` and rewrites +any other to `METHOD /(redacted)`. A path that matches nothing at the top level +has no pattern, so its transaction keeps the SDK's name and the hook withholds +it. Inside a route group, a path that matches no route in the group, and a +request the group's own middleware turns away before routing (a CSRF refusal, +the redirect to `/pages/login`), are named after the group's pattern, such as +`GET /pages/*` or `POST /hook/{sourceID}/*`, and the hook keeps that name. The +URL on a transaction event still falls back to `http://host/(redacted)`, because +the hook has no request there. The headers are an allowlist for the same reason the rules above are unconditional: the SDK's own filter removes four names and passes everything @@ -3175,7 +3188,8 @@ Applied to all routes in this order: 7. **Recoverer** — Panic recovery: one `ERROR` record through `internal/logger` and a `500` 8. **Sentry** — Error reporting to Sentry (if `SENTRY_DSN` is set; configured - with `Repanic: true` so panics still reach Recoverer) + with `Repanic: true` so panics still reach Recoverer), and inside it the + naming of the request's Sentry transaction after its route pattern Recoverer sits seventh rather than first, and both neighbours are the reason. It runs **inside** everything that observes the response, so the `500` it writes @@ -3190,9 +3204,9 @@ set a header or start a timer. Each admin page route group (`/pages`, `/user/*`, `/settings`, `/hooks`, `/hook/*`) starts with its own **Recoverer** and, if `SENTRY_DSN` is set, its -own **Sentry** error reporting. That Recoverer answers a panic with the `500` -error page in the normal layout; the global one keeps the plain-text `500` for -every other route. +own **Sentry** error reporting and transaction naming. That Recoverer answers a +panic with the `500` error page in the normal layout; the global one keeps the +plain-text `500` for every other route. Additionally, form endpoints (`/pages`, `/user/*`, `/settings`, `/hooks`, `/hook/*`) apply a **MaxBodySize** middleware that limits POST/PUT/PATCH request diff --git a/internal/server/export_test.go b/internal/server/export_test.go index 957f4dd..3a3717e 100644 --- a/internal/server/export_test.go +++ b/internal/server/export_test.go @@ -91,6 +91,24 @@ func NewRouterForTest( return s.router } +// NewRouterWithSentryForTest is NewRouterForTest with the sentryhttp +// handler registered, as a set SENTRY_DSN does in production. +func NewRouterWithSentryForTest( + t *testing.T, + log *logger.Logger, + cfg *config.Config, + mw *middleware.Middleware, + h *handlers.Handlers, +) http.Handler { + t.Helper() + + s := newServerForTest(t, log, cfg, mw, h) + s.sentryEnabled.Store(true) + s.SetupRoutes() + + return s.router +} + // ListenAddrForTest exposes the address the HTTP listener binds for // a given Config, so the rendering of host and port — IPv6 // bracketing above all — can be pinned without standing up a diff --git a/internal/server/routes.go b/internal/server/routes.go index 40e7113..d7210ab 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -90,8 +90,8 @@ func (s *Server) setupGlobalMiddleware() { // recoverPanics installs on r the recoverer, answering a panic with // page (a plain 500 when page is nil), and inside it the Sentry error -// reporting (if SENTRY_DSN is set). Repanic is true so panics still -// bubble up to the recoverer. +// reporting and transaction naming (if SENTRY_DSN is set). Repanic is +// true so panics still bubble up to the recoverer. // // Each admin page route group installs its own, with the error page, // as its first middleware. A panic there is logged, reported and @@ -105,6 +105,11 @@ func (s *Server) recoverPanics(r chi.Router, page http.Handler) { Repanic: true, }) r.Use(sentryHandler.Handle) + // Installed with every sentryhttp handler rather than once + // globally: in an admin page route group, the group's own + // handler finishes the transaction as the group returns, so + // a rename further out would come too late. + r.Use(nameSentryTransaction) } } diff --git a/internal/server/sentry.go b/internal/server/sentry.go index 086c3a6..fdd2dbd 100644 --- a/internal/server/sentry.go +++ b/internal/server/sentry.go @@ -1,6 +1,7 @@ package server import ( + "context" "net/http" "net/url" "strings" @@ -71,10 +72,15 @@ func scrubSentryRequest( pattern := sentryRoutePattern(hint) - // Only transaction events carry a Transaction name, and the SDK + // Only transaction events carry a Transaction name. sentryhttp // builds it from the concrete path too (sentryhttp.go:105 via - // tracing.go:553). Rewritten on the same terms. - if event.Transaction != "" { + // tracing.go:553) and marks it SourceURL, so it is rewritten on + // the same terms. Only a name nameSentryTransaction set from the + // route pattern, which it marks SourceRoute, is kept. + namedByRoute := event.TransactionInfo != nil && + event.TransactionInfo.Source == sentry.SourceRoute + + if event.Transaction != "" && !namedByRoute { event.Transaction = sentryTransactionName( event.Transaction, pattern, ) @@ -120,7 +126,8 @@ func scrubSentryRequest( // The transaction dispatch has no such request: Span.doFinish calls // hub.CaptureEvent (tracing.go:356), which passes a nil hint that the // client replaces with an empty one (client.go:620-622). The pattern -// is therefore always "" there, and the callers fall back. +// is therefore always "" there, and the callers fall back. A routed +// transaction is named by nameSentryTransaction instead. func sentryRoutePattern(hint *sentry.EventHint) string { if hint == nil || hint.Context == nil { return "" @@ -142,6 +149,52 @@ func sentryRoutePattern(hint *sentry.EventHint) string { return rctx.RoutePattern() } +// nameSentryTransaction renames the request's Sentry transaction to +// "METHOD ", such as "POST /h/{uuid}". sentryhttp names +// it after the concrete path, which scrubSentryRequest withholds; +// since Sentry groups transactions by name, every transaction in the +// service would otherwise land in one group per method, such as +// "POST /(redacted)". +// +// It has to run inside the sentryhttp handler, where the transaction +// is on the request context. It renames on the way out, once chi has +// routed the request, and does so in a defer so that a request whose +// handler panics is named too. The context is taken when the defer is +// set up, before routing, but the chi routing context on it is a +// pointer that chi fills in as it routes. +func nameSentryTransaction(next http.Handler) http.Handler { + return http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + defer setSentryTransactionName(r.Context(), r.Method) + + next.ServeHTTP(w, r) + }, + ) +} + +// setSentryTransactionName does the rename for nameSentryTransaction. +// A path that matches nothing at the top level has no pattern; its +// name is left as the SDK set it, for scrubSentryRequest to withhold. +// Inside a route group, a path that matches no route in the group, or +// a request the group's own middleware turns away before routing, has +// the group's pattern, such as "/pages/*", and is named after it. +func setSentryTransactionName(ctx context.Context, method string) { + transaction := sentry.TransactionFromContext(ctx) + rctx := chi.RouteContext(ctx) + + if transaction == nil || rctx == nil { + return + } + + pattern := rctx.RoutePattern() + if pattern == "" { + return + } + + transaction.Name = method + " " + pattern + transaction.Source = sentry.SourceRoute +} + // sentryRouteURL rebuilds an event's request URL with the route // pattern in place of the concrete path. // diff --git a/internal/server/sentry_test.go b/internal/server/sentry_test.go index 86e5cb0..cf1dcdc 100644 --- a/internal/server/sentry_test.go +++ b/internal/server/sentry_test.go @@ -16,6 +16,7 @@ import ( "github.com/go-chi/chi" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/config" "sneak.berlin/go/webhooker/internal/server" ) @@ -130,11 +131,15 @@ func (c sentryCase) capture(t *testing.T) []*sentry.Event { // router mirrors the one ordering these tests depend on, over the two // route patterns they need: a recovering middleware outside, then the // sentryhttp handler registered with Use and Repanic set, exactly as -// routes.go orders the two. The bare recover stands in for -// Middleware.Recoverer, which holds that outer slot in production; it -// is here only to keep panic stacks out of the test output. That the -// production one really does catch what sentryhttp re-raises is -// pinned separately, by TestSentryStillSeesAPanic. +// routes.go orders the two. It leaves out nameSentryTransaction, +// which routes.go installs inside the sentryhttp handler, so a +// transaction captured here keeps the SDK's name and shows what the +// hook does with a transaction that missed the rename. The bare +// recover stands in for Middleware.Recoverer, which holds that outer +// slot in production; it is here only to keep panic stacks out of the +// test output. That the production one really does catch what +// sentryhttp re-raises is pinned separately, by +// TestSentryStillSeesAPanic. func (c sentryCase) router() http.Handler { handler := func(_ http.ResponseWriter, r *http.Request) { // This call is what drains the body tee and fills the @@ -353,8 +358,9 @@ func TestSentryScrub_KeepsTheRoutingContext(t *testing.T) { // Span.doFinish captures with a nil hint, so BeforeSendTransaction // gets one with no context and no request: the route pattern is out of // reach and both the URL and the SDK-built transaction name have to -// fall back. Tracing is off in this service, so no transaction event -// is produced today; the hook is a floor against that changing. +// fall back. This router has no nameSentryTransaction, so this is the +// floor under a transaction that missed the rename; +// TestSentryTransaction_NamedByRoute covers the routes as they ship. func TestSentryScrub_RedactsTheTransactionDispatch(t *testing.T) { t.Parallel() @@ -376,6 +382,83 @@ func TestSentryScrub_RedactsTheTransactionDispatch(t *testing.T) { assert.Equal(t, "POST /(redacted)", event.Transaction) } +// TestSentryTransaction_NamedByRoute serves requests through the real +// route tree with tracing on, which the service leaves off. A routed +// request's transaction is named after its route pattern, so Sentry +// can group transactions by route, and the entrypoint UUID in the +// receiver path appears nowhere in the event. A path that matches +// nothing at the top level has no pattern, and its name is still +// withheld; one that matches no route inside a route group is named +// after the group's pattern. +func TestSentryTransaction_NamedByRoute(t *testing.T) { + t.Parallel() + + // newTestEnv leaves the receiver rate limit at zero, which + // refuses every request before it reaches the receiver. + env := newTestEnvWithConfig(t, &config.Config{ + DataDir: t.TempDir(), + Environment: config.EnvironmentDev, + ReceiverRateLimit: 10, + }) + + router := server.NewRouterWithSentryForTest( + t, env.log, env.cfg, env.mw, env.hnd, + ) + + for _, tc := range []struct { + method, path, want string + }{ + { + http.MethodPost, + "/h/" + sentryReceiverUUID, + "POST /h/{uuid}", + }, + // An admin page route group, which installs its own + // sentryhttp handler. + {http.MethodGet, "/pages/login", "GET /pages/login"}, + {http.MethodGet, "/pages/unrouted", "GET /pages/*"}, + { + http.MethodPost, + "/h/" + sentryReceiverUUID + "/unrouted", + "POST /(redacted)", + }, + } { + t.Run(tc.want, func(t *testing.T) { + t.Parallel() + + transport := &captureTransport{} + + opts := server.SentryClientOptionsForTest( + "https://public@sentry.invalid/1", + "webhooker-test", + ) + opts.Transport = transport + opts.EnableTracing = true + opts.TracesSampleRate = 1.0 + + client, err := sentry.NewClient(opts) + require.NoError(t, err) + + req := httptest.NewRequestWithContext( + sentry.SetHubOnContext( + context.Background(), + sentry.NewHub(client, sentry.NewScope()), + ), + tc.method, tc.path, nil, + ) + router.ServeHTTP(httptest.NewRecorder(), req) + + event := onlyEvent(t, transport.events) + require.Equal(t, "transaction", event.Type) + + assert.Equal(t, tc.want, event.Transaction) + assert.NotContains( + t, marshalEvent(t, event), sentryReceiverUUID, + ) + }) + } +} + // TestSentryScrub_TransactionDispatchIsUnscrubbedWithoutTheHook pins // that dispatch's premise the same way, since it is the one the // service does not exercise today. -- 2.54.0