Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (16)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🧰 Additional context used📚 Code guidelines (4)📓 Path-based instructions (11)Source excerpt: Write tests for new functionality.📄 CodeRabbit inference engine (CONTRIBUTING.md) Files:
Source excerpt: **Integration test needing Docker** → add a subtest under `tests/integration/` (e.g.📄 CodeRabbit inference engine (docs/src/content/docs/development.md) Files:
Source excerpt: Create `*_test.go` files in the same package as the code under test.📄 CodeRabbit inference engine (AGENTS.md) Files:
See [AGENTS.md](../AGENTS.md) for project conventions, architecture notes, and AI agent instructions.📄 CodeRabbit inference engine (.github/copilot-instructions.md) Files:
Source excerpt: Create the package under `internal/`.📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: **Never hard-wrap prose.📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: **Docs prose**: never hard-wrap Markdown — one paragraph is one line.📄 CodeRabbit inference engine (CONTRIBUTING.md) Files:
Source excerpt: **Go 1.26**, strict formatting (`gofumpt`, enforced by CI)📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: **Formatting**: Code must be formatted with `gofumpt` (a strict superset of `gofmt`).📄 CodeRabbit inference engine (CONTRIBUTING.md) Files:
Source excerpt: **Strict Go formatting**: Use `gofumpt` (a stricter superset of `gofmt`, enforced by CI).📄 CodeRabbit inference engine (docs/src/content/docs/development.md) Files:
Source excerpt: **E2E test via SDK** → add a `tests/e2e/sdk/*.test.ts` file.📄 CodeRabbit inference engine (docs/src/content/docs/development.md) Files:
🧠 Learnings (2)📚 Learning: 2026-06-26T12:23:22.696ZApplied to files:
📚 Learning: 2026-08-11T21:55:53.726ZApplied to files:
🪛 LanguageToolCHANGELOG.md[typographical] ~111-~111: Consider using an em dash in dialogues and enumerations. (DASH_RULE) [style] ~111-~111: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read. (TOO_LONG_SENTENCE) 📝 SummarySummary by CodeRabbit
WalkthroughThe change ties cached ClickHouse HTTP clients to pool tuple release. Pool reconciliation returns released tuples, and application reload wiring schedules client removal after the pool grace period. Ingest workers now use caller-provided client caches. Tests cover cache retention, idle connection closure, and reload behavior. ChangesClickHouse HTTP client lifecycle
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant App
participant Pools
participant HTTPClients
App->>Pools: Reconcile wanted members
Pools-->>App: Return released tuples
App->>HTTPClients: Release each tuple
HTTPClients->>HTTPClients: After grace, remove client and close idle connections
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The client cleanup appears to preserve clients for active tenants, including when a reload reports another pool error. No issue identified here needs resolution before merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 79.07% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 43 functions across 13 files. (3 skipped: 3 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
documentation
|
📚 Docs preview is live → https://a21088a1-wavehouse-docs.wave-rf.workers.dev |
Contributor
Code Coverage OverviewLanguages: Go GoThe overall line coverage in commit 29fbb0b in the Show a line coverage summary of the most impacted files.
Updated |
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
3 of 4 tasks
taitelee
added a commit
that referenced
this pull request
Oct 13, 2026
<!-- claude-projects-attribution: {"github_login":"taitelee"} -->
_Requested by **Taite** · [project
thread](https://claude.ai/code/project/chan_019fe1KaM5pLaH8s2bVcLf4T?thread=cmsg_019fe1KaM5pLaH8s2bVcLf4TMNd2wVPEpUpcYkDWKyvEGU)_
## Summary
Before: the OpenTelemetry resource carried only `service.name` (the
constant `wavehouse`), `OTEL_SERVICE_NAME` and
`OTEL_RESOURCE_ATTRIBUTES` were ignored, `instance_id` was logged once
at boot and nowhere else, and the build was visible only on `/version`.
Two replicas of one role pushing over OTLP were indistinguishable.
After: every span, metric series and OTLP log record carries
`service.version`, `service.instance.id` (the boot config's
`instance_id`), `wavehouse.roles` (sorted), `wavehouse.git_commit` and
`wavehouse.build_time` alongside `service.name`, and the standard env
vars override or extend them (a malformed `OTEL_RESOURCE_ATTRIBUTES` is
logged, never fatal). A new `wavehouse_build_info` gauge (always 1;
`version`, `git_commit`, `build_time`, `roles`; no instance or tenant
label, since the resource carries the instance) shows a mixed-version
rollout. Every log line after the boot config loads carries
`instance_id`, stdout included.
How: `observability.Identity` (new `identity.go`) builds the resource
with `resource.WithFromEnv()` merged last and registers the build-info
gauge on the meter provider; `InitProvider` takes an `Identity` instead
of a service name. `internal/app/wire_identity.go` (new) builds it from
the boot config and build info. Because #712 and #770 edit `wire.go` and
`app.go`, those touches are minimal: `wire.go` changes two lines inside
`wireObservability` (the `InitProvider` argument and the OTLP logger's
`With`), neither in a region those PRs edit; `app.go` drops the
now-duplicate `instance_id` key from the `process roles` line (line
184), separated by an unchanged line from #712's nearest hunk. A `git
merge-tree` against both PR heads showed no new conflicts.
## Test plan
- [x] `make build`, `go vet ./...` (and with `-tags integration`)
- [x] `go test` for `internal/observability`, `internal/app`,
`internal/config`, `cmd/wavehouse`: resource attributes, env override
key by key, empty fields omitted, malformed env non-fatal, Prometheus
scrape shows `wavehouse_build_info` and `target_info` with
`service_instance_id`
- [x] Pre-push code and docs reviewers: `ship_it`
- [ ] `make ci` (not run here: needs Docker; owner runs it locally).
`golangci-lint` could not be downloaded in this environment either.
`tests/integration/otel_test.go` gains build_info/target_info assertions
that run there.
## Related Issues
Closes #772
Part of #777
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_0174TPmnzAJSxK7ej3Q9q1i8
---
_Generated by [Claude
Code](https://claude.ai/code/session_0174TPmnzAJSxK7ej3Q9q1i8)_
---------
Co-authored-by: Claude <noreply@anthropic.com>
This was referenced Oct 13, 2026
taitelee
added a commit
that referenced
this pull request
Oct 14, 2026
<!-- claude-projects-attribution: {"github_login":"taitelee"} -->
_Requested by **Taite** · [project
thread](https://claude.ai/code/project/chan_019fe1KaM5pLaH8s2bVcLf4T?thread=cmsg_019fe1KaM5pLaH8s2bVcLf4T77ZnX42McjAEHbrkWKKt95)_
## Summary
Before: seven metrics carry a `table` label (the issue lists two; the
others are `wavehouse_ingest_retries_total`,
`wavehouse_ingest_poison_total`,
`wavehouse_ingest_dedupe_commit_failed_total`,
`wavehouse_ingest_dedupe_disabled_total` and
`wavehouse_dedupe_hashed_id_total`), so a schema with thousands of
tables becomes thousands of series per metric, and nothing tells the
operator.
After: `prometheus.per_table_metrics: false`
(`WH_METRICS_PER_TABLE=false`) drops the label from all of them, and
each reports one series per remaining label set, summed over every
table. The default stays `true`. With the label on, the process logs one
`WARN` naming the setting once the metrics have carried more than 500
distinct tables. Configuration § Metric labels states which labels a
metric may carry.
How: one view on the meter provider
(`internal/observability/cardinality.go`) matches every `wavehouse_*`
instrument, so no call site changes and it applies to the Prometheus
endpoint and OTLP push alike, which is why the key also shapes OTLP
output. With the knob off the view's attribute filter denies `table`;
with it on, the filter keeps everything and counts distinct table names
until it warns, then drops the set. The convention limits `table` to
counters, up-down counters and histograms: dropping the label from a
gauge would leave one table's value, not a sum, which matters for the
per-table size gauges #417 plans.
`internal/app/wire.go` (also edited by #712 and #770) gets one line:
`AggregateTables: !cfg.Prometheus.PerTableMetrics` in the
`ProviderConfig` literal.
## Test plan
- [x] `make build`, `go vet ./...`
- [x] `go test -race` for `internal/observability`, `internal/config`,
`internal/app`, `internal/stream`, `internal/api`
- [x] New tests: label kept, label dropped and summed, library
instruments untouched, observable up-down counter summed, single warning
past the threshold, end-to-end Prometheus scrape through `InitProvider`;
defaults test covers the `true` default and the env override
- [x] golangci-lint on the changed packages, markdownlint, misspell
- [ ] `make ci` (owner runs it locally). In this sandbox `make verify`
cannot finish: govulncheck gets a 403 from vuln.go.dev, and
golangci-lint flags `jwksRetryMin` at `internal/auth/auth.go:177`, which
fails on main too.
## Related Issues
Closes #538
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01BPFAg9iJ3uHnZdsjncjgKf
---
_Generated by [Claude
Code](https://claude.ai/code/session_01BPFAg9iJ3uHnZdsjncjgKf)_
---------
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
taitelee
added a commit
that referenced
this pull request
Oct 14, 2026
…#843) <!-- claude-projects-attribution: {"github_login":"taitelee"} --> _Requested by **Taite** · [project thread](https://claude.ai/code/project/chan_019fe1KaM5pLaH8s2bVcLf4T?thread=cmsg_019fe1KaM5pLaH8s2bVcLf4TCHrtVnnNfv8VAPYnyiCiq2)_ ## Summary Before: to diagnose memory growth, a goroutine leak or a CPU hotspot in a running process, you had to rebuild it with debug instrumentation. WaveHouse exposed no `net/http/pprof` endpoints. After: with `profiling.enabled: true` (`WH_PROFILING_ENABLED`, default `false`), every process serves Go's pprof endpoints under `/debug/pprof/` on `profiling.addr` (`WH_PROFILING_ADDR`, default `127.0.0.1:6060`). This applies to every process whatever its roles, and the listener is separate from the API port. Covered: heap, CPU, goroutine, allocs, threadcreate, block, mutex, the execution trace, cmdline and symbol. The endpoints are unauthenticated, so the default address is loopback. `kubectl port-forward` still reaches it. Boot logs a `WARN` for any address that is not loopback. `make dev` turns profiling on unless `WH_PROFILING_ENABLED` is already set. How: - `observability.PprofHandler` mounts pprof's handlers on a mux of its own. It is never added to the API router, and nothing serves `http.DefaultServeMux`, where the package's import also registers them. - `internal/app/wire_profiling.go` (new) runs the listener through the existing `serve`. A bind failure is logged and not fatal, the same as the metrics sidecar. - The listener has no `WriteTimeout`, because pprof refuses a `?seconds=` longer than one. - Its `BaseContext` is canceled as the drain begins, so a CPU profile or trace in flight ends then instead of holding the stop for the whole `server.shutdown_timeout`. - `config.Validate` refuses an address that is not `host:port`, and a port that is `server.port` or the enabled Prometheus sidecar's port. **Call site in a shared file:** `internal/app/app.go` gains one line, `a.wireProfiling()`, at the end of `New`. `wire.go` is untouched, to stay clear of #712 and #770. **Not included:** block and mutex profiles return empty, because WaveHouse sets no sampling rate for either. The docs say so. Setting a rate would add a knob and runtime cost, so I left it for a follow-up if someone wants it. Docs: configuration.mdx (new Profiling section, plus the YAML and env examples), deployment.md (new Profiling section with the commands), development.md (`make dev`), architecture.md, AGENTS.md, config.yaml, CHANGELOG. ## Test plan - [x] `make build`, `go vet ./...` - [x] `go test -race` for `internal/config`, `internal/observability` and `internal/app`: config validation and warning cases, the default and zero-value cases, every pprof endpoint served, no other path served, the listener wired only when enabled, pprof returning 404 on the API router whether profiling is on or off, a live listener serving, a 60s CPU profile ending at the stop (this test fails without the shutdown cancel), and a bind failure that is not fatal - [x] golangci-lint v2.11.4 on the touched packages, and markdownlint on the changed docs - [x] Pre-push reviewers: code `ship_it`, docs `ship_it` - [x] GitHub CI green on the head commit ## Related Issues Closes #143 Part of #777 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Q9hHKxkKwmj22yP4Uu8fJR
taitelee
added a commit
that referenced
this pull request
Oct 14, 2026
…#842) <!-- claude-projects-attribution: {"github_login":"taitelee"} --> _Requested by **Taite** · [project thread](https://claude.ai/code/project/chan_019fe1KaM5pLaH8s2bVcLf4T?thread=cmsg_019fe1KaM5pLaH8s2bVcLf4T3xRphsBikbNzJVvACgXsvm)_ ## Summary Before: an ingest request's trace ended at the queue publish. The publisher wrote the request's trace context into every row's queue headers, but the worker's consumer path never read it back (only the `Subscribe` path did), and the worker started no span, so neither the flush nor the ClickHouse `INSERT` could be tied to the request that produced the rows. After: the worker's consumer path extracts the trace context onto each delivered message, in both the embedded and the external NATS broker. Each flush of a batch opens `IngestWorker.Flush`, a root span linked to the span of every request whose rows the batch holds (one batch holds many requests' rows, so links rather than a parent; at most 128 linked, `producers` counting them all), with `tenant`, `table` and `rows` as attributes. Each `INSERT`, row-by-row isolation included, runs under a `clickhouse.insert` child span with the standard database attributes, and the error is recorded when it fails. - New code lives in `internal/ingest/trace.go`; `internal/ingest/worker.go` gets only the two call sites (`flushGroup` and `insertToClickHouse`, the latter now with a named `err` return so the deferred span end sees it), since open PRs #712 and #770 edit that file. `internal/chconn/chconn.go` is untouched. - `internal/mq/embedded.go` and `internal/mq/external_consumer.go` change one line each; the broker conformance suite gains `ConsumeCarriesTheTraceContext` beside the existing `Subscribe` case. - `tenant` rides as a span attribute, never a metric label. The docs note that a flush is a trace of its own, sampled at `otel.traces.sample_rate` independently of the requests it links to. - Query, pipe and ops ClickHouse spans mentioned in the issue are out of scope here. ## Test plan - [x] `internal/ingest/trace_test.go`: link per distinct producer, the 128 cap, the insert span as the flush span's child with its attributes, error recording, and an end-to-end test through the real embedded queue (publish under a span → consumer → flush links → insert child) - [x] `mqtest` conformance: the consumer path carries the trace context on `CreateConsumer`'s ctx (embedded broker run here; external broker runs under the integration tag) - [x] `tests/integration/otel_test.go` `TestOTel_IngestTraceContinuity`: through the wired app with a caller's `traceparent`, the handler span is in the caller's trace, the flush links to it and the insert is the flush's child - [x] `make build`, `go vet ./...`, gofumpt, goimports, markdownlint, misspell; `-race` unit tests for `internal/ingest`, `internal/mq/...`, `internal/observability`, `internal/api` - [x] GitHub CI green on the head commit: Lint, unit, integration (app and backends), e2e, coverage, docs build ## Related Issues Closes #774 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_011Da6nRMNGVuCzVmpf2G2Pi --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
taitelee
added a commit
that referenced
this pull request
Oct 15, 2026
…851) <!-- claude-projects-attribution: {"github_login":"taitelee"} --> _Requested by **Taite** · [project thread](https://claude.ai/code/project/chan_019fe1KaM5pLaH8s2bVcLf4T?thread=cmsg_019fe1KaM5pLaH8s2bVcLf4T8x6JCiRc55QtYXgY1JMBUb)_ ## Summary Before: settings reload outcomes were log lines only. When a tenant's folder was rejected on one process of several, the only way to find it was a log grep. A reload also counted as adopted when the work after it failed, because `AfterAdopt` hooks return nothing: the ClickHouse pool hook only logged its error, and the Pebble dedupe and embedded queue-budget hooks discarded theirs (`_ = reconcile()`). After: every process exports these metrics, none with a tenant label (#538): - `wavehouse_settings_reloads_total{trigger, result}`, where `result` is `adopted`, `partial` or `rejected` - `wavehouse_settings_last_reload_successful` - `wavehouse_settings_last_reload_success_timestamp_seconds` - `wavehouse_settings_tenants{state}` (`served`, `rejected`) - `wavehouse_settings_tenants_removed_total` - `wavehouse_settings_reconcile_failures_total{component}` (`clickhouse`, `dedupe`, `mq`) How it works: - **Hook failures now count.** The three hooks report through the new `settings.Registry.Reconciled(component, err)`, and a failure makes the reload `partial`. A failure while boot applies the settings counts against the boot reload instead. - **Where it lives.** `internal/settings/stats.go` keeps the counts. `internal/app/wire_settings_metrics.go` (new) exports them, and is registered last in `New`, so no collection can see the boot reload's count go down. - **Unchanged.** The response of `POST /v1/ops/settings/reload` is the same as before. **Files shared with #712 and #770.** `internal/app/wire.go` gets one `Reconciled` call per hook (pools, Pebble dedupe, embedded MQ), and `internal/app/app.go` gets one call at the end of `New`. I test-merged both PRs: this change adds no conflicted files beyond the ones they already conflict on. **Skipped local checks.** Taite agreed to skip the git hooks for this PR, and GitHub CI is the gate. In this sandbox `make verify` cannot run vulncheck (a 403 from vuln.go.dev), and golangci-lint was built from source because the official download was refused. That build flags `jwksRetryMin` in `internal/auth/auth.go`, which CI's lint does not flag. `make ci` was not run. The pre-push code and docs reviewers both returned ship_it on 1dc1b93. ## Test plan - [x] `make build`, `go vet ./...` - [x] `go test -race ./internal/settings/ ./internal/app/` (new `stats_test.go` covers flat, nested, hook-failure, boot-apply-failure and per-tenant reloads; `wire_settings_metrics_test.go` covers every series and unregistering) - [x] markdownlint, Biome, misspell, gofumpt, astro check - [ ] GitHub CI ## Related Issues Closes #776. Part of #777. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01FiBgjWxuMqeWqYTi6hqADH --- _Generated by [Claude Code](https://claude.ai/code/session_01FiBgjWxuMqeWqYTi6hqADH)_ --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This was referenced Oct 15, 2026
taitelee
added a commit
that referenced
this pull request
Oct 20, 2026
…855) <!-- claude-projects-attribution: {"github_login":"taitelee"} --> _Requested by **Taite** · [project thread](https://claude.ai/code/project/chan_019fe1KaM5pLaH8s2bVcLf4T?thread=cmsg_019fe1KaM5pLaH8s2bVcLf4TEM2YcBKk4yXuaNGSEiYbKx)_ ## Summary Before: behind a proxy, WaveHouse's logs only had the proxy's address, and the server span's `client.address` was whatever the client wrote first in `X-Forwarded-For` (otelhttp reads the leftmost entry unconditionally). Any client's `traceparent` was adopted as the parent of the request's span. After: `server.trusted_proxies` (`WH_SERVER_TRUSTED_PROXIES`, CIDRs or bare addresses, default empty) lists the proxies to believe. From a listed peer, `X-Forwarded-For` is walked right to left and the first hop that isn't a listed proxy is the client; from anyone else the client is the connection's own address and the header is ignored. The result is `client.address` on every request-scoped log record (operator-key audit line included) and on the server span, replacing otelhttp's value. A `traceparent` is joined only from a listed proxy; from anyone else otelhttp starts a new root linked to it (`WithPublicEndpointFn`). `r.RemoteAddr` is never rewritten. How: `internal/config/proxies.go` parses and validates the list (bad entry refuses boot, a `/0` entry is a boot warning). `internal/api/clientip.go` holds the walk and a `clientAddress` middleware right after `requestID`; `observability.SetRequestClient` stores it on the existing request scope from #771, and `AnnotateSpan`/`TraceHandler` emit it. The otelhttp middleware is now built once per router instead of once per request. **Behavior change:** with the default empty list, a tracing proxy's `traceparent` is now linked rather than continued until the proxy is listed. It has its own `### Changed` CHANGELOG entry. The integration suite now lists loopback as trusted so `TestOTel_IngestTraceContinuity` still joins the caller's trace. Call sites in files open PRs #712/#770 also edit: one line in `internal/app/wire.go` (`deps.TrustedProxies = …`). `wire_ops.go` gets the same line. ## Test plan - [x] `make build`, `go vet ./...` (and `-tags integration ./tests/integration/`) - [x] `go test -race ./internal/api ./internal/config ./internal/observability`; `go test ./internal/app ./internal/auth` - [x] Table tests for the walk: spoofed leftmost entries, trusted-looking spoof left of the client, chained proxies across header lines, all-trusted chain, empty and unreadable hops, ports, bracketed IPv6, IPv4-mapped and zoned peers, non-IP peer - [x] Router test: one `client.address` on the span with the resolved value; traceparent joined from a trusted peer, linked (new root) from an untrusted one - [x] Code and docs pre-push reviewers: ship_it - [x] GitHub CI green: lint, unit, integration (app and backends), e2e, coverage (new files at 100%) ## Related Issues Closes #333 Part of #777. Reviewer noted a possible follow-up outside this scope: `baggage` is still extracted from any peer (pre-existing; nothing reads it today). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01UbPNcxn6XzeNyU3qBMohvK --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
taitelee
added a commit
that referenced
this pull request
Oct 20, 2026
…ls (#866) <!-- claude-projects-attribution: {"github_login":"taitelee"} --> _Requested by **Taite** · [project thread](https://claude.ai/code/project/chan_019fe1KaM5pLaH8s2bVcLf4T?thread=cmsg_019fe1KaM5pLaH8s2bVcLf4TDQK62Tr5rS3frnE6fVLTPT)_ ## Summary The first tier of #94. Before this, the only request metrics came from otelhttp. Those carry no route, so a slow or failing endpoint could not be told apart from the rest. Nothing timed the statements the query paths send to ClickHouse. - `wavehouse_http_request_duration_seconds{route, method, status_class}` times every request on `server.port` except SSE streams. The middleware lives in `newProbeRouter`, so both the API router and the ops-only router get it. It sits outside `jsonRecoverer`, so a panic counts as a `5xx`. `route` is the chi pattern (`/v1/pipes/{name}`), never the path. It is the mount's own pattern (`/v1/*`, `/v1/ops/*`) for a request a mount answers short of a route, and `unmatched` for one that reached no mount. `method` is net/http's or `other`. - `wavehouse_clickhouse_query_duration_seconds{operation, result}` times each statement sent by `POST /v1/query` (`query`), a pipe (`pipe`, `pipe_write`) or `POST /v1/ops/query` (`ops_query`). `result` is `ok`, `canceled` (the client went away), or the response's error code without its `clickhouse.` prefix, through the same `chFailureOf` the response uses. It is recorded inside the singleflight closure, so a cache hit records nothing and coalesced requests record once. - A histogram's `_count` gives both the rate and the error rate, so there is no separate `_errors_total`. Neither metric carries a tenant or table label. - The docs are in deployment.md § Request latency and ClickHouse queries and in architecture.md. The CHANGELOG and AGENTS.md are updated too. This touches `router.go` (one line), which #855 also edits. It should merge after #855. ## Test plan - [x] `internal/api/metrics_test.go` covers: - Route-pattern labeling: pattern not path, `/v1/*`, `/v1/ops/*`, `unmatched`. Panic as 5xx, `/v1/stream` skipped. - The ops router's wiring. - Every row of the ClickHouse error matrix, across `query`, `pipe` and `pipe_write`. - The proxy's ok, rejected and response-cap paths, plus a refused request recording nothing. - Client gone vs. the time-cap backstop. - Nil no-op. - [x] `go test -race ./internal/api/ ./internal/app/` and golangci-lint on `./internal/api/...` - [ ] GitHub CI ## Related Issues Part of #94, part of #777. The worker's `INSERT` timing, the next tier, waits on #770. --- _Generated by [Claude Code](https://claude.ai/code/session_019YVNGPfArHqjc3Xt6N9App)_ --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2 of 3 tasks
taitelee
added a commit
that referenced
this pull request
Oct 21, 2026
…ol (#871) <!-- claude-projects-attribution: {"github_login":"taitelee"} --> _Requested by **Taite** · [project thread](https://claude.ai/code/project/chan_019fe1KaM5pLaH8s2bVcLf4T?thread=cmsg_019fe1KaM5pLaH8s2bVcLf4TSmjopXqBcaTZokt6Yc1ddh)_ ## Summary Nothing exported how much data a table holds in ClickHouse before and after compression. Each collection now sums `system.parts`' active parts per table in every served tenant's database, over that tenant's own pool (`chconn.Pools.For`), and reports: - `wavehouse_clickhouse_uncompressed_bytes{table}`: live data before compression, the closest analogue to bytes ingested - `wavehouse_clickhouse_bytes_on_disk{table}`: compressed data plus marks and index files This is #418 rebuilt for the per-tenant pools. #418 passed one native connection into `RegisterSystemMetrics`, which no longer exists in that shape on `main`. - **One read per database.** Tenants on one address and database (`chconn.Pools.Target`) read the same tables, so that database is read once. Databases are read at once. - **Labels follow #538.** There is no tenant label, so a table name that several databases share reports their sum. The two metrics are observable up-down counters, not gauges, so `prometheus.per_table_metrics: false` folds them into a total. - **All or nothing.** The scrape is bounded at 5s. If any database fails, both metrics are left out of that collection rather than reported stale or short. The failure is logged at `WARN` once, when it starts. - **New wiring in its own file.** It lives in `internal/app/wire_storage_metrics.go`, with one call added in `app.go`, per #777's convention. It leaves `wire.go`, `chconn.go` and `worker.go` alone, so it doesn't collide with #770 or #712. ## Test plan - [x] Unit (`internal/app`): one read per shared database, sums across databases, the 5s bound, one failure fails the scrape, series absent on failure and back on recovery, instruments are non-monotonic sums - [ ] Integration (`tests/integration/storage_metrics_test.go`): real wiring, three tenants over two databases, `/metrics` matches `system.parts` (no Docker locally, so CI runs it) - [x] `golangci-lint` on `internal/app` and `tests/integration`, markdownlint, misspell ## Related Issues Closes #417 Part of #777. Supersedes #418. --- _Generated by [Claude Code](https://claude.ai/code/session_01UMxzS5YSsdAYLPP7hJ4ma6)_ --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The native ClickHouse pools follow the tenants: a tuple no served tenant names is released after a grace. The two HTTP-interface consumers, the ingest worker's inserts and the
/v1/ops/queryproxy, each keep their ownhttp.Clientcache, and nothing ever dropped a client from it. A tuple with atlsblock gets a TLS config of its own each time its pool opens, so once a tenant was removed, moved to another ClickHouse, or rotated its password, the client made for its old tuple stayed for the life of the process with its idle connections open until the transport's own idle timeout. A test that opened and released one tuple five times counted five clients, each still holding its connection after the native pool had closed.Pools.Reconcilealso returns the tuples it released, with the grace each pool closes after, whether the tenants on it were removed or moved to another address, database, user ortlsblock.query_timeoutamong the tenants the tuple had, so a request in flight finishes. A tuple another tenant still names keeps its client; one that opens again gets a fresh one.Runstarts the worker.tlsblock share one client per cache, which stays: nothing in it belongs to one tuple, and a removed tenant's idle plain-HTTP connections ride out the transport's idle timeout on their own.Test plan
tlsclient stays; only idle connections are closed (internal/chconn)internal/app)internal/api,internal/ingest)make ciRelated Issues
Closes #713
Part of #583 (story 6)