Name each database target's archive for its webhook and target (closes #376)

Each database target now has its own archive file,
archive-WEBHOOKNAME-TARGETNAME-TARGETID.db, instead of one
archive-WEBHOOKID.db per webhook. delivery.ArchiveFileName builds the
name: each name is lowercased, keeps ASCII letters and digits, turns
every other run of characters into one dash, and is cut to 40
characters.

A change of webhook or target name renames its archive files under the
archive writer's lock, before the new name is saved, and back again if
the save fails. A rename never replaces a file: if one already has the
new name, the edit is refused. Deleting a target evicts only that
target's writer. Archive files are never deleted, and nothing looks for
files under the old name.

Model: opus-5-5

README.md

@@ -1929,12 +1929,10 @@ file under the new name.
 The file is moved just before the new name is saved. If the process
 stops between the two, the archive is left under the new name while the
 UI still shows the old one, and the next delivery starts a second
-archive under the name shown. To bring them back together, stop the
+archive under the name shown. To bring them back together, move the
-service before moving either file. If no file has the name shown, move
+file under the new name back to the name shown; if a second archive is
-the file under the new name back to it. If a second archive already has
+already there, move the older file out of the data directory instead
-the name shown, move the file under the new name out of the data
+and keep it as you would any archive moved away.
-directory instead and keep it as you would any archive moved away. Then
-start the service again.
 
 After each write the archive handle is closed
 and reopened, debounced to at most once per second, so an operator can
@@ -2979,15 +2977,13 @@ Components are wired via Uber fx in this order:
 7. `healthcheck.New` — Health check service
 8. `session.New` — Cookie-based session manager (key from database)
 9. `handlers.New` — HTTP handlers
-10. `metrics.NewRegistry` — The registry `/metrics` serves
+10. `middleware.New` — HTTP middleware
-11. `metrics.New` — The delivery collectors, registered on that registry
+11. `delivery.New` — Event-driven delivery engine
-12. `middleware.New` — HTTP middleware
+12. `delivery.NewArchiveSweeper` — Periodic pruning of idle archives
-13. `delivery.New` — Event-driven delivery engine
+13. `delivery.Engine` → `delivery.Notifier` — interface bridge
-14. `delivery.NewArchiveSweeper` — Periodic pruning of idle archives
+14. `delivery.Engine` → `delivery.Archives` — interface bridge so
-15. `delivery.Engine` → `delivery.Notifier` — interface bridge
-16. `delivery.Engine` → `delivery.Archives` — interface bridge so
     deleting or renaming a webhook or target reaches its archive files
-17. `server.New` — HTTP server and router
+15. `server.New` — HTTP server and router
 
 The server starts via `fx.Invoke(func(*server.Server, *delivery.Engine,
 *database.RetentionReaper, *delivery.ArchiveSweeper) {})`, which

cmd/webhooker/main.go

@@ -16,7 +16,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/healthcheck"
 	"sneak.berlin/go/webhooker/internal/logger"
-	"sneak.berlin/go/webhooker/internal/metrics"
 	"sneak.berlin/go/webhooker/internal/middleware"
 	"sneak.berlin/go/webhooker/internal/resetpw"
 	"sneak.berlin/go/webhooker/internal/server"
