feat(observability): metrics, traces, and log ergonomics - #180
feat(observability): metrics, traces, and log ergonomics#180EricAndrechek wants to merge 8 commits into
Conversation
…, log ergonomics Closes the bulk of #94 (Tier S + Tier A). #143 (pprof) is deferred to its own PR; the dev-loop log-format work from #94's follow-up comment ships here. - WH_LOG_FORMAT=auto|text|json with TTY auto-detect; tint-colored text in dev, JSON otherwise. WH_LOG_LEVEL moves into the new logging.* YAML block. - observability.WithComponent(ctx, "<area>") stamped at every HTTP handler entry + the sweeper; TraceHandler injects it onto every record. Top-level attr renamed service=wavehouse so per-process and per-request labels are non-colliding. - Tier S metrics: wavehouse_ingest_duration_seconds (table, outcome), wavehouse_clickhouse_duration_seconds (operation), wavehouse_clickhouse_errors_total (operation, clickhouse_code), wavehouse_http_request_duration_seconds (chi RoutePattern, method, status_class), wavehouse_jetstream_consumer_pending. - Tier A metrics: DLQ depth, schema-validation rejections (typed reasons via discovery.ClassifyValidationError), auth failures (jwt sentinel-based), cache L1 hits/misses (singleflight dimension), dedupe hit/miss, NATS 503. - Trace gaps: schema_validation, jwt_verify, clickhouse.<operation> from executeCHQuery, clickhouse.admin_query on the /v1/admin/query proxy. Raw SQL deliberately omitted from span attributes. - Orchestrator log-tail suppression when an OTel collector responds at WH_OTEL_ADDR (or WH_E2E_LOG_DUMP=0). Default behavior unchanged when no collector is reachable — CI failures still get the 80-line tail. - Centralized cross-cutting instrument registry in internal/observability/instruments.go with must* panic-on-typo helpers. Bento's two pre-existing counters keep their wavehouse_bento_* names. - Log-level audit: per-message bento receive/ack + per-request ingest success demoted from INFO to DEBUG (these were one stdout line per row at scale). - Tests: WithComponent round-trip, TraceHandler component injection, HTTP histogram label set + skip-path coverage, logging.level/format validation, ResolveLogFormat, NewBootstrapLogger. SystemMetricSources struct signature threaded through cmd/wavehouse/main.go + integration otel_test.go updates. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to the observability buildout commit. The Logging YAML block and WH_LOG_FORMAT / WH_LOG_LEVEL env vars are now visible in both surfaces operators read first: `config.yaml` in the repo root (the canonical example) and `deployments/compose/standalone.yaml` (the container-default posture). standalone.yaml pins format=json explicitly so the container runtime's stdout capture hands the log shipper machine-readable lines — matches the deployment.md guidance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two MUSTs from the pre-push-reviewer subagent on the observability buildout: 1. Sweeper logs were missing `component=ingest/sweeper`. The WithComponent on ctx in Start() only affected slog.*Context() calls, but the sweeper uses bare s.logger.X() which slog routes through context.Background(). Fix: pre-tag the logger via .With() in NewSweeper — every record from the sweep loop now carries the field regardless of Context variant. 2. Bento operational logs (per-message receive/reject/ack, per-batch DLQ publish, ClickHouse insert errors) were missing `component=ingest/bento`. The local `bentoLogger` at line 442 only tagged the two lifecycle logs; the high-volume operational paths used bare slog.XContext(msgCtx, ...) with a msgCtx that ExtractNATS produces (trace propagation only, no component). Fix: stamp `WithComponent(msgCtx, "ingest/bento")` once at the top of jsInput.Read, dlqOutput.WriteBatch, and clickhouseOutput.WriteBatch. Plus two SHOULDs: 3. `jwt_verify` span was using `defer span.End()` and then calling next.ServeHTTP, so the span's duration covered the whole downstream request — directly contradicting the comment claiming it covered the verify-only path. Refactored into a `verifyJWT` helper that owns the span lifecycle and ends it before returning to the middleware, regardless of success / failure / role-missing branch. 4. `wavehouse_cache_hits_total` had a `singleflight=true|false` dimension that no caller ever populated as `true` — the L1 cache doesn't see singleflight (handlers do). Removed the dead dimension. Added a separate `wavehouse_query_singleflight_shared_total` counter that fires from the structured-query and pipes handlers when `sf.Do` returns shared=true. Honest split: cache effectiveness vs. coalesce effectiveness. deployment.md metric inventory updated to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Follow-up to the iterate-pass fix that split singleflight out of the cache hits counter into its own wavehouse_query_singleflight_shared_total. The CHANGELOG entry for the observability buildout was written before that split and still advertised the (since-removed) `singleflight` dimension on cache hits. Updates the entry to match what actually shipped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses pre-push-reviewer round 2:
[SHOULD] singleflight counter needs a surface label
- wavehouse_query_singleflight_shared_total now carries
surface=structured_query|pipes. Pre-allocated label sets at file scope
in structured_query.go and pipes.go.
[SHOULD] jwt_verify span panic-safety
- verifyJWT now uses a sentinel-and-defer pattern (`ended bool` + deferred
cleanup) so the span is always finalized — even if jwt.Parse, extractClaim,
or a future library bump panics between Tracer().Start and the explicit
span.End() calls. Each explicit-end path flips `ended = true` to avoid
double-End.
[MAY] dead `dlqOutput.logger` field
- Removed. Every log call in dlqOutput.WriteBatch already uses the
package-level slog.X functions; the field was set in StartIngestWorker
but never read. bento_test.go's struct literals updated.
[MAY] bento ackFn metric context consistency
- `bentoEventsProcessed.Add(...)` now uses the component-stamped msgCtx
rather than Bento's raw ackCtx, so trace_id/span_id propagate onto the
exemplar consistently with every other metric call in the file.
[MAY] no unit test for QuerySingleflightShared
- Investigated and documented in instruments.go: a robust unit test for the
handler's `if shared { Add() }` trigger requires fighting OTel-Go's global
delegating meter, whose instruments list is cleared after the first
SetMeterProvider — repeated swap-and-restore patterns don't re-delegate
to package-init instruments. Same limitation affects the existing
TestHTTPMetricsMiddleware_* and TestRegisterSystemMetrics_* tests; they
pass solo because they're the first MP-swapper in their test order.
Proper coverage needs either a TestMain-installed shared MeterProvider or
a refactor to inject counters at handler-construction time — both bigger
than this PR's scope. Tracked as a TODO at the instrument declaration.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three small [MAY]s from round 3 of pre-push review: - Remove dead DLQHandler.Logger field (mirror of the dlqOutput.logger cleanup from round 2). NewDLQHandler signature drops the logger arg; cmd/wavehouse/main.go, tests/integration/setup_test.go, and internal/api/dlq_test.go updated. - Split auth.failure_reason and auth.role_present span attributes in verifyJWT. The missing-role case is a successful JWT verify but no role claim — previously it set auth.failure_reason="missing_role_claim" on a span that succeeded, which would misleadingly classify the span as a failure in trace queries filtering on presence of that attribute. The attribute is now auth.role_present bool — clean dimension for "did we find a role" without polluting the failure-reason field. The counter still records reason=missing_role_claim so the 403-distinguishability is preserved. - Bento dlqOutput.WriteBatch metric paths now use msgCtx instead of the un-stamped batch ctx, mirroring the round-2 fix to the ack closure. bentoDLQDropped and recordIngestDuration on both the DLQ-success and DLQ-publish-failure paths now carry trace_id/span_id and the bento component on metric exemplars. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three more [MAY]s from round 4: - executeCHQuery's mid-stream error paths (rows.Scan failure, rows.Err iteration error) now bump wavehouse_clickhouse_errors_total and span.RecordError. Without this, network blips after the query started executing under-counted on dashboards filtering on the errors counter. - query.go admin proxy's response-read failure and oversized-body paths now bump the errors counter too. The oversize path uses clickhouse_code="caller_oversize" — distinct from "0" (transport drop) so dashboards can separate caller-fault from upstream-fault without losing visibility. - jsInput.Read's early-reject paths (empty table_name, empty payload) now record wavehouse_ingest_duration_seconds with outcome=dropped, using a parsed receive timestamp from raw.ReceivedTimestamp. Closes the gap where the histogram's documented outcome=dropped was only emitted from the DLQ-publish-failure path. Invalid-JSON path is still excluded — no timestamp is parseable when the unmarshal itself failed. Empty-table-name uses sentinel table="_unknown" to keep the label bounded. New recordIngestDurationFromTS helper takes a parsed time directly; recordIngestDuration now wraps it. - deployment.md updated to document the auth.role_present / auth.failure_reason semantic split landed in round 3, plus auth.role and auth.role_claim attributes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
documentation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughConsolidates observability: adds centralized instruments, configurable console logging with per-request component tags, HTTP latency middleware, handler and ClickHouse tracing/metrics, system-metrics sources, config/bootstrap changes, DLQ/DLQ tests, bento ingest metrics, docs, and orchestrator log-dump suppression. ChangesObservability Buildout
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the observability of the WaveHouse system by introducing structured logging, comprehensive metric instrumentation, and improved distributed tracing. It addresses key gaps in system visibility by adding latency histograms, error counters, and component-aware logging, while also streamlining developer experience through log format auto-detection and intelligent log-tail suppression in CI. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/api/pipes.go (1)
97-111:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
AllowedRolescheck is fail-open when role is empty.The
if role != ""guard skips allowlist enforcement entirely for missing roles. Also,ar == roleallows a malformed empty allowlist entry ("") to match an empty role.Based on learnings: "For WaveHouse pipes authorization allowlist checks, fix the empty-role fail-open behavior by removing `if role != \"\"` guards and requiring non-empty allowlist entries to match (`ar != \"\" && ar == role`)."🔒 Suggested fix
if len(q.AllowedRoles) > 0 { role := RoleFromContext(r.Context()) - if role != "" { - allowed := false - for _, ar := range q.AllowedRoles { - if ar == role || ar == "*" { - allowed = true - break - } - } - if !allowed { - writeJSONError(w, http.StatusForbidden, "forbidden") - return - } + allowed := false + for _, ar := range q.AllowedRoles { + if ar == "*" || (ar != "" && ar == role) { + allowed = true + break + } + } + if !allowed { + writeJSONError(w, http.StatusForbidden, "forbidden") + return } }internal/ingest/sweeper.go (1)
30-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProtect
NewSweeperfrom nil logger panics.Line [41] dereferences
loggerunconditionally. A nil logger from any caller will panic during construction.Proposed fix
func NewSweeper(js jetstream.JetStream, gapWindow time.Duration, logger *slog.Logger) *Sweeper { + if logger == nil { + logger = slog.Default() + } return &Sweeper{ js: js, gapWindow: gapWindow, logger: logger.With("component", "ingest/sweeper"), } }As per coding guidelines,
**/*.go: “Return errors instead of panicking; wrap errors with fmt.Errorf("context: %w", err).”
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d140a2d2-5679-4f18-b827-98d3223ed1f0
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
AGENTS.mdCHANGELOG.mdcmd/wavehouse/main.goconfig.yamldeployments/compose/standalone.yamldocs/src/content/docs/configuration.mddocs/src/content/docs/deployment.mdgo.modinternal/api/clickhouse_exec.gointernal/api/clickhouse_exec_test.gointernal/api/dlq.gointernal/api/dlq_test.gointernal/api/ingest.gointernal/api/metrics_middleware.gointernal/api/metrics_middleware_test.gointernal/api/middleware.gointernal/api/pipes.gointernal/api/query.gointernal/api/router.gointernal/api/stream_sse.gointernal/api/stream_ws.gointernal/api/structured_query.gointernal/cache/local.gointernal/config/config.gointernal/config/config_test.gointernal/discovery/validation.gointernal/ingest/bento.gointernal/ingest/bento_test.gointernal/ingest/sweeper.gointernal/observability/instruments.gointernal/observability/logger.gointernal/observability/logger_test.gointernal/observability/metrics.gointernal/observability/metrics_test.goscripts/orchestrator/main.gotests/integration/otel_test.gotests/integration/setup_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (8)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Go 1.26 with strict formatting via gofumpt, enforced by CI
Use structured logging with log/slog (JSON handler)
Return errors instead of panicking; wrap errors with fmt.Errorf("context: %w", err)
Use constructor injection for dependencies; do not use global state
Every new function should have corresponding test cases; run make lint and make test before considering work complete
Use make build TAGS="foo bar" for conditional build tags
Files:
tests/integration/setup_test.gointernal/api/stream_ws.gointernal/discovery/validation.gointernal/api/router.gointernal/api/dlq.gointernal/ingest/sweeper.gointernal/api/stream_sse.gointernal/api/metrics_middleware_test.gointernal/api/metrics_middleware.goscripts/orchestrator/main.gointernal/cache/local.gointernal/api/pipes.gointernal/api/structured_query.gotests/integration/otel_test.gointernal/config/config_test.gointernal/api/clickhouse_exec_test.gointernal/observability/metrics_test.gointernal/observability/metrics.gointernal/api/middleware.gointernal/ingest/bento_test.gointernal/api/query.gointernal/api/ingest.gointernal/observability/instruments.gointernal/observability/logger.gocmd/wavehouse/main.gointernal/api/dlq_test.gointernal/api/clickhouse_exec.gointernal/config/config.gointernal/ingest/bento.gointernal/observability/logger_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Use table-driven tests with t.Run(tt.name, ...) pattern for test cases
Use shared mocks from internal/testutil/ (MockPublisher, MockCache, MockDeduplicator, MockSubscriber) instead of ad-hoc mocks
Aim for 80%+ coverage on new code; project-wide CI-enforced minimum is 80% (merged unit + integration + e2e)
Use discovery.NewSchemaRegistryFromMap(tables) or testutil.NewTestSchemaRegistry(tables) for schema-aware tests
Create *_test.go files in the same package as the code under test
Files:
tests/integration/setup_test.gointernal/api/metrics_middleware_test.gotests/integration/otel_test.gointernal/config/config_test.gointernal/api/clickhouse_exec_test.gointernal/observability/metrics_test.gointernal/ingest/bento_test.gointernal/api/dlq_test.gointernal/observability/logger_test.go
tests/integration/**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Integration tests must use //go:build integration and ClickHouse testcontainer
Files:
tests/integration/setup_test.gotests/integration/otel_test.go
internal/api/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Chi v5 for HTTP routing
Inject trace_id/span_id from active spans and per-request component via observability.WithComponent(ctx, "") in logs
Never use cookie-based auth or Access-Control-Allow-Credentials; WaveHouse uses Bearer-token-only CORS posture
All /v1/* routes must be behind optional JWT auth middleware
Input JSON must be validated against ClickHouse schemas before processing
Files:
internal/api/stream_ws.gointernal/api/router.gointernal/api/dlq.gointernal/api/stream_sse.gointernal/api/metrics_middleware_test.gointernal/api/metrics_middleware.gointernal/api/pipes.gointernal/api/structured_query.gointernal/api/clickhouse_exec_test.gointernal/api/middleware.gointernal/api/query.gointernal/api/ingest.gointernal/api/dlq_test.gointernal/api/clickhouse_exec.go
internal/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use lowercase, single-word (or abbreviated) package names; internal/ enforces module privacy
Files:
internal/api/stream_ws.gointernal/discovery/validation.gointernal/api/router.gointernal/api/dlq.gointernal/ingest/sweeper.gointernal/api/stream_sse.gointernal/api/metrics_middleware_test.gointernal/api/metrics_middleware.gointernal/cache/local.gointernal/api/pipes.gointernal/api/structured_query.gointernal/config/config_test.gointernal/api/clickhouse_exec_test.gointernal/observability/metrics_test.gointernal/observability/metrics.gointernal/api/middleware.gointernal/ingest/bento_test.gointernal/api/query.gointernal/api/ingest.gointernal/observability/instruments.gointernal/observability/logger.gointernal/api/dlq_test.gointernal/api/clickhouse_exec.gointernal/config/config.gointernal/ingest/bento.gointernal/observability/logger_test.go
internal/api/**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Use testutil.MakeJWT(t, claims) and testutil.MakeExpiredJWT(t, claims) for auth tests
Use testutil.AssertJSONResponse(t, rec, status, expected) and testutil.AssertJSONContains(t, rec, status, substring) for HTTP handler assertions
Files:
internal/api/metrics_middleware_test.gointernal/api/clickhouse_exec_test.gointernal/api/dlq_test.go
internal/observability/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
The slog logger stdout output is always 100% — never sample stdout, only OTLP push; WARN+ERROR records always export at 100%
Use a private prometheus.Registry in the OTel Prometheus exporter to avoid leaking process/Go collectors
Wire OpenTelemetry trace/metric/log providers via OTLP gRPC with each signal independently gated; gRPC exporters dial lazily so unreachable collectors never block startup
Files:
internal/observability/metrics_test.gointernal/observability/metrics.gointernal/observability/instruments.gointernal/observability/logger.gointernal/observability/logger_test.go
internal/config/config.go
📄 CodeRabbit inference engine (AGENTS.md)
Add config struct fields with yaml, env, and env-default tags in internal/config/config.go
Files:
internal/config/config.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Use V=1 make test for verbose test output; use make test ARGS="-run TestFoo" for specific tests
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Core behaviors must be defined as Go interfaces (Cache, Deduplicator, Publisher, Subscriber); standalone and clustered modes use different implementations
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Document new API endpoints in docs/src/content/docs/api.md and README.md
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Document new config options in docs/src/content/docs/configuration.md, config.yaml, and compose env blocks
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Document architecture changes and new packages in docs/src/content/docs/architecture.md and AGENTS.md
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Document notable changes in CHANGELOG.md under [Unreleased] section
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Keep config struct tags in internal/config/config.go synchronized with docs/src/content/docs/configuration.md, config.yaml, and compose env blocks
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Keep EventMessage JSON tags synchronized with docs/src/content/docs/api.md event format, SSE/WS examples, and ClickHouse INSERT columns
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Keep route registrations in internal/api/router.go synchronized with docs/src/content/docs/api.md endpoint list
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Keep handler error responses synchronized with docs/src/content/docs/api.md error tables
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Grep for identifiers you touched (field names, env var names, endpoint paths) across docs to catch staleness after changes
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Update TypeScript SDK in clients/ts/ when backend changes alter the public API surface
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Add a matching area/<pkg> repo label when adding a new internal package (e.g., area/foo for internal/foo/)
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Never use git push --no-verify or git commit --no-verify unless explicitly intentional (WIP/draft pushes)
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Validate locally before pushing by running make ci; every push consumes shared CI capacity
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Keep Conventional Commits type list in CONTRIBUTING.md synchronized with the regex in housekeeping.yml
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Create PRs with gh pr create --draft only for AI agents; only humans transition draft → ready-for-review
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: Merge upstream main with git merge --no-edit (not rebase) when syncing PR branches; force-push is blocked
Learnt from: CR
Repo: Wave-RF/WaveHouse
Timestamp: 2026-05-24T19:44:39.639Z
Learning: JWT secret must be cryptographically strong when auth is enabled in production
📚 Learning: 2026-05-20T01:02:00.784Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 164
File: internal/api/router_test.go:289-350
Timestamp: 2026-05-20T01:02:00.784Z
Learning: In WaveHouse’s internal API tests (files matching internal/api/**/*_test.go), follow the existing separation-of-concerns convention for testing the RequireRole middleware: inject `ContextKeyRole` directly into the request `context.Context` instead of using `testutil.MakeJWT`/JWT-driven flows. Do not refactor role-gate tests to use JWT tokens—JWT parsing and token handling are covered separately in `middleware_test.go` (the dedicated JWT parsing tests), and mixing those concerns would expand the failure surface and reduce isolation.
Applied to files:
internal/api/metrics_middleware_test.gointernal/api/clickhouse_exec_test.gointernal/api/dlq_test.go
📚 Learning: 2026-05-23T01:23:59.268Z
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse PR: 174
File: internal/api/ingest_test.go:111-111
Timestamp: 2026-05-23T01:23:59.268Z
Learning: In WaveHouse Go tests in internal/api/**/*_test.go, use internal/testutil.AssertJSONErrorResponse(t, w) for HTTP error-path JSON assertions. Do not use (or reintroduce) package-local assertJSONErrorResponse helpers. AssertJSONErrorResponse verifies the response Content-Type is application/json, includes the X-Content-Type-Options: nosniff header, and that the JSON body contains an "error" field.
Applied to files:
internal/api/metrics_middleware_test.gointernal/api/clickhouse_exec_test.gointernal/api/dlq_test.go
📚 Learning: 2026-05-20T20:30:15.808Z
Learnt from: taitelee
Repo: Wave-RF/WaveHouse PR: 172
File: internal/api/pipes_test.go:106-118
Timestamp: 2026-05-20T20:30:15.808Z
Learning: For WaveHouse pipes authorization allowlist checks, fix the empty-role fail-open behavior by (1) removing any outer guard that prevents allowlist evaluation when the incoming `role` is `""` (e.g., don’t short-circuit with `if role != "" { ... }`), and (2) during allowlist scanning, ensure only non-empty allowlist entries can match—e.g., require `ar != "" && ar == role` (so a malformed allowlist like `["" ]` cannot grant access to an empty incoming role via `"" == ""`).
Applied to files:
internal/api/pipes.go
🔇 Additional comments (31)
AGENTS.md (1)
24-24: LGTM!CHANGELOG.md (1)
10-22: LGTM!docs/src/content/docs/configuration.md (1)
140-147: LGTM!docs/src/content/docs/deployment.md (1)
371-399: LGTM!tests/integration/otel_test.go (1)
136-136: LGTM!Also applies to: 246-246, 287-287
tests/integration/setup_test.go (1)
326-326: LGTM!internal/observability/metrics.go (2)
15-24: LGTM!
44-126: LGTM!internal/observability/metrics_test.go (1)
47-48: LGTM!Also applies to: 71-71, 100-100
go.mod (1)
26-26: LGTM!internal/observability/logger.go (1)
18-193: LGTM!internal/config/config.go (1)
30-53: LGTM!Also applies to: 201-214
internal/config/config_test.go (1)
43-46: LGTM!Also applies to: 48-121
cmd/wavehouse/main.go (1)
53-60: LGTM!Also applies to: 90-109, 140-140, 243-250, 351-351
config.yaml (1)
20-25: LGTM!deployments/compose/standalone.yaml (1)
31-35: LGTM!internal/api/metrics_middleware.go (1)
26-57: LGTM!internal/api/metrics_middleware_test.go (1)
21-67: LGTM!internal/api/router.go (1)
86-90: LGTM!internal/api/middleware.go (1)
18-36: LGTM!Also applies to: 103-121, 123-223
internal/api/ingest.go (1)
40-56: LGTM!Also applies to: 82-95, 141-161, 189-207
internal/api/pipes.go (1)
20-24: LGTM!Also applies to: 88-89, 154-186
internal/api/structured_query.go (1)
24-25: LGTM!Also applies to: 56-57, 136-173
internal/api/query.go (1)
23-36: LGTM!Also applies to: 146-147, 245-387
internal/api/stream_sse.go (1)
12-12: LGTM!Also applies to: 33-33
internal/api/stream_ws.go (1)
54-54: LGTM!internal/api/clickhouse_exec.go (1)
47-131: LGTM!internal/api/clickhouse_exec_test.go (1)
144-145: LGTM!Also applies to: 156-157, 182-183
internal/api/dlq.go (1)
15-20: LGTM!Also applies to: 25-26
internal/api/dlq_test.go (1)
22-23: LGTM!Also applies to: 62-63, 94-95
internal/ingest/bento_test.go (1)
93-94: LGTM!Also applies to: 109-110, 125-126, 141-142, 369-370, 384-385
github-project-automation
Bot
moved this from Backlog
to In review
in WaveHouse Task Board
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive observability buildout, introducing Tier S and Tier A metrics, enhanced tracing across ingest and query paths, and improved logging ergonomics with support for multiple formats and per-component tagging. The changes include a centralized instrument registry and automated JetStream metric probes. Feedback highlights a critical typo using a non-existent errors.AsType function, a recommendation to record database latency before row scanning to avoid including application processing time, and suggestions to better align with OpenTelemetry semantic conventions and simplify redundant span management logic in the JWT middleware. Verdict: Iterate
EricAndrechek
changed the title
- pipes auth: empty role no longer short-circuits the allowlist; empty allowlist entries can't match an empty role (CodeRabbit outside-diff). - ingest: unknown_table rejections collapse `table` label to a constant so a flood of bogus names can't blow up cardinality. - bento: skip ingest-duration record on negative latency (clock skew); record ClickHouseDuration on request-construction failure for metric completeness. - orchestrator: TrimSpace WH_OTEL_ENABLED; new unit tests for suppressLogDumpReason covering each env combination + reachable probe. - middleware: drop the `ended`+defer dance for jwt_verify; a plain `defer span.End()` is correct here since verifyJWT is synchronous. - metrics: fix stale comment on SystemMetricSources.StreamName (it only gates the consumer-pending probe, not DLQ). - tests: add table-driven coverage for clickhouseDriverErrCode, ClassifyValidationError, clickhouseErrCode, parseReceivedTimestamp, and the recordIngestDurationFromTS skip paths (CodeRabbit nits). - comments: aggressive cull across all touched files — strip narrative WHAT-the-code-does prose, keep concise WHY notes for non-obvious constraints. CHANGELOG entry collapsed to the same shape. Net effect: PR delta drops from +1474/-163 to +1507/-487. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai Following up on the two outside-diff findings in your initial review:
Re-running Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com |
|
Triggering the re-review now. ✏️ Learnings added
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/middleware.go (1)
174-186:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReconsider counting
missing_role_claimas an auth failure.A successfully verified token with a missing role claim isn't an authentication failure—it's a valid JWT that lacks authorization data. Counting it via
AuthFailuresconflates authn and authz, potentially misleading dashboards. The span attributeauth.role_present=falseis appropriate, but the counter increment may inflate failure rates for tokens that are cryptographically valid.Consider either:
- Using a separate counter (e.g.,
wavehouse_auth_missing_role_total), or- Removing the counter bump here and relying on downstream 403 metrics from
RequireRole.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5c4a6dce-9f9d-437a-8248-052b23a50e2f
📒 Files selected for processing (25)
CHANGELOG.mdcmd/wavehouse/main.godocs/src/content/docs/deployment.mdinternal/api/clickhouse_exec.gointernal/api/clickhouse_exec_test.gointernal/api/ingest.gointernal/api/metrics_middleware.gointernal/api/metrics_middleware_test.gointernal/api/middleware.gointernal/api/pipes.gointernal/api/query.gointernal/api/structured_query.gointernal/cache/local.gointernal/config/config.gointernal/discovery/discovery_test.gointernal/discovery/validation.gointernal/ingest/bento.gointernal/ingest/bento_test.gointernal/ingest/sweeper.gointernal/observability/instruments.gointernal/observability/logger.gointernal/observability/logger_test.gointernal/observability/metrics.goscripts/orchestrator/main.goscripts/orchestrator/main_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (11)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Go 1.26 with strict formatting enforced by gofumpt
Use structured logging with log/slog (JSON handler)
Return errors, don't panic. Wrap with fmt.Errorf("context: %w", err)
No global state: dependencies are passed explicitly (constructor injection)
Package naming: lowercase, single word (or abbreviated). internal/ enforces module privacy
Files:
scripts/orchestrator/main_test.gointernal/ingest/sweeper.gointernal/api/structured_query.gointernal/api/metrics_middleware_test.goscripts/orchestrator/main.gointernal/cache/local.gointernal/discovery/validation.gointernal/discovery/discovery_test.gointernal/api/pipes.gointernal/api/metrics_middleware.gointernal/api/middleware.gointernal/config/config.gocmd/wavehouse/main.gointernal/observability/metrics.gointernal/observability/instruments.gointernal/observability/logger_test.gointernal/api/clickhouse_exec_test.gointernal/api/ingest.gointernal/ingest/bento_test.gointernal/ingest/bento.gointernal/api/clickhouse_exec.gointernal/observability/logger.gointernal/api/query.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Use table-driven tests with tests := []struct{ name string; ... } and t.Run(tt.name, ...)
Use shared mocks from internal/testutil/ (MockPublisher, MockCache, MockDeduplicator, MockSubscriber) instead of ad-hoc mocks
Files:
scripts/orchestrator/main_test.gointernal/api/metrics_middleware_test.gointernal/discovery/discovery_test.gointernal/observability/logger_test.gointernal/api/clickhouse_exec_test.gointernal/ingest/bento_test.go
internal/ingest/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Ingest pipeline is insert-only: non-insert mutations (DELETE/UPDATE/TRUNCATE) must go through POST /v1/admin/query under admin/service role
Async ingestion: ingest returns 200 immediately after optional dedup + MQ publish. ClickHouse writes happen asynchronously via Bento ingest pipeline. If NATS stream full, returns 503 + Retry-After
Per-table batching: Bento ingest pipeline groups events by table name and performs dynamic INSERTs using schema's column order. Each table's batch is independent
Dead Letter Queue: failed batch inserts published to separate NATS stream (WAVEHOUSE_DLQ) with subjects dlq.
. Prevents silent data loss. Controlled by dlq.enabledActive Sweeper: NATS messages retained for SSE/WS gap-fill. Sweeper purges messages both ACKed (written to ClickHouse) and older than gap window. Gap-fill uses NATS DeliverByStartTime
Files:
internal/ingest/sweeper.gointernal/ingest/bento_test.gointernal/ingest/bento.gointernal/api/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Use Chi v5 for HTTP routing
Files:
internal/api/structured_query.gointernal/api/metrics_middleware_test.gointernal/api/pipes.gointernal/api/metrics_middleware.gointernal/api/middleware.gointernal/api/clickhouse_exec_test.gointernal/api/ingest.gointernal/api/clickhouse_exec.gointernal/api/query.go**/internal/api/**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Use testutil.MakeJWT(t, claims) and testutil.MakeExpiredJWT(t, claims) for auth tests
Use testutil.AssertJSONResponse(t, rec, status, expected) and testutil.AssertJSONContains(t, rec, status, substring) for response assertions
Files:
internal/api/metrics_middleware_test.gointernal/api/clickhouse_exec_test.gointernal/cache/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Singleflight: TieredCache uses golang.org/x/sync/singleflight to prevent cache stampede
Files:
internal/cache/local.go**/internal/discovery/**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
Use testutil.NewTestSchemaRegistry(tables) or discovery.NewSchemaRegistryFromMap(tables) for schema-aware tests
Files:
internal/discovery/discovery_test.goCHANGELOG.md
📄 CodeRabbit inference engine (AGENTS.md)
Update CHANGELOG.md under [Unreleased] for any notable change
Files:
CHANGELOG.mddocs/src/content/docs/deployment.md
📄 CodeRabbit inference engine (AGENTS.md)
Update docs/src/content/docs/deployment.md and compose files when changing deployment or Docker
Files:
docs/src/content/docs/deployment.mdinternal/config/config.go
📄 CodeRabbit inference engine (AGENTS.md)
Add config struct fields to internal/config/config.go with yaml, env, and env-default tags
Files:
internal/config/config.gointernal/observability/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Observability invariants: stdout always 100%, slog fans out to stdout AND OTLP, sampling applies only to OTLP push, WARN+ERROR always export at 100%
When changing logger, sampler, or provider wiring, preserve observability invariants: stdout 100%, sampling only on OTLP, WARN+ERROR at 100%
OTel Prometheus exporter uses a private prometheus.Registry to avoid leaking process/Go collectors into /metrics output
Files:
internal/observability/metrics.gointernal/observability/instruments.gointernal/observability/logger_test.gointernal/observability/logger.go🧠 Learnings (6)
📓 Common learnings
Learnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Every new function should have corresponding test cases. Run make lint and make test before considering work completeLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Aim for 80%+ coverage on new code. Project-wide CI-enforced minimum is 80% (merged unit + integration + e2e)Learnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Per-suite coverage minima: unit 70%, integration 12%, e2e 50%, sdk 50%Learnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Config struct tags in internal/config/config.go must agree with docs/src/content/docs/configuration.md, config.yaml, and compose env blocksLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: EventMessage JSON tags must agree with docs/src/content/docs/api.md event format, SSE/WS examples, and ClickHouse INSERT columnsLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Route registrations in router.go must agree with docs/src/content/docs/api.md endpoint listLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Handler error responses must agree with docs/src/content/docs/api.md error tablesLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Validate locally before pushing using make ci. Don't use CI as your first feedback loopLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: git hooks installed via make tools apply: .githooks/pre-commit runs make verify (~30s); .githooks/pre-push checks for tmp/ci-passed-<HEAD-sha> markerLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: When delegating to a subagent, tell them explicitly 'run locally first.' Agents default to 'commit and let CI run' because it looks like progressLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Every review comment gets a substantive reply, and every thread gets resolved before mergeLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: For review response: decide (accept/push back/defer), reply substantively, mention the bot, fix in this PR or link tracking issue, resolve the thread, re-request review from humans after substantive changesLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Do not argue in circles in code reviews. If the reviewer repeats the same point, escalate to a maintainer rather than loopingLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Do not resolve a thread that has an open child commentLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Sync a PR branch with main using merge, not rebase: git fetch origin main && git merge origin/main --no-edit && git pushLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Force-pushes are blocked by .claude/settings.json. Use merge instead of rebase to preserve review-thread anchorsLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Agents must create PRs with --draft flag. Only humans transition draft → ready-for-reviewLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Adding/removing human reviewers via gh pr edit is blocked for agents. housekeeping.yml auto-assigns the non-author adminLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Agents can re-request bot reviewers by mentioning them in PR comments: claude, /review, gemini-code-assist, /gemini review, coderabbitai reviewLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Before pushing to any branch with an open PR, agents must invoke pre-push-reviewer subagent. Verdict must be 'ship_it' with zero findings at any severityLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Agents cannot use git push --no-verify or git commit --no-verify. Markers are written exclusively by make ci (ci-passed) and pre-push-reviewer (review-passed)Learnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: For reviewing someone else's PR locally, use .claude/skills/pr-review-locally/SKILL.md. Do not post comments manually; use gh workflow run or claude mentionLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: When adding a new API endpoint: create handler in internal/api/, register route in internal/api/router.go, add dependencies to Dependencies struct, wire in cmd/wavehouse/main.go, add tests, document in docs/src/content/docs/api.mdLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: When adding a new config option: add field to internal/config/config.go with yaml/env/env-default tags, use in cmd/wavehouse/main.go or relevant package, document in docs/src/content/docs/configuration.mdLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: When adding a new internal package: create under internal/, define interface if multiple implementations, wire into cmd/wavehouse/main.go, document in docs/src/content/docs/architecture.md, add matching area/<pkg> repo labelLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Conventional Commits type list in CONTRIBUTING.md must stay in sync with the regex in housekeeping.ymlLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Avoid overly broad patterns in glob rules. Generate patterns that cover all relevant files and nothing elseLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Schema-discovery failure on boot is non-fatal: api.BootState diagnostic recorded, :8080 bound, SchemaRegistry.RetryRefresh retries in background. /health and /ready return 503 until Refresh succeedsLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Interface-first design: core behaviors defined as Go interfaces (Cache, Deduplicator, Publisher, Subscriber). Standalone and future clustered modes use different implementationsLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Bring Your Own Schema (BYOS): users create tables in ClickHouse directly. WaveHouse discovers schemas by querying system.columns and validates ingest payloads. No auto-migration, no fixed table schemaLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: When backend changes alter public API surface, update SDK with corresponding changes. Decision test: would a wavehouse/sdk user's code need to change?Learnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Verbose test output: V=1 make test. Extra flags: make test ARGS="-run TestFoo". Build tags: make build TAGS="foo bar"Learnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: Most dev tools (gotestsum, gofumpt, goimports, govulncheck, go-test-coverage, deadcode, gsa, goda) pinned in go.mod via tool directives and invoked with go tool <name>Learnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: golangci-lint pinned in Makefile (v2.11.4), auto-installed to .bin/<os>_<arch>/ on first make lint. Not in go.mod due to dependency conflictsLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: pnpm (>= 11.1) and Node.js 22 LTS (pinned via .nvmrc) must be on PATH. make tools runs pnpm install --frozen-lockfileLearnt from: CR Repo: Wave-RF/WaveHouse Timestamp: 2026-05-25T11:23:51.010Z Learning: GNU Make 4+ required. macOS ships BSD Make 3.81. See docs/src/content/docs/development.md for full setup checklist📚 Learning: 2026-05-20T01:02:00.784Z
Learnt from: EricAndrechek Repo: Wave-RF/WaveHouse PR: 164 File: internal/api/router_test.go:289-350 Timestamp: 2026-05-20T01:02:00.784Z Learning: In WaveHouse’s internal API tests (files matching internal/api/**/*_test.go), follow the existing separation-of-concerns convention for testing the RequireRole middleware: inject `ContextKeyRole` directly into the request `context.Context` instead of using `testutil.MakeJWT`/JWT-driven flows. Do not refactor role-gate tests to use JWT tokens—JWT parsing and token handling are covered separately in `middleware_test.go` (the dedicated JWT parsing tests), and mixing those concerns would expand the failure surface and reduce isolation.Applied to files:
internal/api/metrics_middleware_test.gointernal/api/clickhouse_exec_test.go📚 Learning: 2026-05-23T01:23:59.268Z
Learnt from: EricAndrechek Repo: Wave-RF/WaveHouse PR: 174 File: internal/api/ingest_test.go:111-111 Timestamp: 2026-05-23T01:23:59.268Z Learning: In WaveHouse Go tests in internal/api/**/*_test.go, use internal/testutil.AssertJSONErrorResponse(t, w) for HTTP error-path JSON assertions. Do not use (or reintroduce) package-local assertJSONErrorResponse helpers. AssertJSONErrorResponse verifies the response Content-Type is application/json, includes the X-Content-Type-Options: nosniff header, and that the JSON body contains an "error" field.Applied to files:
internal/api/metrics_middleware_test.gointernal/api/clickhouse_exec_test.go📚 Learning: 2026-05-25T11:24:21.130Z
Learnt from: EricAndrechek Repo: Wave-RF/WaveHouse PR: 180 File: internal/cache/local.go:0-0 Timestamp: 2026-05-25T11:24:21.130Z Learning: In WaveHouse’s cache packages (e.g., internal/cache/local.go), it’s acceptable to define package-level `var` constants that hold immutable OpenTelemetry metric attribute sets / `metric.MeasurementOption` values (for example: `cacheL1Attrs = metric.WithAttributes(attribute.String("tier","L1"))`). Treat these as stateless, pre-allocated option values (analogous to `regexp.MustCompile(...)`), not mutable global state. When applying the AGENTS.md “no global state / constructor injection” guideline, apply it to application dependencies (e.g., Cache, Publisher, Deduplicator) rather than to these immutable OTel attribute/measurement option variables—do not flag them as constructor-injection violations.Applied to files:
internal/cache/local.go📚 Learning: 2026-05-20T20:30:15.808Z
Learnt from: taitelee Repo: Wave-RF/WaveHouse PR: 172 File: internal/api/pipes_test.go:106-118 Timestamp: 2026-05-20T20:30:15.808Z Learning: For WaveHouse pipes authorization allowlist checks, fix the empty-role fail-open behavior by (1) removing any outer guard that prevents allowlist evaluation when the incoming `role` is `""` (e.g., don’t short-circuit with `if role != "" { ... }`), and (2) during allowlist scanning, ensure only non-empty allowlist entries can match—e.g., require `ar != "" && ar == role` (so a malformed allowlist like `["" ]` cannot grant access to an empty incoming role via `"" == ""`).Applied to files:
internal/api/pipes.go📚 Learning: 2026-05-25T11:25:08.794Z
Learnt from: EricAndrechek Repo: Wave-RF/WaveHouse PR: 180 File: internal/observability/instruments.go:40-117 Timestamp: 2026-05-25T11:25:08.794Z Learning: In WaveHouse (Go), it is idiomatic to declare OpenTelemetry (OTel) metric instruments as package-level `var` values created via the OTel Go SDK global proxy pattern (e.g., `var h metric.Float64Histogram = Meter().Float64Histogram(...)`, `var c metric.Int64Counter = Meter().Int64Counter(...)`). When reviewing, do NOT flag these as “global state” violations under the AGENTS.md constructor-injection rule; that rule is intended for swappable application-level dependencies (Cache/Publisher/Subscriber/Deduplicator), not OTel proxy instrument declarations. Do not recommend refactoring these instruments behind an `Instruments` struct for dependency injection.Applied to files:
internal/observability/metrics.gointernal/observability/instruments.gointernal/observability/logger_test.gointernal/observability/logger.go🧬 Code graph analysis (10)
internal/cache/local.go (1)
internal/observability/instruments.go (2)
CacheMisses(91-94)CacheHits(87-90)internal/discovery/discovery_test.go (2)
internal/discovery/discovery.go (2)
TableSchema(25-28)Column(17-22)internal/discovery/validation.go (2)
Validate(33-67)ClassifyValidationError(12-29)internal/api/pipes.go (1)
internal/api/middleware.go (1)
RoleFromContext(52-55)internal/api/middleware.go (1)
internal/observability/instruments.go (2)
Tracer(16-16)AuthFailures(80-83)cmd/wavehouse/main.go (2)
internal/config/config.go (1)
Logging(40-43)internal/observability/logger.go (2)
ResolveLogFormat(31-45)NewBootstrapLogger(127-134)internal/observability/logger_test.go (1)
internal/observability/logger.go (2)
LogFormatJSON(25-25)NewBootstrapLogger(127-134)internal/api/ingest.go (1)
internal/observability/instruments.go (1)
SchemaRejected(74-77)internal/ingest/bento_test.go (1)
internal/mq/mq.go (1)
NewMessage(20-22)internal/ingest/bento.go (3)
internal/observability/instruments.go (2)
ClickHouseDuration(51-55)IngestDuration(43-47)internal/observability/logger.go (1)
WithComponent(73-75)internal/observability/tracer.go (1)
ExtractNATS(47-53)internal/api/clickhouse_exec.go (1)
internal/observability/instruments.go (1)
Tracer(16-16)🔇 Additional comments (29)
CHANGELOG.md (1)
12-21: LGTM!cmd/wavehouse/main.go (1)
49-56: LGTM!Also applies to: 101-109, 123-132, 230-240, 345-356
docs/src/content/docs/deployment.md (1)
373-373: LGTM!Also applies to: 396-396
internal/api/clickhouse_exec.go (1)
20-27: LGTM!Also applies to: 29-37, 43-45, 88-89, 107-109, 117-120, 142-147, 169-173, 183-195, 243-246, 276-278, 317-319
internal/api/clickhouse_exec_test.go (1)
5-6: LGTM!Also applies to: 11-37
internal/api/ingest.go (1)
63-67: LGTM!Also applies to: 84-85, 203-203
internal/api/metrics_middleware.go (1)
16-19: LGTM!Also applies to: 40-40
internal/api/metrics_middleware_test.go (1)
17-19: LGTM!Also applies to: 61-61, 67-69, 87-100
internal/api/middleware.go (1)
18-34: LGTM!Also applies to: 119-152, 191-204
internal/api/pipes.go (2)
94-109: LGTM!
20-21: LGTM!Also applies to: 181-183
internal/api/query.go (2)
20-33: LGTM!Also applies to: 168-194, 196-243
245-271: LGTM!internal/api/structured_query.go (1)
21-23: LGTM!Also applies to: 54-55, 135-135, 170-172
internal/cache/local.go (1)
14-17: LGTM!Also applies to: 39-51
internal/config/config.go (2)
35-43: LGTM!Also applies to: 177-188
64-79: LGTM!Also applies to: 221-232
internal/discovery/discovery_test.go (1)
688-729: LGTM!internal/discovery/validation.go (1)
8-29: LGTM!internal/observability/instruments.go (2)
19-21: Panic-on-registration remains an unresolved error-handling concern.
mustFloat64HistogramandmustInt64Counterstill terminate viapanic(err)on registration failures.As per coding guidelines,
**/*.go: Return errors, don't panic. Wrap withfmt.Errorf("context: %w", err).Also applies to: 22-37
11-17: LGTM!Also applies to: 40-42, 49-50, 57-58, 64-65, 72-73, 79-79, 85-87, 96-99, 104-105, 111-113
internal/ingest/bento.go (1)
38-39: LGTM!Also applies to: 54-55, 64-65, 99-104, 114-123, 164-174, 201-203, 219-233, 238-249, 384-384, 428-428
internal/ingest/bento_test.go (1)
665-721: LGTM!internal/ingest/sweeper.go (1)
34-36: LGTM!internal/observability/logger.go (1)
19-20: LGTM!Also applies to: 28-30, 70-73, 82-83, 102-104, 113-115, 125-127, 136-138
internal/observability/logger_test.go (1)
17-18: LGTM!Also applies to: 57-60, 81-87, 119-119, 125-125, 178-179, 255-257
internal/observability/metrics.go (1)
15-17: LGTM!Also applies to: 24-27, 31-31, 36-39, 58-60, 91-94
scripts/orchestrator/main.go (1)
245-250: LGTM!Also applies to: 256-256, 263-264
scripts/orchestrator/main_test.go (1)
12-61: LGTM!
github-project-automation
Bot
moved this from In review
to In progress
in WaveHouse Task Board
|
Closing — restarting the observability buildout in a fresh PR. Main has moved on significantly (#172 RBAC, #182 api/ingest/clickhouse refactor, #189 structured auth-denial logging) since this branch was last touched, and a clean re-implementation from current main will be easier to review than translating this branch through the divergence. Spec from this branch's CHANGELOG carried forward for the next pass. |
github-project-automation
Bot
moved this from In progress
to Done
in WaveHouse Task Board
1 participant
Summary
Closes the bulk of #94 (Tier S + Tier A) and ships the dev-loop log-format work the issue's follow-up comment called out. #143 (pprof) is intentionally deferred to its own PR.
WH_LOG_FORMAT=auto|text|jsonwith TTY auto-detect (coloredtinttext in dev, JSON in containers).WH_LOG_LEVELmoves into the newlogging.*YAML block. Per-subsystemcomponentfield stamped viaobservability.WithComponent(ctx, ...)at every handler entry + background worker;TraceHandler.Handleinjects it onto every record alongside trace_id/span_id. Top-level attribute renamedservice=wavehouseso per-process and per-request labels don't collide.wavehouse_ingest_duration_seconds(table, outcome),wavehouse_clickhouse_duration_seconds(operation),wavehouse_clickhouse_errors_total(operation, clickhouse_code),wavehouse_http_request_duration_seconds(chi RoutePattern, method, status_class),wavehouse_jetstream_consumer_pending.discovery.ClassifyValidationError), auth failures (typed JWT sentinels), cache L1 hits/misses, dedupe hit/miss, NATS publish-503,wavehouse_query_singleflight_shared_total(surface=structured_query|pipes).schema_validationunder ingest,jwt_verifywithauth.method/auth.role_present/auth.failure_reasonattributes,clickhouse.<operation>fromexecuteCHQuerycovering structured-query + pipes,clickhouse.admin_queryon the HTTP proxy. Raw SQL deliberately omitted.scripts/orchestrator/main.gonow skips the 80-line stderr tail when an OTel collector responds atWH_OTEL_ADDR(orWH_E2E_LOG_DUMP=0). When suppressed, prints a one-line breadcrumb pointing at the collector. Default behavior unchanged when no collector reachable.internal/observability/instruments.gowithmust*panic-on-typo helpers. Bento's two pre-existing counters keep theirwavehouse_bento_*names for dashboard continuity.logging.*block. config.yaml anddeployments/compose/standalone.yamlshow the new env. AGENTS.md observability paragraph updated. CHANGELOG entry under [Unreleased] > Added.Test plan
make cipasses locally (4 commits in, all 4 suites green at 84.3% coverage)ship_itNewLoggersignature updated toobservability.LogFormatJSONmake devin a terminal and confirm colored text outputmake obs-grafana+make test-e2eand confirm orchestrator suppresses the log-tail dump/metricsfrom amake devinstance and confirm new instruments appear🤖 Generated with Claude Code