Skip to content

ARCHITECTURE.md describes behaviour that does not exist and omits the gzip middleware #115

Description

@LarsLaskowski

Summary

docs/ARCHITECTURE.md is the project's "how it works and why" reference and is cited by CLAUDE.md, AGENTS.md, README.md and the PR template as the document to consult and keep current. Three statements in it are wrong.


1. It claims the dashboard polls /api/v1/alerts — it does not

The "Web dashboard (internal/web)" section states:

The dashboard polls /api/v1/metrics, /api/v1/metrics/history, /api/v1/config, and /api/v1/alerts on the interval /api/v1/config reports...

internal/web/assets/app.js contains no reference to /api/v1/alerts anywhere:

$ grep -n "api/v1" internal/web/assets/app.js
176:      config = await fetchJSON('/api/v1/config');
352:  // configured). The entered key is validated against GET /api/v1/config
369:      await fetchJSON('/api/v1/config', key);
585:      const snap = await fetchJSON('/api/v1/metrics');
602:      const hist = await fetchJSON('/api/v1/metrics/history');

The alert engine is fully implemented server-side and exposed via the API, but it has no dashboard surface at all — that gap is tracked separately as open issue #11 ("C3: Surface alert status in the dashboard UI"). The documentation currently describes issue #11 as though it were already done.

The same sentence continues:

...colors metric cards using the same warn/crit cutoffs the server-side alert engine evaluates against (>=)

That part is accurate — levelClass in app.js uses the same >= cutoffs — but it is describing threshold colouring, not alert state. The two must not be conflated, because it is precisely that conflation that makes the false claim plausible.

Fix: remove /api/v1/alerts from the list of polled endpoints, and state explicitly that the alert engine has no dashboard representation yet, cross-referencing #11. Keep the (correct) note that the card colouring uses the same cutoffs as the engine — that is a genuinely useful design fact.


2. The "HTTP layer" section never mentions withGzip

The section presents the middleware chain as:

withLogging(withSecurityHeaders(mux))

and then describes withSecurityHeaders, withLogging and withAPIKey. withGzip is missing entirely, even though it is on the request path for all four /api/v1/... routes (added in Compress /api/v1 responses with gzip (#95)) and is documented in docs/API.md.

The diagram is also slightly misleading about structure: withLogging and withSecurityHeaders wrap the whole mux, whereas withGzip and withAPIKey are applied per route in New:

mux.Handle("GET /api/v1/metrics", s.withGzip(s.withAPIKey(http.HandlerFunc(s.handleMetrics))))

Listing withAPIKey as a bullet alongside the two global wrappers obscures that distinction — which matters, because it is exactly why /healthz and the static assets are not gated.

Fix: show both layers explicitly, e.g.

global:     withLogging(withSecurityHeaders(mux))
per /api/v1 route:  withGzip(withAPIKey(handler))

and add a withGzip bullet covering: it only engages when the client advertises Accept-Encoding: gzip; it sets Content-Encoding and Vary; the writers come from a sync.Pool because the endpoints are polled every few seconds; and clients that do not negotiate it receive identity responses unchanged, which is what keeps it backwards compatible for naive /api/v1 consumers.


3. "exactly four collaborators" followed by five numbered items

The "Process wiring" section says:

run() is the composition root: it resolves configuration, then constructs and wires together exactly four collaborators before starting anything:

and then lists five (config.Load, alert.NewNotifier, collector.New, web.Handler, httpapi.New).

Fix: change "four" to "five", or drop the count entirely — a count that has to be maintained alongside the list is a trap that will simply break again on the next change. Dropping it is better.


Suggested approach

Three independent edits to one file, safe to do in a single small PR. Read the current source alongside the document rather than trusting the descriptions above — the point of this issue is that the document drifted, so it may have drifted further (or been fixed) since.

Useful verification commands:

grep -n "api/v1" internal/web/assets/app.js
sed -n '/mux := http.NewServeMux()/,/^	}$/p' internal/httpapi/server.go
sed -n '/func run(/,/staticHandler, log)/p' cmd/pimonitor/main.go

While in the file, it is worth a quick scan for other drift against these recently-merged commits, which all changed documented behaviour: #95 (gzip), #96 (shared vcgencmd runner), #97 (asset cache validators), #98 (load gauge thresholds).

Related issues that touch the same sections

Several open issues will also need to edit ARCHITECTURE.md — coordinate to avoid conflicts, or simply land this one first since it is small:

  • The API cache-header issue and the request-throttling issue both add middleware to the "HTTP layer" section.
  • The asset-ETag issue rewrites the "Web dashboard" caching paragraph.
  • The async-persistence issue rewrites the "History persistence" section.

Testing requirements

Per docs/TESTS.md, this is documentation-only and needs no new Go tests — state that explicitly in the PR rather than leaving the checklist ambiguous.

Verification is by reading the source and confirming each corrected statement, using the commands above.

Files to touch

No source changes.


⚠️ Note on this issue

This issue was drafted by an AI code review of the repository. What is written here is not law — the analysis was produced by an AI and may contain mistakes.

Before implementing, first verify that this issue is still factually correct. Implementation may happen considerably later than this issue was written. In particular, if issue #11 has been implemented in the meantime, the statement about the dashboard polling /api/v1/alerts may have become true, and correcting it would then be wrong. Re-read app.js and server.go at their current state and adjust — or close — the issue accordingly.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    documentationImprovements or additions to documentation

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions