Backend observability: fx logs bypass slog, Identify() is dead, Sentry/Prometheus config is parsed but unwired #27

New Issue

clawbot · 2026-08-09T03:45:07+02:00

clawbot commented

2026-08-09 03:45:07 +02:00

Problem

The backend parses configuration for three observability features that do not exist, and part of its own logging escapes the structured logger. Verified on main at fbfe1df.

1. fx's own logging bypasses slog entirely

backend/cmd/netwatch-server/main.go:29-41 calls fx.New(...) with no fx.WithLogger(...). fx therefore falls back to its built-in console logger, writing unstructured plain text to stderr for every provide, invoke, and lifecycle hook, plus all shutdown output.

CODE_STYLEGUIDE.md: "For services/servers, log JSON to stdout... Use structured logging whenever possible." Today a production deployment's log stream is a mix of clean JSON from internal/logger and unparseable plain text from fx.

Side effect: go.uber.org/zap v1.26.0 (go.mod:26) is linked into the binary purely as fx's default logger backend. No application code imports it. Bridging fx to slog removes zap's runtime role.

2. `logger.Identify()` is dead code

internal/logger/logger.go:79-85 defines Identify(), which logs appname, version, and buildarch. Grepping the backend finds only the definition — no call site.

CODE_STYLEGUIDE_GO.md: "Embed the git commit hash into the binary and include it in startup logs and in health check output." The version does reach the logs by accident via internal/server/http.go:29-33 ("http begin listen" carries version and buildarch), but appname never does, and the intended identification line never fires.

3. Three config values are parsed and never read

internal/config/config.go loads SENTRY_DSN, METRICS_USERNAME, and METRICS_PASSWORD into the Config struct. Nothing reads any of them.

GO_HTTP_SERVER_CONVENTIONS.md lists these as mandatory libraries and specifies the wiring:

github.com/getsentry/sentry-go — conditional init on SENTRY_DSN, sentryhttp middleware with Repanic: true, sentry.Flush(2*time.Second) on shutdown.
github.com/prometheus/client_golang + github.com/slok/go-http-metrics + github.com/99designs/basicauth-go — Metrics() middleware and a basic-auth-protected GET /metrics.

None are in go.mod. internal/middleware/middleware.go has no Metrics() or MetricsAuth(). internal/server/routes.go registers no /metrics route. The Server struct (server.go:37-47) omits the sentryEnabled field the conventions doc specifies.

backend/README.md:61 already lists Prometheus metrics as an open TODO.

4. Healthcheck JSON keys violate the snake_case rule

internal/healthcheck/healthcheck.go:39-40 emits uptimeHuman and uptimeSeconds.

CODE_STYLEGUIDE_GO.md: "Use snake_case for JSON keys." GO_HTTP_SERVER_CONVENTIONS.md independently specifies uptime_seconds and uptime_human. Both authorities agree; the code disagrees with both.

Also divergent, same file: the type is Response rather than HealthcheckResponse, the method is Check() rather than Healthcheck(), and maintenance_mode is absent (there is no MAINTENANCE_MODE config at all).

Correct and not to be regressed: the endpoint path /.well-known/healthcheck, Content-Type: application/json; charset=utf-8, "status":"ok", HTTP 200, and the git hash reaching the output via Version.

Definition of done

