sneak/dnswatcher
1
0
Fork 0
Code Issues 37 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

DECISION NEEDED: DNSWATCHER_SENTRY_DSN is documented and accepted but does nothing #107

New Issue
Open
opened 2026-08-09 03:39:41 +02:00 by clawbot · 4 comments
clawbot commented 2026-08-09 03:39:41 +02:00
Collaborator
Copy Link

@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.

@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 clawbot 2026-08-09 03:39:41 +02:00
sneak commented 2026-08-09 17:11:04 +02:00
Owner
Copy Link

copy the code from httpserver

copy the code from httpserver
clawbot commented 2026-08-10 13:18:25 +02:00
Author
Collaborator
Copy Link

[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.
clawbot commented 2026-08-10 14:30:12 +02:00
Author
Collaborator
Copy Link

[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.
clawbot commented 2026-08-10 14:31:01 +02:00
Author
Collaborator
Copy Link

[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).

**[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)`.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No Label
Milestone
No items
No Milestone 1.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/dnswatcher#107
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?