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

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

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)
 	}
 }
 

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 <route pattern>", 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.
 //

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.