fx.WithLogger bridges fx's lifecycle logging into the existing slog logger, so the entire process emits one coherent structured stream. Verify by running the binary non-TTY and confirming every startup and shutdown line is valid JSON.
logger.Identify() is called at startup (from main.go or an fx OnStart), or deleted if genuinely unwanted. Do not leave it dead.
Healthcheck JSON keys are snake_case: uptime_seconds, uptime_human. Rename the type and method to match the conventions doc while you are in the file.
For Sentry and Prometheus, choose one and apply it consistently: either wire them up per the conventions doc, or remove the dead SENTRY_DSN / METRICS_USERNAME / METRICS_PASSWORD config entries so the config surface stops advertising features that do not exist.
- Config that is parsed, documented, and ignored is worse than either extreme — it tells an operator the feature is available when it is not.
- My recommendation: wire up Prometheus, drop Sentry. /metrics behind basic auth is cheap, self-hosted, and immediately useful for a monitoring tool. Sentry adds an external dependency and an outbound network egress to a third party for a service with no users yet — if you want it later it is a small, isolated addition.
- This is a judgement call. State what you chose and why in the PR; if you would rather I escalate it to @sneak first, say so on this issue instead of guessing.
backend/README.md's config table lists exactly the variables that are actually honoured — no more, no fewer. It currently documents 3 of the 6 defined.
Any new dependency is checked against the Go package defaults before being added.
cd backend && make check passes; root make check passes.
TODO.md updated in the same commit.
Commit title ends with (closes #N).

Notes on scope and sequencing

The healthcheck key rename is a breaking change to the response shape. Nothing currently consumes it (see #25 — the frontend does not call the backend at all), so now is the cheapest possible moment to make it.
This issue is deliberately not on the 1.0.0 milestone. It is real compliance debt, but it does not block a first tag, and #25 may reduce its urgency further. Attach it to the milestone if #25 resolves toward shipping the backend.
Do not fold in the Recoverer panic-logging fix — that belongs to #23, which is already in that file.

Implementation requirements

Follow GO_HTTP_SERVER_CONVENTIONS.md for fx wiring, middleware shape, and the metrics route pattern.
make targets only; never raw go invocations.
No attribution trailers in the commit message.

## Problem The backend parses configuration for three observability features that do not exist, and part of its own logging escapes the structured logger. Verified on `main` at `fbfe1df`. ### 1. fx's own logging bypasses slog entirely `backend/cmd/netwatch-server/main.go:29-41` calls `fx.New(...)` with no `fx.WithLogger(...)`. fx therefore falls back to its built-in console logger, writing **unstructured plain text to stderr** for every provide, invoke, and lifecycle hook, plus all shutdown output. `CODE_STYLEGUIDE.md`: "For services/servers, log JSON to stdout... Use structured logging whenever possible." Today a production deployment's log stream is a mix of clean JSON from `internal/logger` and unparseable plain text from fx. Side effect: `go.uber.org/zap v1.26.0` (`go.mod:26`) is linked into the binary purely as fx's default logger backend. No application code imports it. Bridging fx to slog removes zap's runtime role. ### 2. `logger.Identify()` is dead code `internal/logger/logger.go:79-85` defines `Identify()`, which logs appname, version, and buildarch. Grepping the backend finds **only the definition** — no call site. `CODE_STYLEGUIDE_GO.md`: "Embed the git commit hash into the binary and include it **in startup logs** and in health check output." The version does reach the logs by accident via `internal/server/http.go:29-33` ("http begin listen" carries `version` and `buildarch`), but `appname` never does, and the intended identification line never fires. ### 3. Three config values are parsed and never read `internal/config/config.go` loads `SENTRY_DSN`, `METRICS_USERNAME`, and `METRICS_PASSWORD` into the `Config` struct. Nothing reads any of them. `GO_HTTP_SERVER_CONVENTIONS.md` lists these as **mandatory** libraries and specifies the wiring: - `github.com/getsentry/sentry-go` — conditional init on `SENTRY_DSN`, `sentryhttp` middleware with `Repanic: true`, `sentry.Flush(2*time.Second)` on shutdown. - `github.com/prometheus/client_golang` + `github.com/slok/go-http-metrics` + `github.com/99designs/basicauth-go` — `Metrics()` middleware and a basic-auth-protected `GET /metrics`. None are in `go.mod`. `internal/middleware/middleware.go` has no `Metrics()` or `MetricsAuth()`. `internal/server/routes.go` registers no `/metrics` route. The `Server` struct (`server.go:37-47`) omits the `sentryEnabled` field the conventions doc specifies. `backend/README.md:61` already lists Prometheus metrics as an open TODO. ### 4. Healthcheck JSON keys violate the snake_case rule `internal/healthcheck/healthcheck.go:39-40` emits `uptimeHuman` and `uptimeSeconds`. `CODE_STYLEGUIDE_GO.md`: "**Use snake_case for JSON keys.**" `GO_HTTP_SERVER_CONVENTIONS.md` independently specifies `uptime_seconds` and `uptime_human`. Both authorities agree; the code disagrees with both. Also divergent, same file: the type is `Response` rather than `HealthcheckResponse`, the method is `Check()` rather than `Healthcheck()`, and `maintenance_mode` is absent (there is no `MAINTENANCE_MODE` config at all). Correct and not to be regressed: the endpoint path `/.well-known/healthcheck`, `Content-Type: application/json; charset=utf-8`, `"status":"ok"`, HTTP 200, and the git hash reaching the output via `Version`. ## Definition of done - [ ] `fx.WithLogger` bridges fx's lifecycle logging into the existing slog logger, so the entire process emits one coherent structured stream. Verify by running the binary non-TTY and confirming every startup and shutdown line is valid JSON. - [ ] `logger.Identify()` is called at startup (from `main.go` or an fx `OnStart`), or deleted if genuinely unwanted. Do not leave it dead. - [ ] Healthcheck JSON keys are snake_case: `uptime_seconds`, `uptime_human`. Rename the type and method to match the conventions doc while you are in the file. - [ ] For Sentry and Prometheus, choose one and apply it consistently: **either** wire them up per the conventions doc, **or** remove the dead `SENTRY_DSN` / `METRICS_USERNAME` / `METRICS_PASSWORD` config entries so the config surface stops advertising features that do not exist. - Config that is parsed, documented, and ignored is worse than either extreme — it tells an operator the feature is available when it is not. - **My recommendation: wire up Prometheus, drop Sentry.** `/metrics` behind basic auth is cheap, self-hosted, and immediately useful for a monitoring tool. Sentry adds an external dependency and an outbound network egress to a third party for a service with no users yet — if you want it later it is a small, isolated addition. - This is a judgement call. State what you chose and why in the PR; if you would rather I escalate it to @sneak first, say so on this issue instead of guessing. - [ ] `backend/README.md`'s config table lists exactly the variables that are actually honoured — no more, no fewer. It currently documents 3 of the 6 defined. - [ ] Any new dependency is checked against the Go package defaults before being added. - [ ] `cd backend && make check` passes; root `make check` passes. - [ ] `TODO.md` updated in the same commit. - [ ] Commit title ends with ` (closes #N)`. ## Notes on scope and sequencing - The healthcheck key rename is a **breaking change** to the response shape. Nothing currently consumes it (see #25 — the frontend does not call the backend at all), so now is the cheapest possible moment to make it. - This issue is deliberately **not** on the `1.0.0` milestone. It is real compliance debt, but it does not block a first tag, and #25 may reduce its urgency further. Attach it to the milestone if #25 resolves toward shipping the backend. - Do not fold in the `Recoverer` panic-logging fix — that belongs to #23, which is already in that file. ## Implementation requirements - Follow `GO_HTTP_SERVER_CONVENTIONS.md` for fx wiring, middleware shape, and the metrics route pattern. - `make` targets only; never raw `go` invocations. - No attribution trailers in the commit message.

clawbot commented

2026-08-09 11:57:03 +02:00

Warning before anyone adopts `simplelog`: v1.0.0 deadlocks, and the fix tag is unresolvable

This issue notes that CODE_STYLEGUIDE_GO.md mandates importing sneak.berlin/go/simplelog for logging defaults, and that netwatch instead has a hand-rolled internal/logger. I recorded that as a documented conflict with GO_HTTP_SERVER_CONVENTIONS.md, which mandates the hand-rolled package.

Verified: netwatch does not depend on simplelog today. No entry in backend/go.mod or backend/go.sum, no import anywhere in the Go source. So the following does not affect this repo right now — but it directly affects anyone who acts on the styleguide's instruction, which is a live possibility while this issue is open.

The hazard, from elsewhere in the org

simplelog v1.0.0 deadlocks on its first log line in any non-TTY context. For this repo that would be catastrophic and near-invisible in testing: internal/logger already does TTY detection, so a developer running the binary in a terminal would see nothing wrong, and it would hang on first log in Docker, in CI, and under systemd — every context that actually matters.
The fix commit is tagged 1.0.1, without the leading v. Go's module resolver cannot resolve that tag, so the obvious remedy — asking for v1.0.1 — fails.
The working pin is the pseudo-version: v1.0.1-0.20260208172955-9121da9aaed2.
Tracked upstream as simplelog #18 and #19.

What this means for this issue

If the logging half of this issue is ever implemented — swapping internal/logger for simplelog to satisfy CODE_STYLEGUIDE_GO.md — then:

Pin v1.0.1-0.20260208172955-9121da9aaed2 exactly. Do not take v1.0.0, and do not assume v1.0.1 resolves.
Verify the service actually logs in a non-TTY context before considering it done — run the binary in a container with output redirected, not in a terminal. A TTY run will not surface the deadlock.

My standing recommendation is unchanged and this reinforces it: the two authoritative documents conflict, the code currently follows GO_HTTP_SERVER_CONVENTIONS.md, and internal/logger works. There is no reason to churn it. Adopting a dependency whose latest release tag deadlocks in exactly this service's deployment mode, to satisfy a styleguide line that another authoritative document contradicts, would be a poor trade.

Recording it here so the decision is made with the hazard visible rather than discovered afterwards.

## Warning before anyone adopts `simplelog`: v1.0.0 deadlocks, and the fix tag is unresolvable This issue notes that `CODE_STYLEGUIDE_GO.md` mandates importing `sneak.berlin/go/simplelog` for logging defaults, and that netwatch instead has a hand-rolled `internal/logger`. I recorded that as a documented conflict with `GO_HTTP_SERVER_CONVENTIONS.md`, which mandates the hand-rolled package. **Verified: netwatch does not depend on simplelog today.** No entry in `backend/go.mod` or `backend/go.sum`, no import anywhere in the Go source. So the following does not affect this repo right now — but it directly affects anyone who acts on the styleguide's instruction, which is a live possibility while this issue is open. ### The hazard, from elsewhere in the org - **`simplelog v1.0.0` deadlocks on its first log line in any non-TTY context.** For this repo that would be catastrophic and near-invisible in testing: `internal/logger` already does TTY detection, so a developer running the binary in a terminal would see nothing wrong, and it would hang on first log in Docker, in CI, and under systemd — every context that actually matters. - **The fix commit is tagged `1.0.1`, without the leading `v`.** Go's module resolver cannot resolve that tag, so the obvious remedy — asking for `v1.0.1` — fails. - The working pin is the pseudo-version: **`v1.0.1-0.20260208172955-9121da9aaed2`**. - Tracked upstream as simplelog #18 and #19. ### What this means for this issue If the logging half of this issue is ever implemented — swapping `internal/logger` for `simplelog` to satisfy `CODE_STYLEGUIDE_GO.md` — then: - [ ] Pin `v1.0.1-0.20260208172955-9121da9aaed2` exactly. Do **not** take `v1.0.0`, and do not assume `v1.0.1` resolves. - [ ] Verify the service actually logs in a **non-TTY** context before considering it done — run the binary in a container with output redirected, not in a terminal. A TTY run will not surface the deadlock. My standing recommendation is unchanged and this reinforces it: the two authoritative documents conflict, the code currently follows `GO_HTTP_SERVER_CONVENTIONS.md`, and `internal/logger` works. There is no reason to churn it. Adopting a dependency whose latest release tag deadlocks in exactly this service's deployment mode, to satisfy a styleguide line that another authoritative document contradicts, would be a poor trade. Recording it here so the decision is made with the hazard visible rather than discovered afterwards.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/netwatch#27

Backend observability: fx logs bypass slog, Identify() is dead, Sentry/Prometheus config is parsed but unwired #27