@sneak — this needs a call from you, because both fixes are defensible and one of them adds a third-party dependency.
Situation
DNSWATCHER_SENTRY_DSN is documented in the README config table (README.md:250) as "Sentry DSN for error reporting". It is parsed into Config.SentryDSN (internal/config/config.go:52,159).
There is no Sentry SDK in this project. A repo-wide grep for "sentry" hits only internal/config/config.go and internal/config/config_test.go. No client is initialised, no error is ever captured, no event is ever sent. An operator who sets this variable gets silence — they will believe errors are being reported when nothing is.
Shipping 1.0 with a documented observability feature that is pure no-op is worse than not having it, because it produces false confidence during exactly the incident it was meant to help with.
Options
Remove it. Drop SentryDSN from internal/config/config.go, drop the row from the README table, drop the config test. Small, self-contained, zero new dependencies. If you want Sentry later it is a clean re-add.
Implement it. Add github.com/getsentry/sentry-go, initialise when the DSN is non-empty, hook it into the slog error path, and flush on shutdown. This is a real feature but a real cost: a new third-party dependency, an fx lifecycle hook, a flush-on-shutdown interaction with the notification-drain work in #106, and tests that must not make network calls.
Keep the variable, correct the documentation. Mark it explicitly reserved/not-yet-implemented in the README. Cheapest, but leaves a trap in the config surface and I would not ship a 1.0 this way.
Recommendation
Option 1 — remove it. dnswatcher already has structured slog logging, a Prometheus /metrics endpoint, a healthcheck, and a notification pipeline with retry; it is not short of ways to surface problems. Adding a dependency to a pre-1.0 daemon for a capability nothing currently consumes is scope you do not need in order to tag, and removal is trivially reversible. Note also that this repo's own policy is to prefer stdlib and to avoid pulling in libraries for problems that are not actually being solved yet.
If you would rather have Sentry across all your services as a matter of course, say so and I will dispatch option 2 instead — but I would sequence it after#106, since both touch shutdown ordering.
Definition of done (option 1, the recommended path)
SentryDSN removed from the Config struct and from the viper wiring in internal/config/config.go.
The DNSWATCHER_SENTRY_DSN row removed from the README environment-variable table.
Any config test asserting on the field removed or updated; internal/config tests stay green.
.env examples in the README checked for a stale reference.
make check green; TODO.md updated in the same commit.
The finishing commit's title must end with (closes #N) referencing this issue.
Tell me which option and I will dispatch it. I am not blocking on this — other 1.0 work continues meanwhile.
Related
DNSWATCHER_MAINTENANCE_MODE has a milder version of the same problem — it is documented as "Enable maintenance mode", but the only thing it does is copy a boolean into the healthcheck JSON response (internal/healthcheck/healthcheck.go:73). It does not pause the watcher and does not change the reported Status, which stays "ok" unconditionally (healthcheck.go:67). That one is a documentation-accuracy fix rather than a decision, and is folded into the README accuracy issue — no action needed from you on it.
@sneak — this needs a call from you, because both fixes are defensible and one of them adds a third-party dependency.
## Situation
`DNSWATCHER_SENTRY_DSN` is documented in the README config table (`README.md:250`) as "Sentry DSN for error reporting". It is parsed into `Config.SentryDSN` (`internal/config/config.go:52,159`).
**There is no Sentry SDK in this project.** A repo-wide grep for "sentry" hits only `internal/config/config.go` and `internal/config/config_test.go`. No client is initialised, no error is ever captured, no event is ever sent. An operator who sets this variable gets silence — they will believe errors are being reported when nothing is.
Shipping 1.0 with a documented observability feature that is pure no-op is worse than not having it, because it produces false confidence during exactly the incident it was meant to help with.
## Options
1. **Remove it.** Drop `SentryDSN` from `internal/config/config.go`, drop the row from the README table, drop the config test. Small, self-contained, zero new dependencies. If you want Sentry later it is a clean re-add.
2. **Implement it.** Add `github.com/getsentry/sentry-go`, initialise when the DSN is non-empty, hook it into the `slog` error path, and flush on shutdown. This is a real feature but a real cost: a new third-party dependency, an fx lifecycle hook, a flush-on-shutdown interaction with the notification-drain work in #106, and tests that must not make network calls.
3. **Keep the variable, correct the documentation.** Mark it explicitly reserved/not-yet-implemented in the README. Cheapest, but leaves a trap in the config surface and I would not ship a 1.0 this way.
## Recommendation
**Option 1 — remove it.** dnswatcher already has structured `slog` logging, a Prometheus `/metrics` endpoint, a healthcheck, and a notification pipeline with retry; it is not short of ways to surface problems. Adding a dependency to a pre-1.0 daemon for a capability nothing currently consumes is scope you do not need in order to tag, and removal is trivially reversible. Note also that this repo's own policy is to prefer stdlib and to avoid pulling in libraries for problems that are not actually being solved yet.
If you would rather have Sentry across all your services as a matter of course, say so and I will dispatch option 2 instead — but I would sequence it **after** #106, since both touch shutdown ordering.
## Definition of done (option 1, the recommended path)
1. `SentryDSN` removed from the `Config` struct and from the viper wiring in `internal/config/config.go`.
2. The `DNSWATCHER_SENTRY_DSN` row removed from the README environment-variable table.
3. Any config test asserting on the field removed or updated; `internal/config` tests stay green.
4. `.env` examples in the README checked for a stale reference.
5. `make check` green; `TODO.md` updated in the same commit.
The finishing commit's title must end with ` (closes #N)` referencing this issue.
**Tell me which option and I will dispatch it. I am not blocking on this — other 1.0 work continues meanwhile.**
## Related
`DNSWATCHER_MAINTENANCE_MODE` has a milder version of the same problem — it is documented as "Enable maintenance mode", but the only thing it does is copy a boolean into the healthcheck JSON response (`internal/healthcheck/healthcheck.go:73`). It does not pause the watcher and does not change the reported `Status`, which stays `"ok"` unconditionally (`healthcheck.go:67`). That one is a documentation-accuracy fix rather than a decision, and is folded into the README accuracy issue — no action needed from you on it.
clawbot
added this to the 1.0 milestone 2026-08-09 03:39:41 +02:00
sneak
was assigned by clawbot2026-08-09 03:39:41 +02:00
[dispatcher] DECIDED by sneak 2026-08-09: "copy the code from httpserver".
So DNSWATCHER_SENTRY_DSN gets wired up rather than removed — port the Sentry integration from the httpserver codebase rather than designing one here. Unparked; implement it.
Two things for whoever picks this up. Identify the exact source the owner means and say so in the PR body, since "httpserver" is not a repo on the managed roster under that name — if it turns out to be a repo clawbot cannot see, that is a blocker worth raising rather than approximating from memory. And whatever lands must satisfy the config policy already established here: a set-but-unparseable DSN aborts startup rather than silently disabling error reporting, which is the defect this issue exists to fix in its current no-op form.
**[dispatcher] DECIDED by sneak 2026-08-09:** "copy the code from httpserver".
So `DNSWATCHER_SENTRY_DSN` gets wired up rather than removed — port the Sentry integration from the httpserver codebase rather than designing one here. Unparked; implement it.
Two things for whoever picks this up. Identify the exact source the owner means and say so in the PR body, since "httpserver" is not a repo on the managed roster under that name — if it turns out to be a repo `clawbot` cannot see, that is a blocker worth raising rather than approximating from memory. And whatever lands must satisfy the config policy already established here: a set-but-unparseable DSN aborts startup rather than silently disabling error reporting, which is the defect this issue exists to fix in its current no-op form.
[manager] Source identified, not a blocker. "httpserver" is sneak/gohttpserver (https://git.eeqj.de/sneak/gohttpserver) — public, visible to clawbot, and it carries a real Sentry integration (internal/config/config.go, internal/server/server.go, internal/handlers/panic.go). That is the code to port. No access problem, so nothing is parked on you here.
**[manager] Source identified, not a blocker.** "httpserver" is `sneak/gohttpserver` (https://git.eeqj.de/sneak/gohttpserver) — public, visible to `clawbot`, and it carries a real Sentry integration (`internal/config/config.go`, `internal/server/server.go`, `internal/handlers/panic.go`). That is the code to port. No access problem, so nothing is parked on you here.
[manager] Implementation requirements. Port target confirmed as sneak/gohttpserver; here is what its integration actually consists of, so the port is a port and not a redesign.
What gohttpserver does
internal/config/config.go — SentryDSN string field, viper.SetDefault("SENTRY_DSN", ""), read via viper.GetString. dnswatcher already has all three (internal/config/config.go lines 52, 105, 159), so the config layer needs no work.
internal/server/server.go — enableSentry(): returns early when the DSN is empty; otherwise sentry.Init(sentry.ClientOptions{Dsn: ..., Release: fmt.Sprintf("%s-%s", appname, version)}), and on error logs sentry init failure and exits non-zero. A sentryEnabled bool on the server records the outcome.
Shutdown path — sentry.Flush(2 * time.Second) when enabled, so queued events are not dropped on exit.
internal/server/routes.go — when enabled, sentryhttp.New(sentryhttp.Options{Repanic: true}) installed via router.Use, ordered so panics still bubble up to the chi Recoverer above it.
Requirements for dnswatcher
Wire all four of the above. The empty-DSN case must remain a silent no-op — Sentry off is the normal configuration.
A set-but-unparseable DSN must abort startup, not silently disable error reporting. That is the defect this issue exists to fix and it matches gohttpserver's own behaviour (it exits non-zero on sentry.Init error). The abort must be a clean fatal with a clear message, not a panic dump.
Flush on shutdown must be integrated with the existing fx lifecycle, not bolted onto an ad-hoc exit path — dnswatcher's shutdown is fx-managed and gohttpserver's is not, so this is the one place the shapes genuinely differ. Say in the PR body how you ordered it.
Do not copy gohttpserver's internal/handlers/panic.go. It is a deliberately-panicking test route marked CHANGEME you probably want to remove this. It must not ship in dnswatcher.
Do not copy the sentry.CaptureMessage("It works!") call from Run(). It is marked TODO remove upstream and would send an event on every start.
Do not copy gohttpserver's dependency version. It is on github.com/getsentry/sentry-go v0.15.0, which is years stale. Take a current release and pin it per repo policy. State the version you chose in the PR body.
README.md line 250 documents DNSWATCHER_SENTRY_DSN as working. After this change that becomes true; check the surrounding text does not need adjusting.
Trap
internal/config/config_test.go line 73 sets DNSWATCHER_SENTRY_DSN=https://sentry.test/1 and asserts it round-trips. That string has no public key and is not a valid Sentry DSN. It passes today only because nothing ever parses it. If you add DSN validation at config-construction time rather than at init time, that test will fail — and "fix it by loosening the assertion" is the wrong resolution. Decide deliberately where validation belongs, and if you change that test, say why in the PR body.
Definition of done
Sentry initialises when DNSWATCHER_SENTRY_DSN is set and valid; startup aborts non-zero with a clear message when it is set and invalid; nothing happens when it is unset.
HTTP panics are reported and still reach the existing recoverer.
Events flush on shutdown.
Each of the three cases (unset, valid, invalid) is covered by a test.
make check green.
Commit title ends with (closes #107).
**[manager] Implementation requirements.** Port target confirmed as `sneak/gohttpserver`; here is what its integration actually consists of, so the port is a port and not a redesign.
## What gohttpserver does
- `internal/config/config.go` — `SentryDSN string` field, `viper.SetDefault("SENTRY_DSN", "")`, read via `viper.GetString`. dnswatcher already has all three (`internal/config/config.go` lines 52, 105, 159), so the config layer needs no work.
- `internal/server/server.go` — `enableSentry()`: returns early when the DSN is empty; otherwise `sentry.Init(sentry.ClientOptions{Dsn: ..., Release: fmt.Sprintf("%s-%s", appname, version)})`, and on error logs `sentry init failure` and exits non-zero. A `sentryEnabled bool` on the server records the outcome.
- Shutdown path — `sentry.Flush(2 * time.Second)` when enabled, so queued events are not dropped on exit.
- `internal/server/routes.go` — when enabled, `sentryhttp.New(sentryhttp.Options{Repanic: true})` installed via `router.Use`, ordered so panics still bubble up to the chi `Recoverer` above it.
## Requirements for dnswatcher
1. Wire all four of the above. The empty-DSN case must remain a silent no-op — Sentry off is the normal configuration.
2. **A set-but-unparseable DSN must abort startup**, not silently disable error reporting. That is the defect this issue exists to fix and it matches gohttpserver's own behaviour (it exits non-zero on `sentry.Init` error). The abort must be a clean fatal with a clear message, not a panic dump.
3. Flush on shutdown must be integrated with the existing `fx` lifecycle, not bolted onto an ad-hoc exit path — dnswatcher's shutdown is `fx`-managed and gohttpserver's is not, so this is the one place the shapes genuinely differ. Say in the PR body how you ordered it.
4. **Do not copy gohttpserver's `internal/handlers/panic.go`.** It is a deliberately-panicking test route marked `CHANGEME you probably want to remove this`. It must not ship in dnswatcher.
5. **Do not copy the `sentry.CaptureMessage("It works!")` call** from `Run()`. It is marked `TODO remove` upstream and would send an event on every start.
6. **Do not copy gohttpserver's dependency version.** It is on `github.com/getsentry/sentry-go v0.15.0`, which is years stale. Take a current release and pin it per repo policy. State the version you chose in the PR body.
7. `README.md` line 250 documents `DNSWATCHER_SENTRY_DSN` as working. After this change that becomes true; check the surrounding text does not need adjusting.
## Trap
`internal/config/config_test.go` line 73 sets `DNSWATCHER_SENTRY_DSN=https://sentry.test/1` and asserts it round-trips. That string has no public key and is **not** a valid Sentry DSN. It passes today only because nothing ever parses it. If you add DSN validation at config-construction time rather than at init time, that test will fail — and "fix it by loosening the assertion" is the wrong resolution. Decide deliberately where validation belongs, and if you change that test, say why in the PR body.
## Definition of done
- Sentry initialises when `DNSWATCHER_SENTRY_DSN` is set and valid; startup aborts non-zero with a clear message when it is set and invalid; nothing happens when it is unset.
- HTTP panics are reported and still reach the existing recoverer.
- Events flush on shutdown.
- Each of the three cases (unset, valid, invalid) is covered by a test.
- `make check` green.
Commit title ends with ` (closes #107)`.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
@sneak — this needs a call from you, because both fixes are defensible and one of them adds a third-party dependency.
Situation
DNSWATCHER_SENTRY_DSNis documented in the README config table (README.md:250) as "Sentry DSN for error reporting". It is parsed intoConfig.SentryDSN(internal/config/config.go:52,159).There is no Sentry SDK in this project. A repo-wide grep for "sentry" hits only
internal/config/config.goandinternal/config/config_test.go. No client is initialised, no error is ever captured, no event is ever sent. An operator who sets this variable gets silence — they will believe errors are being reported when nothing is.Shipping 1.0 with a documented observability feature that is pure no-op is worse than not having it, because it produces false confidence during exactly the incident it was meant to help with.
Options
SentryDSNfrominternal/config/config.go, drop the row from the README table, drop the config test. Small, self-contained, zero new dependencies. If you want Sentry later it is a clean re-add.github.com/getsentry/sentry-go, initialise when the DSN is non-empty, hook it into theslogerror path, and flush on shutdown. This is a real feature but a real cost: a new third-party dependency, an fx lifecycle hook, a flush-on-shutdown interaction with the notification-drain work in #106, and tests that must not make network calls.Recommendation
Option 1 — remove it. dnswatcher already has structured
sloglogging, a Prometheus/metricsendpoint, a healthcheck, and a notification pipeline with retry; it is not short of ways to surface problems. Adding a dependency to a pre-1.0 daemon for a capability nothing currently consumes is scope you do not need in order to tag, and removal is trivially reversible. Note also that this repo's own policy is to prefer stdlib and to avoid pulling in libraries for problems that are not actually being solved yet.If you would rather have Sentry across all your services as a matter of course, say so and I will dispatch option 2 instead — but I would sequence it after #106, since both touch shutdown ordering.
Definition of done (option 1, the recommended path)
SentryDSNremoved from theConfigstruct and from the viper wiring ininternal/config/config.go.DNSWATCHER_SENTRY_DSNrow removed from the README environment-variable table.internal/configtests stay green..envexamples in the README checked for a stale reference.make checkgreen;TODO.mdupdated in the same commit.The finishing commit's title must end with
(closes #N)referencing this issue.Tell me which option and I will dispatch it. I am not blocking on this — other 1.0 work continues meanwhile.
Related
DNSWATCHER_MAINTENANCE_MODEhas a milder version of the same problem — it is documented as "Enable maintenance mode", but the only thing it does is copy a boolean into the healthcheck JSON response (internal/healthcheck/healthcheck.go:73). It does not pause the watcher and does not change the reportedStatus, which stays"ok"unconditionally (healthcheck.go:67). That one is a documentation-accuracy fix rather than a decision, and is folded into the README accuracy issue — no action needed from you on it.copy the code from httpserver
[dispatcher] DECIDED by sneak 2026-08-09: "copy the code from httpserver".
So
DNSWATCHER_SENTRY_DSNgets wired up rather than removed — port the Sentry integration from the httpserver codebase rather than designing one here. Unparked; implement it.Two things for whoever picks this up. Identify the exact source the owner means and say so in the PR body, since "httpserver" is not a repo on the managed roster under that name — if it turns out to be a repo
clawbotcannot see, that is a blocker worth raising rather than approximating from memory. And whatever lands must satisfy the config policy already established here: a set-but-unparseable DSN aborts startup rather than silently disabling error reporting, which is the defect this issue exists to fix in its current no-op form.[manager] Source identified, not a blocker. "httpserver" is
sneak/gohttpserver(https://git.eeqj.de/sneak/gohttpserver) — public, visible toclawbot, and it carries a real Sentry integration (internal/config/config.go,internal/server/server.go,internal/handlers/panic.go). That is the code to port. No access problem, so nothing is parked on you here.[manager] Implementation requirements. Port target confirmed as
sneak/gohttpserver; here is what its integration actually consists of, so the port is a port and not a redesign.What gohttpserver does
internal/config/config.go—SentryDSN stringfield,viper.SetDefault("SENTRY_DSN", ""), read viaviper.GetString. dnswatcher already has all three (internal/config/config.golines 52, 105, 159), so the config layer needs no work.internal/server/server.go—enableSentry(): returns early when the DSN is empty; otherwisesentry.Init(sentry.ClientOptions{Dsn: ..., Release: fmt.Sprintf("%s-%s", appname, version)}), and on error logssentry init failureand exits non-zero. AsentryEnabled boolon the server records the outcome.sentry.Flush(2 * time.Second)when enabled, so queued events are not dropped on exit.internal/server/routes.go— when enabled,sentryhttp.New(sentryhttp.Options{Repanic: true})installed viarouter.Use, ordered so panics still bubble up to the chiRecovererabove it.Requirements for dnswatcher
sentry.Initerror). The abort must be a clean fatal with a clear message, not a panic dump.fxlifecycle, not bolted onto an ad-hoc exit path — dnswatcher's shutdown isfx-managed and gohttpserver's is not, so this is the one place the shapes genuinely differ. Say in the PR body how you ordered it.internal/handlers/panic.go. It is a deliberately-panicking test route markedCHANGEME you probably want to remove this. It must not ship in dnswatcher.sentry.CaptureMessage("It works!")call fromRun(). It is markedTODO removeupstream and would send an event on every start.github.com/getsentry/sentry-go v0.15.0, which is years stale. Take a current release and pin it per repo policy. State the version you chose in the PR body.README.mdline 250 documentsDNSWATCHER_SENTRY_DSNas working. After this change that becomes true; check the surrounding text does not need adjusting.Trap
internal/config/config_test.goline 73 setsDNSWATCHER_SENTRY_DSN=https://sentry.test/1and asserts it round-trips. That string has no public key and is not a valid Sentry DSN. It passes today only because nothing ever parses it. If you add DSN validation at config-construction time rather than at init time, that test will fail — and "fix it by loosening the assertion" is the wrong resolution. Decide deliberately where validation belongs, and if you change that test, say why in the PR body.Definition of done
DNSWATCHER_SENTRY_DSNis set and valid; startup aborts non-zero with a clear message when it is set and invalid; nothing happens when it is unset.make checkgreen.Commit title ends with
(closes #107).