@@ -178,10 +177,6 @@ func newApp() *fx.App {
 			healthcheck.New,
 			session.New,
 			handlers.New,
-			// The registry /metrics serves, and the delivery
-			// collectors registered on it.
-			metrics.NewRegistry,
-			metrics.New,
 			middleware.New,
 			// The one SSRF guard both target-creation validation
 			// and the delivery dialer consult, so they cannot

internal/delivery/engine.go

@@ -14,7 +14,6 @@ import (
 	"go.uber.org/fx"
 	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/globals"
 	"sneak.berlin/go/webhooker/internal/lifecycle"
 	"sneak.berlin/go/webhooker/internal/logger"
 	"sneak.berlin/go/webhooker/internal/metrics"
@@ -150,10 +149,8 @@ type EngineParams struct {
 
 	DB        *database.Database
 	DBManager *database.WebhookDBManager
-	Globals   *globals.Globals
 	Logger    *logger.Logger
 	SSRFGuard *Guard
-	Metrics   *metrics.Set
 }
 
 // Engine processes queued deliveries in the background
@@ -173,14 +170,10 @@ type Engine struct {
 	retryCh    chan Task
 	workers    int
 
-	// version is the running build's version, the one the web UI
+	// mtr is the delivery metric set. Production wires the
-	// footer shows. userAgent puts it on every outbound request.
+	// process-wide one; a test can substitute a set registered on
-	version string
+	// a private registry so its assertions are not disturbed by
-
+	// deliveries other tests are making at the same time.
-	// mtr is the delivery metric set. Production wires the one
-	// registered on the registry /metrics serves; a test can
-	// substitute a set registered on a registry it holds, so it can
-	// gather what its own deliveries recorded.
 	mtr *metrics.Set
 
 	// targets maps each target type to its implementation.
@@ -214,8 +207,7 @@ func New(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    defaultWorkers,
-		version:    params.Globals.Version,
+		mtr:        metrics.Default(),
-		mtr:        params.Metrics,
 	}
 
 	e.initTargets(&http.Client{
@@ -338,13 +330,6 @@ func (e *Engine) ScheduleRetry(
 	})
 }
 
-// userAgent is the User-Agent header of every http and slack
-// delivery request: the program name and the running build's
-// version.
-func (e *Engine) userAgent() string {
-	return "webhooker/" + e.version
-}
-
 // registerHooks wires the engine's start and stop into the fx
 // lifecycle. The start hook's context is deliberately ignored
 // (see start for why the worker pool must not inherit it); the

internal/delivery/engine_test.go

@@ -1239,6 +1239,11 @@ func TestDoHTTPRequest_ForwardsHeaders(t *testing.T) {
 		testContentType,
 		receivedHeaders.Get("Content-Type"),
 	)
+
+	assert.Equal(t,
+		"webhooker/1.0",
+		receivedHeaders.Get("User-Agent"),
+	)
 }
 
 // The event's stored inbound headers carry the same Content-Type the
@@ -1307,7 +1312,6 @@ func TestApplyRequestHeaders_SendsOneContentType(t *testing.T) {
 					ContentType: tc.event,
 				},
 				cfg,
-				"webhooker/dev",
 			)
 
 			assert.Equal(t,

internal/delivery/export_test.go

@@ -9,7 +9,6 @@ import (
 	"net/url"
 	"time"
 
-	"github.com/prometheus/client_golang/prometheus"
 	"go.uber.org/fx"
 	"gorm.io/gorm"
 	"sneak.berlin/go/webhooker/internal/database"
@@ -83,9 +82,8 @@ func ExportApplyRequestHeaders(
 	req *http.Request,
 	event *database.Event,
 	cfg *HTTPTargetConfig,
-	userAgent string,
 ) []string {
-	return applyRequestHeaders(req, event, cfg, userAgent)
+	return applyRequestHeaders(req, event, cfg)
 }
 
 // ExportTruncate exposes truncate for testing.
@@ -401,7 +399,7 @@ func NewTestEngine(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    workers,
-		mtr:        metrics.New(prometheus.NewRegistry()),
+		mtr:        metrics.Default(),
 	}
 	e.initTargets(client)
 
@@ -416,7 +414,7 @@ func NewTestEngineSmallRetry(
 	e := &Engine{
 		log:     log,
 		retryCh: make(chan Task, 1),
-		mtr:     metrics.New(prometheus.NewRegistry()),
+		mtr:     metrics.Default(),
 	}
 	e.initTargets(nil)
 
@@ -439,7 +437,7 @@ func NewTestEngineWithDB(
 		deliveryCh: make(chan Task, deliveryChannelSize),
 		retryCh:    make(chan Task, retryChannelSize),
 		workers:    workers,
-		mtr:        metrics.New(prometheus.NewRegistry()),
+		mtr:        metrics.Default(),
 	}
 	e.initTargets(client)
 
@@ -447,7 +445,8 @@ func NewTestEngineWithDB(
 }
 
 // ExportSetMetrics substitutes the engine's metric set, so a test can
-// assert on collectors registered on a registry it holds.
+// assert on collectors registered on a private registry instead of
+// the process-wide ones every other test is also moving.
 func (e *Engine) ExportSetMetrics(mtr *metrics.Set) {
 	e.mtr = mtr
 }

internal/delivery/metrics_test.go

@@ -35,8 +35,9 @@ const (
 )
 
 // mIsolate gives the setup's engine a metric set registered on a
-// registry this test holds, so its exact assertions can gather from
+// private registry. The process-wide collectors are moved by every
-// it.
+// other delivery test running in parallel, so exact assertions are
+// only possible against a registry this test owns.
 func mIsolate(
 	t *testing.T, s iSetup,
 ) *prometheus.Registry {

internal/delivery/redirect_test.go

@@ -375,7 +375,6 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 				"Content-Type":  testContentType,
 			},
 		},
-		"webhooker/dev",
 	)
 
 	assert.Equal(t,

internal/delivery/target_database_test.go

@@ -521,58 +521,47 @@ func TestRename_MovesTheFile(t *testing.T) {
 	)
 }
 
-// TestRename_NeverReplacesAFile plants a file at the new name, once
+// TestRename_NeverReplacesAFile plants a file at the new name and
-// the .db alone, once a lone -wal and once a lone -shm, and proves
+// proves the rename is refused, the planted file survives, and the
-// each time that the rename is refused, the planted file survives,
+// archive keeps its name and its rows.
-// and the archive keeps its name and its rows.
 func TestRename_NeverReplacesAFile(t *testing.T) {
 	t.Parallel()
 
-	for _, suffix := range archiveFileSuffixes() {
+	env := setupArchiveTest(t)
-		t.Run("planted .db"+suffix, func(t *testing.T) {
+	tgt := env.seedDatabaseTarget(t, "")
-			t.Parallel()
+	oldPath := env.archivePath(tgt)
 
-			env := setupArchiveTest(t)
+	webhookDB := testWebhookDB(t)
-			tgt := env.seedDatabaseTarget(t, "")
+	first := seedEvent(t, webhookDB, `{"n":1}`)
-			oldPath := env.archivePath(tgt)
+	env.eng.ExportDeliverDatabase(
+		webhookDB, seedDatabaseTargetDelivery(t, webhookDB, first, tgt),
+	)
 
-			webhookDB := testWebhookDB(t)
+	newPath := filepath.Join(
-			first := seedEvent(t, webhookDB, `{"n":1}`)
+		env.dataDir, "archive-orders-long-term-"+tgt.ID+".db",
-			env.eng.ExportDeliverDatabase(
+	)
-				webhookDB,
+	require.NoError(t, os.WriteFile(newPath, []byte("planted"), 0o600))
-				seedDatabaseTargetDelivery(t, webhookDB, first, tgt),
-			)
 
-			newPath := filepath.Join(
+	require.ErrorIs(
-				env.dataDir, "archive-orders-long-term-"+tgt.ID+".db",
+		t, env.eng.Rename(tgt.ID, "Orders", "Long Term"),
-			)
+		delivery.ErrArchiveNameTaken,
-			plantedPath := newPath + suffix
+	)
-			require.NoError(
-				t, os.WriteFile(plantedPath, []byte("planted"), 0o600),
-			)
 
-			require.ErrorIs(
+	//nolint:gosec // reads the file the test planted under t.TempDir()
-				t, env.eng.Rename(tgt.ID, "Orders", "Long Term"),
+	planted, err := os.ReadFile(newPath)
-				delivery.ErrArchiveNameTaken,
+	require.NoError(t, err)
-			)
+	assert.Equal(t, "planted", string(planted))
 
-			//nolint:gosec // reads the file the test planted under t.TempDir()
+	second := seedEvent(t, webhookDB, `{"n":2}`)
-			planted, err := os.ReadFile(plantedPath)
+	env.eng.ExportDeliverDatabase(
-			require.NoError(t, err)
+		webhookDB,
-			assert.Equal(t, "planted", string(planted))
+		seedDatabaseTargetDelivery(t, webhookDB, second, tgt),
+	)
 
-			second := seedEvent(t, webhookDB, `{"n":2}`)
+	assert.ElementsMatch(
-			env.eng.ExportDeliverDatabase(
+		t, []string{first.ID, second.ID},
-				webhookDB,
+		archivedEventIDs(t, oldPath),
-				seedDatabaseTargetDelivery(t, webhookDB, second, tgt),
+	)
-			)
-
-			assert.ElementsMatch(
-				t, []string{first.ID, second.ID},
-				archivedEventIDs(t, oldPath),
-			)
-		})
-	}
 }
 
 // TestRename_BeforeTheNameIsSaved covers the order the handlers

internal/delivery/target_http.go

@@ -442,9 +442,7 @@ func (t *httpTarget) doHTTPRequest(
 		)
 	}
 
-	originScoped := applyRequestHeaders(
+	originScoped := applyRequestHeaders(req, event, cfg)
-		req, event, cfg, t.eng.userAgent(),
-	)
 
 	client := t.clientForRequest(cfg, originScoped)
 
@@ -564,13 +562,10 @@ func isForwardableHeader(name string) bool {
 // Content-Type goes out once: a Content-Type configured on the target
 // wins, otherwise the event's ContentType, otherwise none. The inbound
 // Content-Type in the event's headers is never forwarded.
-//
-// userAgent is set last, over any configured or inbound User-Agent.
 func applyRequestHeaders(
 	req *http.Request,
 	event *database.Event,
 	cfg *HTTPTargetConfig,
-	userAgent string,
 ) []string {
 	if event.ContentType != "" {
 		req.Header.Set(
@@ -585,7 +580,7 @@ func applyRequestHeaders(
 		originScoped[http.CanonicalHeaderKey(k)] = struct{}{}
 	}
 
-	req.Header.Set("User-Agent", userAgent)
+	req.Header.Set("User-Agent", "webhooker/1.0")
 
 	// A Content-Type configured on the target describes the body
 	// being sent rather than the sender. A 307/308 preserves the

internal/delivery/target_slack.go

@@ -136,7 +136,7 @@ func (t *slackTarget) attempt(
 	}
 
 	req.Header.Set("Content-Type", "application/json")
-	req.Header.Set("User-Agent", t.eng.userAgent())
+	req.Header.Set("User-Agent", "webhooker/1.0")
 
 	resp, doErr := executeHTTPRequest(t.client, req)
 	durationMs := time.Since(start).Milliseconds()

internal/delivery/user_agent_test.go

@@ -1,91 +0,0 @@
-package delivery_test
-
-import (
-	"context"
-	"encoding/json"
-	"net/http"
-	"net/http/httptest"
-	"net/netip"
-	"testing"
-
-	"github.com/google/uuid"
-	"github.com/prometheus/client_golang/prometheus"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"go.uber.org/fx/fxtest"
-	"sneak.berlin/go/webhooker/internal/database"
-	"sneak.berlin/go/webhooker/internal/delivery"
-	"sneak.berlin/go/webhooker/internal/globals"
-	"sneak.berlin/go/webhooker/internal/logger"
-	"sneak.berlin/go/webhooker/internal/metrics"
-)
-
-// Both the http and the slack target send webhooker/ and the version
-// in Globals, the value the web UI footer shows. A User-Agent
-// configured on the target or carried in by the sender does not
-// replace it.
-func TestUserAgent_IsTheBuildVersion(t *testing.T) {
-	t.Parallel()
-
-	const want = "webhooker/1.2.3-test"
-
-	userAgents := make(chan string, 1)
-
-	ts := httptest.NewServer(http.HandlerFunc(
-		func(w http.ResponseWriter, r *http.Request) {
-			userAgents <- r.Header.Get("User-Agent")
-
-			w.WriteHeader(http.StatusOK)
-		},
-	))
-	defer ts.Close()
-
-	g := &globals.Globals{Version: "1.2.3-test"}
-	lc := fxtest.NewLifecycle(t)
-
-	log, err := logger.New(lc, logger.LoggerParams{Globals: g})
-	require.NoError(t, err)
-
-	e := delivery.New(lc, delivery.EngineParams{
-		Globals: g,
-		Logger:  log,
-		// httptest listens on loopback, which the default guard
-		// refuses.
-		SSRFGuard: delivery.NewTestGuard(
-			netip.MustParsePrefix("127.0.0.0/8"),
-		),
-		Metrics: metrics.New(prometheus.NewRegistry()),
-	})
-
-	statusCode, _, _, err := e.ExportDoHTTPRequest(
-		context.Background(),
-		&delivery.HTTPTargetConfig{
-			URL:     ts.URL,
-			Headers: map[string]string{"User-Agent": "configured/1"},
-		},
-		&database.Event{Headers: `{"User-Agent":["curl/8"]}`},
-	)
-	require.NoError(t, err)
-	require.Equal(t, http.StatusOK, statusCode)
-	require.Len(t, userAgents, 1, "the http target sent no request")
-	assert.Equal(t, want, <-userAgents, "http target")
-
-	db := testWebhookDB(t)
-	targetID := uuid.New().String()
-
-	slackCfg, err := json.Marshal(
-		delivery.SlackTargetConfig{WebhookURL: ts.URL},
-	)
-	require.NoError(t, err)
-
-	event := seedEvent(t, db, `{"action":"test"}`)
-	dlv := seedDelivery(
-		t, db, event.ID, targetID, database.DeliveryStatusPending,
-	)
-
-	e.ExportDeliverSlack(context.Background(), db, buildSlackDelivery(
-		dlv, event, targetID, "test-slack", string(slackCfg),
-	))
-	require.Len(t, userAgents, 1, "the slack target sent no request")
-	assert.Equal(t, want, <-userAgents, "slack target")
-}

internal/handlers/handlers.go

@@ -12,7 +12,6 @@ import (
 	"net/http"
 	"sync/atomic"
 
-	"github.com/prometheus/client_golang/prometheus"
 	"go.uber.org/fx"
 	"sneak.berlin/go/webhooker/internal/database"
 	"sneak.berlin/go/webhooker/internal/delivery"
@@ -65,8 +64,6 @@ type HandlersParams struct {
 	Notifier     delivery.Notifier
 	Archives     delivery.Archives
 	SSRFGuard    *delivery.Guard
-	Metrics      *metrics.Set
-	Registry     *prometheus.Registry
 }
 
 // Handlers provides HTTP handler methods for all application
@@ -132,7 +129,7 @@ func New(
 	s.mw = params.Middleware
 	s.notifier = params.Notifier
 	s.archives = params.Archives
-	s.mtr = params.Metrics
+	s.mtr = metrics.Default()
 	s.ssrf = params.SSRFGuard
 
 	// Parse all page templates once at startup

internal/handlers/handlers_test.go

@@ -21,7 +21,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/healthcheck"
 	"sneak.berlin/go/webhooker/internal/logger"
-	"sneak.berlin/go/webhooker/internal/metrics"
 	"sneak.berlin/go/webhooker/internal/middleware"
 	"sneak.berlin/go/webhooker/internal/session"
 )
@@ -183,8 +182,6 @@ func newTestApp(
 			func(r *recordingArchives) delivery.Archives {
 				return r
 			},
-			metrics.NewRegistry,
-			metrics.New,
 			middleware.New,
 			delivery.NewGuard,
 			handlers.New,

internal/handlers/metrics.go

@@ -1,21 +0,0 @@
-package handlers
-
-import (
-	"net/http"
-
-	"github.com/prometheus/client_golang/prometheus/promhttp"
-)
-
-// HandleMetrics returns the Prometheus scrape handler for the
-// registry built by metrics.NewRegistry, which the HTTP, delivery, Go
-// runtime and process collectors register on. It is what
-// promhttp.Handler builds for the global default registry, including
-// the promhttp_metric_handler_* series that count scrapes, pointed at
-// that registry instead.
-func (s *Handlers) HandleMetrics() http.HandlerFunc {
-	reg := s.params.Registry
-
-	return promhttp.InstrumentMetricHandler(
-		reg, promhttp.HandlerFor(reg, promhttp.HandlerOpts{}),
-	).ServeHTTP
-}

internal/handlers/source_delete_test.go

@@ -80,10 +80,6 @@ func seedTarget(
 // from a delete statement.
 var errInjectedDelete = errors.New("injected delete failure")
 
-// errInjectedSave is the failure failSaveOnTable reports from a
-// save of an existing row.
-var errInjectedSave = errors.New("injected save failure")
-
 // seedEntrypoint inserts an entrypoint for a webhook.
 func seedEntrypoint(
 	t *testing.T,
@@ -151,28 +147,6 @@ func failDeleteOnTable(
 	)
 }
 
-// failSaveOnTable is failDeleteOnTable for saves: every update of
-// an existing row in the named table fails.
-func failSaveOnTable(
-	t *testing.T,
-	db *database.Database,
-	table string,
-) {
-	t.Helper()
-
-	require.NoError(t, db.DB().Callback().Update().
-		Before("gorm:update").
-		Register(
-			"test:fail_save_"+table,
-			func(tx *gorm.DB) {
-				if tx.Statement.Table == table {
-					_ = tx.AddError(errInjectedSave)
-				}
-			},
-		),
-	)
-}
-
 // archivePathFor returns the archive database path the
 // delivery engine would use for a database target: beside the
 // webhook's event database in the data directory.

internal/handlers/source_detail_test.go

@@ -241,37 +241,3 @@ func TestHandleSourceDetail_RendersNamedTargetFields(
 	assert.Contains(t, body, "(unavailable)")
 	assert.NotContains(t, body, "beak")
 }
-
-// TestHandleSourceDetail_FitsWideAndNarrowWindows pins the webhook
-// page's maximum width at 108rem (1728 px), half again the 72rem of
-// max-w-6xl that the webhook list and the event log use, so an
-// entrypoint URL fits on one line in a 1920-pixel window; and the
-// wrapping of its title row, so the buttons beside the title do not
-// push a phone-width window into scrolling sideways.
-func TestHandleSourceDetail_FitsWideAndNarrowWindows(t *testing.T) {
-	t.Parallel()
-
-	var (
-		h    *handlers.Handlers
-		sess *session.Session
-		db   *database.Database
-	)
-
-	app := newTestApp(t, &h, &sess, &db)
-	app.RequireStart()
-
-	t.Cleanup(app.RequireStop)
-
-	wh := seedWebhook(t, db)
-
-	body := renderSourceDetailPage(t, h, sess, wh.ID)
-
-	assert.Contains(
-		t, body,
-		`<div class="mx-auto px-6 py-8" style="max-width: 108rem"`,
-	)
-	assert.Contains(
-		t, body,
-		`<div class="flex flex-wrap justify-between items-center gap-2 mt-2">`,
-	)
-}

internal/handlers/source_management_test.go

@@ -575,42 +575,6 @@ func TestHandleSourceEditSubmit_FailedRenameKeepsTheName(
 	)
 }
 
-// TestHandleSourceEditSubmit_FailedSaveRenamesBack proves that when
-// the archive is renamed but the new name cannot be saved, the
-// archive is renamed back to the stored name and the stored name
-// stays.
-func TestHandleSourceEditSubmit_FailedSaveRenamesBack(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-	wh := seedWebhookWithRetention(t, env.db, 7)
-	tgt := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase)
-
-	failSaveOnTable(t, env.db, "webhooks")
-
-	oldName := wh.Name
-	wh.Name = renamedWebhookName
-
-	w := submitEdit(t, env, wh, "")
-	require.Equal(t, http.StatusInternalServerError, w.Code)
-
-	var stored database.Webhook
-
-	require.NoError(
-		t, env.db.DB().First(&stored, "id = ?", wh.ID).Error,
-	)
-	assert.Equal(t, oldName, stored.Name)
-
-	assert.Equal(
-		t,
-		[]archiveRename{
-			{tgt.ID, renamedWebhookName, tgt.Name},
-			{tgt.ID, oldName, tgt.Name},
-		},
-		env.archives.Renames(),
-	)
-}
-
 // TestHandleSourceEditSubmit_ArchiveNameTaken proves that when a file
 // already has an archive's new name, the edit is refused with an
 // error naming that file, and the webhook keeps its stored name.

internal/handlers/target_edit_test.go

@@ -700,35 +700,3 @@ func TestHandleTargetEditSubmit_RenamesArchive(t *testing.T) {
 		t, renamedTargetName, storedTarget(t, env, archive.ID).Name,
 	)
 }
-
-// TestHandleTargetEditSubmit_FailedSaveRenamesBack proves that when a
-// database target's archive is renamed but the new name cannot be
-// saved, the archive is renamed back to the stored name and the
-// stored name stays.
-func TestHandleTargetEditSubmit_FailedSaveRenamesBack(t *testing.T) {
-	t.Parallel()
-
-	env := setupSourceTest(t)
-	wh := seedWebhookWithRetention(t, env.db, 7)
-	archive := seedTarget(t, env.db, wh.ID, database.TargetTypeDatabase)
-
-	failSaveOnTable(t, env.db, "targets")
-
-	form := url.Values{}
-	form.Set("name", renamedTargetName)
-
-	w := submitTargetEdit(env, wh.ID, archive.ID, form)
-	require.Equal(t, http.StatusInternalServerError, w.Code)
-	assert.Equal(
-		t, archive.Name, storedTarget(t, env, archive.ID).Name,
-	)
-
-	assert.Equal(
-		t,
-		[]archiveRename{
-			{archive.ID, wh.Name, renamedTargetName},
-			{archive.ID, wh.Name, archive.Name},
-		},
-		env.archives.Renames(),
-	)
-}

internal/metrics/metrics.go

@@ -3,18 +3,17 @@
 // deliveries are attempted, how they end, how long they take, how
 // deep the queues are, and how many circuit breakers are open.
 //
-// It also builds the registry the authenticated /metrics route
+// The inbound HTTP metrics come from the go-http-metrics recorder in
-// serves. In production, these collectors, the inbound HTTP metrics
+// internal/middleware and land on prometheus.DefaultRegisterer. These
-// recorded in internal/middleware, and the Go runtime and process
+// collectors register there too, so both surfaces are gathered by the
-// collectors all register on that one registry, never on Prometheus's
+// one promhttp handler mounted on the authenticated /metrics route.
-// global default.
 package metrics
 
 import (
+	"sync"
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
-	"github.com/prometheus/client_golang/prometheus/collectors"
 	"github.com/prometheus/client_golang/prometheus/promauto"
 	"sneak.berlin/go/webhooker/internal/database"
 )
@@ -58,31 +57,25 @@ var knownTargetTypes = []database.TargetType{
 	database.TargetTypeSlack,
 }
 
-// NewRegistry returns the registry /metrics serves, carrying the Go
+// defaultSet is the process-wide metric set, registered on the same
-// runtime and process collectors that Prometheus's global default
+// registry the HTTP middleware and the /metrics handler already use.
-// registry carries, so the go_* and process_* series stay in the
+// It is built on first use rather than in an init so that a test
-// scrape.
+// binary that never touches metrics never registers them.
 //
-// A registry of its own, rather than the global default, is what lets
+//nolint:gochecknoglobals // one process-wide registration, by design
-// two dependency graphs in one process — two tests, say — each
+var defaultSet = sync.OnceValue(func() *Set {
-// register their collectors without the second registration
+	return New(prometheus.DefaultRegisterer)
-// panicking.
+})
-func NewRegistry() *prometheus.Registry {
-	reg := prometheus.NewRegistry()
-	reg.MustRegister(
-		collectors.NewGoCollector(),
-		collectors.NewProcessCollector(
-			collectors.ProcessCollectorOpts{},
-		),
-	)
 
-	return reg
+// Default returns the process-wide metric set.
+func Default() *Set {
+	return defaultSet()
 }
 
 // Set is one registered group of webhooker's delivery collectors.
-// Production builds one on the registry /metrics serves; tests build
+// Production uses the single Default set; tests build their own
-// one on a registry of their own so they can gather what their own
+// against a private registry so assertions are not disturbed by
-// deliveries recorded.
+// deliveries other tests are making concurrently.
 type Set struct {
 	eventsReceived      prometheus.Counter
 	deliveryAttempts    *prometheus.CounterVec
@@ -100,7 +93,7 @@ type Set struct {
 // New registers a full set of delivery collectors on reg and returns
 // it. It panics if reg already holds them, which is the intended
 // behaviour for a duplicate registration.
-func New(reg *prometheus.Registry) *Set {
+func New(reg prometheus.Registerer) *Set {
 	factory := promauto.With(reg)
 
 	s := &Set{

internal/middleware/export_test.go

@@ -10,7 +10,8 @@ import (
 
 // MetricsMiddlewareForTest builds the metrics recording middleware
 // against a caller-supplied recorder, so a test can gather from its
-// own Prometheus registry without building a whole Middleware.
+// own Prometheus registry rather than the process-wide default one
+// that Middleware.Metrics uses.
 func MetricsMiddlewareForTest(
 	rec httpmetrics.Recorder,
 ) func(http.Handler) http.Handler {

internal/middleware/metrics.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/go-chi/chi"
 	httpmetrics "github.com/slok/go-http-metrics/metrics"
+	prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
 	ghmm "github.com/slok/go-http-metrics/middleware"
 	"github.com/slok/go-http-metrics/middleware/std"
 )
@@ -150,17 +151,17 @@ func (r boundedLabelRecorder) AddInflightRequests(
 
 var _ httpmetrics.Recorder = boundedLabelRecorder{}
 
-// Metrics returns middleware that records Prometheus HTTP metrics
+// Metrics returns middleware that records Prometheus HTTP metrics on
-// with the Middleware's one recorder, which New builds on the registry
+// the default registry, which is the one the /metrics route gathers.
-// the /metrics route serves and NewForTest on a registry of its own.
-// Every call reuses that recorder, so any number of routers can
-// install it.
 func (s *Middleware) Metrics() func(http.Handler) http.Handler {
-	return metricsMiddleware(s.metricsRecorder)
+	return metricsMiddleware(
+		prommetrics.NewRecorder(prommetrics.Config{}),
+	)
 }
 
 // metricsMiddleware builds the recording middleware against a given
-// recorder, so tests can gather from a registry of their own.
+// recorder, so tests can gather from a registry of their own instead
+// of the process-wide default.
 func metricsMiddleware(
 	rec httpmetrics.Recorder,
 ) func(http.Handler) http.Handler {

internal/middleware/metrics_test.go

@@ -57,8 +57,9 @@ const (
 // Server.setupWebhookRoutes inside it. That ordering is the whole
 // defect, so a test that flattens it would prove nothing.
 //
-// The recorder writes to a registry of the test's own, so each test
+// The recorder writes to a registry of the test's own rather than the
-// observes only its own traffic.
+// process-wide default one, so each test observes only its own
+// traffic.
 func metricsTestRouter(
 	t *testing.T,
 	receiverLimit int,
@@ -454,29 +455,3 @@ func TestMetrics_StatusAndSizeStillRecorded(t *testing.T) {
 		"the interceptor must still count written bytes",
 	)
 }
-
-// TestMetrics_WorksOnNewForTestMiddleware pins that a Middleware built
-// by NewForTest has a recorder of its own: its Metrics() serves a
-// request instead of panicking, and a second one does not collide
-// with the first.
-func TestMetrics_WorksOnNewForTestMiddleware(t *testing.T) {
-	t.Parallel()
-
-	log := slog.New(slog.DiscardHandler)
-	cfg := &config.Config{Environment: "prod"}
-	ok := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
-		_, _ = w.Write([]byte(okBody))
-	})
-
-	for range 2 {
-		h := middleware.NewForTest(log, cfg, nil).Metrics()(ok)
-
-		req := httptest.NewRequestWithContext(
-			t.Context(), http.MethodGet, okRoute, nil,
-		)
-		w := httptest.NewRecorder()
-		h.ServeHTTP(w, req)
-
-		assert.Equal(t, http.StatusOK, w.Code)
-	}
-}

internal/middleware/middleware.go

@@ -14,9 +14,6 @@ import (
 	"github.com/go-chi/chi"
 	"github.com/go-chi/chi/middleware"
 	"github.com/go-chi/cors"
-	"github.com/prometheus/client_golang/prometheus"
-	httpmetrics "github.com/slok/go-http-metrics/metrics"
-	prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
 	"go.uber.org/fx"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/globals"
@@ -152,11 +149,10 @@ const (
 type MiddlewareParams struct {
 	fx.In
 
-	Logger   *logger.Logger
+	Logger  *logger.Logger
-	Globals  *globals.Globals
+	Globals *globals.Globals
-	Config   *config.Config
+	Config  *config.Config
-	Session  *session.Session
+	Session *session.Session
-	Registry *prometheus.Registry
 }
 
 // Middleware provides HTTP middleware for logging, CORS, auth, and
@@ -166,14 +162,6 @@ type Middleware struct {
 	params  *MiddlewareParams
 	session *session.Session
 
-	// metricsRecorder records the inbound HTTP metrics. New builds
-	// it on the registry /metrics serves, NewForTest on a registry
-	// of its own. Either way it is built once per Middleware and
-	// Metrics reuses it, because building it registers its
-	// collectors, and a second registration on the same registry
-	// panics.
-	metricsRecorder httpmetrics.Recorder
-
 	// loginGuard counts failed credential verifications and bounds
 	// concurrent password hashing. It is built on first use so that
 	// every construction path gets one; see guard().
@@ -192,9 +180,6 @@ func New(
 	s.params = &params
 	s.log = params.Logger.Get()
 	s.session = params.Session
-	s.metricsRecorder = prommetrics.NewRecorder(
-		prommetrics.Config{Registry: params.Registry},
-	)
 
 	return s, nil
 }

internal/middleware/testing.go

@@ -3,17 +3,12 @@ package middleware
 import (
 	"log/slog"
 
-	"github.com/prometheus/client_golang/prometheus"
-	prommetrics "github.com/slok/go-http-metrics/metrics/prometheus"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/session"
 )
 
 // NewForTest creates a Middleware with the minimum dependencies
 // needed for testing. This bypasses the fx lifecycle.
-//
-// Its metrics recorder writes to a fresh registry of its own, so
-// Metrics() works on it and two of them never collide.
 func NewForTest(
 	log *slog.Logger,
 	cfg *config.Config,
@@ -25,8 +20,5 @@ func NewForTest(
 			Config: cfg,
 		},
 		session: sess,
-		metricsRecorder: prommetrics.NewRecorder(
-			prommetrics.Config{Registry: prometheus.NewRegistry()},
-		),
 	}
 }

internal/resetpw/resetpw_test.go

@@ -24,7 +24,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/healthcheck"
 	"sneak.berlin/go/webhooker/internal/logger"
-	"sneak.berlin/go/webhooker/internal/metrics"
 	"sneak.berlin/go/webhooker/internal/middleware"
 	"sneak.berlin/go/webhooker/internal/resetpw"
 	"sneak.berlin/go/webhooker/internal/session"
@@ -170,8 +169,6 @@ func newServerApp(
 			session.New,
 			func() delivery.Notifier { return &noopNotifier{} },
 			func() delivery.Archives { return &noopArchives{} },
-			metrics.NewRegistry,
-			metrics.New,
 			middleware.New,
 			delivery.NewGuard,
 			handlers.New,

internal/server/routes.go

@@ -7,6 +7,7 @@ import (
 	sentryhttp "github.com/getsentry/sentry-go/http"
 	"github.com/go-chi/chi"
 	"github.com/go-chi/chi/middleware"
+	"github.com/prometheus/client_golang/prometheus/promhttp"
 	"sneak.berlin/go/webhooker/static"
 )
 
@@ -148,7 +149,12 @@ func (s *Server) setupRoutes() {
 	if s.params.Config.MetricsAuthEnabled() {
 		s.router.Group(func(r chi.Router) {
 			r.Use(s.mw.MetricsAuth())
-			r.Get("/metrics", s.h.HandleMetrics())
+			r.Get(
+				"/metrics",
+				http.HandlerFunc(
+					promhttp.Handler().ServeHTTP,
+				),
+			)
 		})
 	}
 

internal/server/routes_test.go

@@ -24,7 +24,6 @@ import (
 	"sneak.berlin/go/webhooker/internal/handlers"
 	"sneak.berlin/go/webhooker/internal/healthcheck"
 	"sneak.berlin/go/webhooker/internal/logger"
-	"sneak.berlin/go/webhooker/internal/metrics"
 	"sneak.berlin/go/webhooker/internal/middleware"
 	"sneak.berlin/go/webhooker/internal/server"
 	"sneak.berlin/go/webhooker/internal/session"
@@ -120,8 +119,6 @@ func newTestEnvWithConfig(
 			session.New,
 			func() delivery.Notifier { return &noopNotifier{} },
 			func() delivery.Archives { return &noopArchives{} },
-			metrics.NewRegistry,
-			metrics.New,
 			middleware.New,
 			delivery.NewGuard,
 			handlers.New,
@@ -1487,46 +1484,3 @@ func TestMetricsRouteUnmountedOnHalfSetConfig(t *testing.T) {
 		})
 	}
 }
-
-// TestTwoMetricsRoutersInOneProcess pins
-// https://git.eeqj.de/sneak/webhooker/issues/227: a second
-// metrics-enabled router in one process used to panic, because the
-// HTTP metrics registered on Prometheus's global default registry.
-// Two routers are built over separate dependency graphs and a third
-// over the first graph again, and each must still serve the HTTP,
-// delivery, Go runtime and process series, and the series counting
-// scrapes of /metrics itself.
-func TestTwoMetricsRoutersInOneProcess(t *testing.T) {
-	t.Parallel()
-
-	first := newTestEnvWithConfig(
-		t, metricsConfig(t, metricsUser, metricsAuthValue),
-	)
-	second := newTestEnvWithConfig(
-		t, metricsConfig(t, metricsUser, metricsAuthValue),
-	)
-	third := &testEnv{
-		router: server.NewRouterForTest(
-			first.log.Get(), first.cfg, first.mw, first.hnd,
-		),
-	}
-
-	for _, env := range []*testEnv{first, second, third} {
-		env.get("/", nil)
-
-		scrape := env.metricsRequest(metricsUser, metricsAuthValue)
-		require.Equal(t, http.StatusOK, scrape.Code)
-
-		for _, series := range []string{
-			"http_request_duration_seconds",
-			"http_response_size_bytes",
-			"http_requests_inflight",
-			"webhooker_events_received_total",
-			"go_goroutines",
-			"process_start_time_seconds",
-			"promhttp_metric_handler_requests_total",
-		} {
-			assert.Contains(t, scrape.Body.String(), series)
-		}
-	}
-}

templates/source_detail.html

@@ -3,14 +3,10 @@
 {{define "title"}}{{.Webhook.Name}} - Webhooker{{end}}
 
 {{define "content"}}
-<!-- 108rem, half again the 72rem (max-w-6xl) of the webhook list, the
+<div class="max-w-6xl mx-auto px-6 py-8" x-data="{ showAddEntrypoint: false, showAddTarget: false }">
-     event log, the navbar and the footer, so an entrypoint URL fits on
-     one line. An inline style, because the committed tailwind.css has
-     no class this wide. -->
-<div class="mx-auto px-6 py-8" style="max-width: 108rem" x-data="{ showAddEntrypoint: false, showAddTarget: false }">
     <div class="mb-6">
         <a href="/hooks" class="text-sm text-primary-600 hover:text-primary-700">&larr; Back to webhooks</a>
-        <div class="flex flex-wrap justify-between items-center gap-2 mt-2">
+        <div class="flex justify-between items-center mt-2">
             <div>
                 <h1 class="text-2xl font-medium text-gray-900">{{.Webhook.Name}}</h1>
                 {{if .Webhook.Description}}