Repository navigation
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds scheme-aware OTLP dialing (https vs http), OTLP header parsing/validation and per-signal endpoint overrides; wires TLS+headers into OTLP exporters and tests; extends test utilities to support TLS and header capture; and adds a local SigNoz observability stack, Makefile targets, dashboards, loader script, and docs. ChangesDirect-to-Cloud OTLP + Local SigNoz Integration
Sequence Diagram(s) sequenceDiagram
participant WaveHouse
participant Parser
participant Provider
participant Exporter
participant OTLP
rect rgba(200,150,255,0.5)
Note over WaveHouse,OTLP: Old flow (plaintext only)
WaveHouse->>Provider: WH_OTEL_ADDR=host:4317
Provider->>Exporter: Setup with Insecure transport
Exporter->>OTLP: Connect plaintext
end
rect rgba(150,200,255,0.5)
Note over WaveHouse,OTLP: New flow (TLS + Headers)
WaveHouse->>Parser: WH_OTEL_ADDR=https://api.example.com/otlp
WaveHouse->>Parser: WH_OTEL_HEADERS=Authorization=Bearer xyz
Parser-->>Provider: host api.example.com, useTLS true, headers map
Provider->>Exporter: Setup TLS creds and headers
Exporter->>OTLP: Connect TLS and send headers
end
rect rgba(200,255,150,0.5)
Note over WaveHouse,Provider: Per-signal override
WaveHouse->>Provider: WH_OTEL_TRACES_ADDR=https://traces.example.com
Provider->>Exporter: Traces → HTTPS endpoint
Provider->>Exporter: Metrics/Logs → default plaintext endpoint
end
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 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. 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 enhances WaveHouse's observability capabilities by enabling direct-to-cloud OTLP exports, removing the need for sidecar collectors for cloud-native backends. It also introduces a robust local development environment using SigNoz, complete with automated dashboard management, to improve the developer experience for observability features. 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
|
github-actions
Bot
added
documentation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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 introduces direct-to-cloud OTLP support, allowing WaveHouse to export telemetry to TLS-protected endpoints (e.g., Honeycomb, Grafana Cloud) without a sidecar collector. It also adds a local SigNoz observability stack for development, complete with version-controlled dashboards and updated documentation. I have identified that the documentation for loading dashboards incorrectly references email/password authentication, which is no longer supported in favor of token-based authentication. Please update the documentation and ensure this API change is reflected in the changelog.
|
2 [MUST], 0 [SHOULD], 1 [MAY] — see inline threads for detail. The Go implementation is solid: The only issue is a doc-code mismatch introduced in the same PR: Iterate — resolve the email/password doc/code mismatch before merge. |
|
0 [MUST], 1 [SHOULD] (this pass) — prior The Go core is solid: Prior [MUST] threads still unresolved (commit New this pass: 1 [SHOULD] — see the inline comment on Iterate — resolve the open prior [MUST] threads + fix the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deployments/signoz/clickhouse/users.xml`:
- Line 68: The ClickHouse users.xml entry currently has an empty <password> and
wide network allow entries ("::/0"), which is too permissive; update the user
block (the <password> element for this default user) to set a strong password or
tighten the <networks> allow rules to only 127.0.0.1 and ::1, or alternatively
add a clear inline comment/docstring in the same user block explaining this is
strictly a dev-only config and why empty password/global access is acceptable;
locate the user stanza and adjust the <password> and <networks> (or add
documentation) accordingly.
In `@deployments/signoz/compose.yaml`:
- Line 129: Replace the hardcoded SIGNOZ_TOKENIZER_JWT_SECRET=secret with a
runtime-supplied environment variable and add a fail-fast check: set
SIGNOZ_TOKENIZER_JWT_SECRET to reference the external env (e.g.,
${SIGNOZ_TOKENIZER_JWT_SECRET}) instead of the literal "secret", and ensure the
service's startup path (entrypoint/command or init script used by the container)
validates SIGNOZ_TOKENIZER_JWT_SECRET is present and non-empty and exits with a
clear error if missing so the deployment cannot start with a predictable key.
- Around line 68-75: The startup script downloads and installs the
histogram-quantile binary without verifying integrity; update the block that
defines version, node_os/node_arch and runs wget/tar/mv to fetch and verify a
checksum or signature before extracting and moving the binary (e.g., download
accompanying .sha256 or .asc, verify with sha256sum or gpg against a trusted key
or published checksum for histogram-quantile_$${node_os}_$${node_arch}.tar.gz),
fail the startup if verification fails, and only then run tar and mv to
/var/lib/clickhouse/user_scripts/histogramQuantile so that wget/tar/mv (and the
version variable) are the identifiable symbols to change.
In `@deployments/signoz/dashboards/wavehouse-overview.json`:
- Around line 77-79: The dashboard's widget filter blocks all have an empty
"expression" (the JSON snippet "filter": { "expression": "" }), causing metrics
to be global; update each widget in the "WaveHouse · Overview" dashboard to set
a non-empty filter expression using the canonical WaveHouse service resource key
(replace the empty string with your service filter, e.g. resource.service.name
== "WaveHouse" or the project's canonical key), applying this change for every
occurrence of the empty expression so each widget is scoped to WaveHouse.
In `@deployments/signoz/load-dashboards.sh`:
- Around line 55-57: The script reads title="$(jq -r '.title' "$f")" but doesn't
validate it; if .title is missing or null the variable becomes empty/"null" and
causes unpredictable behavior. After setting title (and before computing
id/upserting), add a guard that checks title for emptiness or the literal "null"
and on failure print a clear error referencing the file variable f and exit
non-zero (or skip/return for that file per your chosen flow); ensure this check
is placed before the id=... line so functions/variables like title and id are
only used when title is valid.
In `@docs/src/content/docs/deployment.md`:
- Around line 393-400: Update the dashboard loader auth instructions to reflect
token-only auth: replace examples that set SIGNOZ_EMAIL and SIGNOZ_PASSWORD with
examples that set SIGNOZ_TOKEN (e.g., SIGNOZ_TOKEN=<AUTH_TOKEN> make
signoz-dashboards or SIGNOZ_TOKEN=<AUTH_TOKEN>
deployments/signoz/load-dashboards.sh), remove any wording that frames
SIGNOZ_TOKEN as an optional fallback, and change the explanatory text to state
the loader requires the AUTH_TOKEN from the SigNoz UI localStorage; keep
SIGNOZ_URL note and the requirements for curl/jq and the export instructions
unchanged.
In `@docs/src/content/docs/development.md`:
- Around line 166-177: Docs outdated: the dashboard loader now accepts
SIGNOZ_TOKEN only, but docs still mention SIGNOZ_EMAIL/SIGNOZ_PASSWORD; update
docs/development.md text around the "First run" block to remove references to
SIGNOZ_EMAIL and SIGNOZ_PASSWORD and clearly state that you must export
SIGNOZ_TOKEN (e.g., export SIGNOZ_TOKEN='...') before running make dev-obs or
make signoz-dashboards; also update the explanatory sentence about dev-obs/dev
loader to say it runs a token-only best-effort loader (failure non-fatal) and
add a note pointing to deployments/signoz/load-dashboards.sh for the token
requirement.
In `@internal/observability/endpoint_test.go`:
- Around line 3-6: Tests use reflect.DeepEqual and t.Fatalf; update them to use
testify assertions to match project convention: add
"github.com/stretchr/testify/require" to the test imports (remove "reflect"),
then replace reflect.DeepEqual checks with require.Equal(t, expected, actual)
(or require.True(t, reflect.DeepEqual(...)) -> require.Truef/require.Equalf as
appropriate) and replace any t.Fatalf calls with the corresponding require.*
(e.g., require.FailNowf or require.Equalf/require.NoError) in the test functions
referenced (the assertions around reflect.DeepEqual and t.Fatalf at the
locations using those symbols).
In `@WHissues.md`:
- Around line 21-24: Update the fenced code blocks in WHissues.md to include a
language identifier (use "text") for each block that currently lacks one;
specifically add ```text before blocks containing the lines starting with
"migrate/bootstrap.go:119 Creating databases", "mq init failed error=",
"schema discovery failed on boot error=", and "query system.columns: code: 516,
message:" (and the other similar blocks at the locations mentioned), so every
triple-backtick fence is ```text instead of ``` to satisfy markdownlint MD040.
- Line 30: The inline code span contains a padded trailing space ("`... ON
CLUSTER `") which triggers markdownlint MD038; update the markdown so the inline
code is "`... ON CLUSTER`" (remove the trailing space inside the backticks)
wherever that exact code span appears to eliminate the padding.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 85732c2b-8729-4905-941f-8f0e6273b1ac
📒 Files selected for processing (31)
AGENTS.mdCHANGELOG.mdMakefileREADME.mdWHissues.mdcmd/wavehouse/main.godeployments/compose/standalone.signoz.yamldeployments/signoz/.env.exampledeployments/signoz/clickhouse/cluster.xmldeployments/signoz/clickhouse/config.xmldeployments/signoz/clickhouse/custom-function.xmldeployments/signoz/clickhouse/user_scripts/.gitkeepdeployments/signoz/clickhouse/users.xmldeployments/signoz/compose.yamldeployments/signoz/dashboards/README.mddeployments/signoz/dashboards/wavehouse-overview.jsondeployments/signoz/dashboards/wavehouse-runtime-internals.jsondeployments/signoz/docker-compose.yamldeployments/signoz/load-dashboards.shdeployments/signoz/otel-collector-config.yamldeployments/signoz/otel-collector-opamp-config.yamldocs/src/content/docs/configuration.mddocs/src/content/docs/deployment.mddocs/src/content/docs/development.mdinternal/config/config.gointernal/config/config_test.gointernal/observability/endpoint.gointernal/observability/endpoint_test.gointernal/observability/provider.gointernal/testutil/otlp.gotests/integration/otel_test.go
💤 Files with no reviewable changes (2)
- deployments/signoz/docker-compose.yaml
- deployments/signoz/.env.example
| version="v0.0.1" | ||
| node_os=$$(uname -s | tr '[:upper:]' '[:lower:]') | ||
| node_arch=$$(uname -m | sed s/aarch64/arm64/ | sed s/x86_64/amd64/) | ||
| echo "Fetching histogram-quantile binary for $${node_os}/$${node_arch}" | ||
| cd /tmp | ||
| wget -O histogram-quantile.tar.gz "https://github.com/SigNoz/signoz/releases/download/histogram-quantile%2F$${version}/histogram-quantile_$${node_os}_$${node_arch}.tar.gz" | ||
| tar -xvzf histogram-quantile.tar.gz | ||
| mv histogram-quantile /var/lib/clickhouse/user_scripts/histogramQuantile |
There was a problem hiding this comment.
Verify integrity of the downloaded histogram-quantile binary before install.
This startup path executes a network-downloaded artifact without checksum/signature verification, which is a supply-chain risk.
🔒 Suggested hardening
command:
- bash
- -c
- |
+ set -euo pipefail
version="v0.0.1"
+ expected_sha256="${HISTOGRAM_QUANTILE_SHA256:?set HISTOGRAM_QUANTILE_SHA256}"
node_os=$$(uname -s | tr '[:upper:]' '[:lower:]')
node_arch=$$(uname -m | sed s/aarch64/arm64/ | sed s/x86_64/amd64/)
echo "Fetching histogram-quantile binary for $${node_os}/$${node_arch}"
cd /tmp
wget -O histogram-quantile.tar.gz "https://github.com/SigNoz/signoz/releases/download/histogram-quantile%2F$${version}/histogram-quantile_$${node_os}_$${node_arch}.tar.gz"
+ echo "$${expected_sha256} histogram-quantile.tar.gz" | sha256sum -c -
tar -xvzf histogram-quantile.tar.gz
mv histogram-quantile /var/lib/clickhouse/user_scripts/histogramQuantile📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| version="v0.0.1" | |
| node_os=$$(uname -s | tr '[:upper:]' '[:lower:]') | |
| node_arch=$$(uname -m | sed s/aarch64/arm64/ | sed s/x86_64/amd64/) | |
| echo "Fetching histogram-quantile binary for $${node_os}/$${node_arch}" | |
| cd /tmp | |
| wget -O histogram-quantile.tar.gz "https://github.com/SigNoz/signoz/releases/download/histogram-quantile%2F$${version}/histogram-quantile_$${node_os}_$${node_arch}.tar.gz" | |
| tar -xvzf histogram-quantile.tar.gz | |
| mv histogram-quantile /var/lib/clickhouse/user_scripts/histogramQuantile | |
| set -euo pipefail | |
| version="v0.0.1" | |
| expected_sha256="${HISTOGRAM_QUANTILE_SHA256:?set HISTOGRAM_QUANTILE_SHA256}" | |
| node_os=$$(uname -s | tr '[:upper:]' '[:lower:]') | |
| node_arch=$$(uname -m | sed s/aarch64/arm64/ | sed s/x86_64/amd64/) | |
| echo "Fetching histogram-quantile binary for $${node_os}/$${node_arch}" | |
| cd /tmp | |
| wget -O histogram-quantile.tar.gz "https://github.com/SigNoz/signoz/releases/download/histogram-quantile%2F$${version}/histogram-quantile_$${node_os}_$${node_arch}.tar.gz" | |
| echo "$${expected_sha256} histogram-quantile.tar.gz" | sha256sum -c - | |
| tar -xvzf histogram-quantile.tar.gz | |
| mv histogram-quantile /var/lib/clickhouse/user_scripts/histogramQuantile |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deployments/signoz/compose.yaml` around lines 68 - 75, The startup script
downloads and installs the histogram-quantile binary without verifying
integrity; update the block that defines version, node_os/node_arch and runs
wget/tar/mv to fetch and verify a checksum or signature before extracting and
moving the binary (e.g., download accompanying .sha256 or .asc, verify with
sha256sum or gpg against a trusted key or published checksum for
histogram-quantile_$${node_os}_$${node_arch}.tar.gz), fail the startup if
verification fails, and only then run tar and mv to
/var/lib/clickhouse/user_scripts/histogramQuantile so that wget/tar/mv (and the
version variable) are the identifiable symbols to change.
|
2 [MUST], 0 [SHOULD] (new this pass), 1 [MAY] — see inline threads. The Go implementation is solid: Open threads requiring resolution: The two [MUST] email/password threads from the first review pass are now marked outdated (HEAD moved from
The [SHOULD] from the second pass ( New this pass: 1 [MAY] on Iterate — resolve the email/password doc/code mismatch and the |
| func TestOTel_PerSignalEndpoint_Override(t *testing.T) { | ||
| guardOTelGlobals(t) | ||
| rDefault := testutil.NewFakeOTLP(t) | ||
| rTraces := testutil.NewFakeOTLP(t) | ||
|
|
||
| shutdown, _ := initAndShutdown(t, observability.ProviderConfig{ | ||
| Endpoint: rDefault.Addr(), // metrics + logs land here | ||
| TracesEndpoint: rTraces.Addr(), // traces override | ||
| TracesEnabled: true, | ||
| TracesSampleRate: 1.0, | ||
| MetricsEnabled: true, | ||
| }) | ||
|
|
||
| _, span := otel.Tracer("test").Start(context.Background(), "split-op") | ||
| span.End() | ||
|
|
||
| counter, err := otel.GetMeterProvider().Meter("test").Int64Counter("split_counter") | ||
| require.NoError(t, err) | ||
| counter.Add(context.Background(), 1) | ||
|
|
||
| drainCtx, drainCancel := context.WithTimeout(context.Background(), 5*time.Second) | ||
| defer drainCancel() | ||
| require.NoError(t, shutdown(drainCtx)) | ||
|
|
||
| assert.Equal(t, 1, rTraces.SpanCount(), "trace endpoint should have received the span") | ||
| assert.Zero(t, rDefault.SpanCount(), "default endpoint must NOT receive traces when overridden") | ||
| assert.GreaterOrEqual(t, rDefault.MetricCount(), 1, "default endpoint should receive metrics (no override)") | ||
| assert.Zero(t, rTraces.MetricCount(), "trace endpoint should NOT receive metrics") | ||
| } |
There was a problem hiding this comment.
[MAY] This test only exercises TracesEndpoint override; MetricsEndpoint and LogsEndpoint go through the same pickEndpoint() helper in provider.go but aren't independently verified. A copy-paste bug (e.g. cfg.TracesEndpoint hardcoded where cfg.MetricsEndpoint should appear) wouldn't be caught here. Consider spinning up a third receiver, setting MetricsEndpoint to it, and asserting that metrics land there instead of rDefault — one extra signal is enough to confirm the wiring is symmetric.
There was a problem hiding this comment.
Fixed in 5babcf4. Added a third FakeOTLP receiver (rMetrics) and pointed MetricsEndpoint at it. With both signals individually routed, the default endpoint now expects to receive nothing — caught by rDefault.MetricCount() == 0. The new cross-asserts (rTraces.MetricCount() == 0 and rMetrics.SpanCount() == 0) directly fail the copy-paste scenario you described — if the metrics exporter were wired to cfg.TracesEndpoint, metrics would silently land on rTraces and both cross-asserts would fail with messages naming the bug class. Held off on a fourth receiver for logs since logs goes through the same pickEndpoint() helper (~5-line extension if needed later). Docstring rewritten to call out the symmetric-wiring intent.
|
Review: 0 [MUST], 0 [SHOULD], 3 [MAY] (see inline threads). Implementation is correct — TLS/headers/per-signal wiring is clean, integration tests cover all three critical paths, doc sync is complete. Verdict: Ship it. Inline threads cover WHissues.md discoverability, dual-parse-logic coupling comment, and MetricsEndpoint/LogsEndpoint override test coverage gap. None block merge. |
…validation `cmd/wavehouse/main.go` previously discarded the error from `observability.ParseOTelHeaders` with a comment noting that the parser cannot fail for input that `Validate()` already accepted. The assumption holds today — `internal/config/config.go`'s `validateOTelHeaders` is hand-mirrored from the same parsing logic — but the two implementations are independent. If they ever drift (e.g. a future auth-scheme tweak lands in one but not the other), `validateOTelHeaders` would accept input that `ParseOTelHeaders` silently truncates, the OTLP exporters would ship with no auth metadata, and the failure would surface only as rejected-on-the-wire telemetry — not as a startup error or a log message. Switched the blank-identifier to a real error check that logs and returns 1. Matches the fatal-error pattern used everywhere else in `run()`. Costs four lines; eliminates the silent-auth-drop class. Possible follow-up (not in this commit): consolidate the two parsers into a single leaf package both `config` and `observability` import, so drift is impossible by construction. Out of scope here because it shifts the package dependency graph (config currently imports nothing from observability so the rest of the codebase doesn't transitively pull in the OTel SDK through config). Closes the security-MEDIUM [MUST] thread at cmd/wavehouse/main.go:107.
The previous fix (commit 380daf5) for the "make signoz-up hard-fails" review thread switched compose.yaml from `:?` to `:-wavehouse-local-dev-only`. That eliminated the boot break but introduced a different problem flagged on re-review: a public repository now ships a known JWT signing key, so anyone with the string can forge valid tokens against any local SigNoz instance whose operator didn't override the env var. Severity is low for a tool that binds to localhost, but the default is genuinely a worse posture than either fail-fast or a per-checkout random value. Replaced both failure modes with a per-checkout random secret cached in a gitignored file: - `deployments/signoz/compose.yaml` reverts to the `:?` form so a raw `docker compose -f ... up` fails fast with a clear pointer at the remediation paths (`make signoz-up` or `export SIGNOZ_TOKENIZER_JWT_SECRET=$(openssl rand -hex 32)`). Inline comment names the cache path and explains how it rotates. - `Makefile` gains a `$(SIGNOZ_SECRET_FILE)` rule (`deployments/signoz/.signoz-secret.local`) that generates the secret via `openssl rand -hex 32` on first need, with `chmod 600`. `SIGNOZ_COMPOSE` (recursively expanded) now prefixes every `docker compose` call with the secret read from the cache file, so all `signoz-*` recipes get it transparently. `signoz-wipe` rms the cache after `docker compose down -v`, so the next `signoz-up` rotates to a fresh value — matching the volume reset which already invalidates any tokens issued under the old secret. All four `signoz-{up,down,logs,wipe}` targets list the secret file as a prerequisite, so any compose invocation in those recipes is guaranteed to find a cached secret without ever hitting compose's interpolation guard. - `.gitignore` ignores `deployments/signoz/.signoz-secret.local`. Verified end-to-end: `docker compose config` without the env fails with the new pointer message; the Makefile rule generates a 64-char hex file with mode 0600; compose parses cleanly with the cached secret loaded; `git check-ignore` confirms the cache file is matched. Closes the security-LOW [SHOULD] thread at compose.yaml:144 (escalated in scope because the underlying bug — shared known secret — undid the prior [MUST] fix's intent).
|
0 [MUST], 1 [SHOULD], 0 [MAY] — see inline thread for detail. Ship it — the SHOULD (noisy The Go code is clean: |
config.validateOTelHeaders and observability.ParseOTelHeaders encode the same parsing rules but live in different packages because config can't import observability without transitively pulling the OTel SDK into every config consumer (config sits at the bottom of the dep graph by design). That arrangement is fine, but it creates a drift surface: a rule change in one without the matching change in the other would silently let `config.Validate()` accept input that `ParseOTelHeaders` truncates, and `cmd/wavehouse/main.go`'s defensive error check (commit 98621af) would catch the drift at startup loudly — but only after it had already happened. Added a "MUST stay in sync with X" comment on both functions naming the counterpart explicitly, plus a one-line reminder that any rule change here needs the same change there. Makes the coupling discoverable from either side rather than relying on future editors to grep for it. Closes the [MAY] thread at config.go:169.
TestOTel_PerSignalEndpoint_Override only verified the TracesEndpoint
override path. Metrics and logs overrides go through the same
pickEndpoint() helper in provider.go, but a copy-paste bug — e.g.
the metrics exporter wired to cfg.TracesEndpoint instead of
cfg.MetricsEndpoint — wouldn't have failed the test: metrics would
silently land on the trace receiver and the assertion checking the
default endpoint received metrics would also pass (because before
this change, no MetricsEndpoint override was set, so metrics
correctly went to the default).
Extended the test:
- Added a third FakeOTLP receiver, `rMetrics`, and pointed
MetricsEndpoint at it.
- The default endpoint now expects to receive *nothing* (both
signals individually routed) — caught by `rDefault.MetricCount()`
being zero.
- Cross-asserts: rTraces sees zero metrics, rMetrics sees zero
spans. Either of these would fail if someone swapped the
endpoint variable in a future provider.go edit.
The docstring is rewritten to call out the symmetric-wiring
intent — so the next contributor to touch the test knows the cross-
asserts aren't paranoid filler, they're the actual point.
Logs are still excluded; logs goes through the same pickEndpoint()
helper so by induction the test now covers that too. If a fourth
receiver / LogsEndpoint assertion turns out to be necessary later
(say, the helper gets refactored signal-by-signal), it's a five-line
extension following the same pattern.
Closes the [MAY] thread at tests/integration/otel_test.go:433.
Review — 0 [MUST], 2 [SHOULD], 0 [MAY]See the inline threads for detail. What was already resolved (prior review cycles, confirmed at HEAD New findings at this HEAD:
Ship it — no [MUST]s. The core OTLP implementation (scheme-aware TLS, header propagation, per-signal endpoint routing), the |
The SIGNOZ_COMPOSE macro reads the cached JWT secret via
`cat $(SIGNOZ_SECRET_FILE)`. Every `signoz-*` recipe declares the
secret file as a prerequisite, so Make ensures the file exists before
those recipes run — `cat` always succeeds inside their bodies.
`clean-all` is different: it calls `$(SIGNOZ_COMPOSE) down -v ... ||
true` without depending on the secret file (correctly — we don't want
to generate a secret just to tear something down). If the developer
runs `make clean-all` on a fresh checkout that never invoked
`signoz-up`, the `cat` subshell emits a `cat: deployments/signoz/
.signoz-secret.local: No such file or directory` message to stderr
before the compose call runs. The compose `:?` interpolation then
fails, `down` exits non-zero, `|| true` swallows the exit, and the
user sees only the leaked `cat:` stderr line. The overall command
still succeeds — the noise is purely cosmetic, but confusing.
One-character fix: `2>/dev/null` on the `cat` inside the macro
definition. With this:
- signoz-{up,down,logs,wipe} are unaffected (Make generates the
file before invoking the recipe, so cat doesn't need stderr).
- clean-all's compose call now sees an empty
SIGNOZ_TOKENIZER_JWT_SECRET, compose's `:?` guard fires silently
on its own stderr (already redirected by `2>/dev/null` in the
clean-all recipe), `|| true` mops up the exit, and the user sees
nothing — which is what they want from a teardown command.
Closes the [SHOULD] thread at Makefile:242.
Removes the running-issues log from the repo root. Review feedback
(claude[bot] at WHissues.md:1) flagged that this kind of operational-
notes file accumulates drift and overlaps with what CHANGELOG.md
already covers — and the entries also point at follow-up work in
`standalone.yaml` that isn't part of this PR's scope, which invites
readers to mistake "documented" for "fixed."
Untracking only — no inline-migration of the §-references. The
CHANGELOG and `docs/development.md` references to `WHissues.md` are
left in place as dangling pointers; they'll fall out naturally on
the next pass that edits those files. Minimum-disruption deletion.
The follow-up work this file pointed at:
- §2 (standalone.yaml's 50 GB JetStream default vs ~28 GB OrbStack
disk → boot failure) — still an open bug in
deployments/compose/standalone.yaml.
- §3 (missing ClickHouse healthcheck in standalone.yaml → schema
discovery races a still-initializing CH) — also still open.
- §1 (the SigNoz compose rebuild) and §4 (the network-collision
dual-home gotcha) are already addressed by this PR's commits.
§2 and §3 should land as separate fixes or be filed as issues — they
don't belong bundled in this PR. The deletion makes that explicit.
Closes the [SHOULD] thread at WHissues.md:1.
Review — PR #135: direct-to-cloud OTLP + local SigNoz dev stack0 [MUST], 0 [SHOULD], 1 [MAY] — see the inline thread for detail. Ship it. The core Go changes are solid: The only remaining nit is a missing dev-only annotation in |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deployments/signoz/dashboards/wavehouse-runtime-internals.json`:
- Around line 126-190: The query for the Go memory panel is missing a service
filter so it aggregates across all services; update the queryData entry with
queryName "A" (the aggregation for metric "go.memory.used" and legend
"{{go.memory.type}}") by setting its filter.expression to restrict to WaveHouse,
e.g. add a filter like service.name="WaveHouse" (or a case-insensitive regex if
needed) so the panel only shows memory for the WaveHouse service.
- Around line 68-125: The panel's query is missing a service filter so the
aggregation pulls gos from all services; update the query builder entry for
queryData where "queryName": "A" (metricName "go.goroutine.count", legend
"goroutines") to set filter.expression to "service.name = 'wavehouse'". Ensure
you add the exact expression string into the existing filter object for that
query (the same pattern used elsewhere in the dashboard) so the panel only
includes WaveHouse metrics.
- Around line 191-248: The Go allocation metric panel (query id
"c39d7174-e9d2-5b0b-b43e-cfb2b575d403", queryData entry with "queryName": "A"
and metric "go.memory.allocations") has an empty filter; add a service filter to
scope it to WaveHouse by populating the filter.expression with the appropriate
service.name condition (e.g., restrict to the WaveHouse service), leaving the
rest of the aggregation and legend intact.
- Around line 492-576: The two metric query blocks for "A"
(http.server.request.body.size.bucket, legend "request p99") and "B"
(http.server.response.body.size.bucket, legend "response p99") currently have
empty filter.expression values and will aggregate across all services; update
both queryData entries to set filter.expression to restrict by service.name
(e.g. service.name = "<YOUR_SERVICE>" or a dashboard variable like
${service_name}) so each aggregation only includes the intended service.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 96a121a7-216b-45f7-85af-c70ce34f6e65
📒 Files selected for processing (32)
.gitignoreAGENTS.mdCHANGELOG.mdMakefileREADME.mdcmd/wavehouse/main.goconfig.yamldeployments/compose/standalone.signoz.yamldeployments/signoz/.env.exampledeployments/signoz/clickhouse/cluster.xmldeployments/signoz/clickhouse/config.xmldeployments/signoz/clickhouse/custom-function.xmldeployments/signoz/clickhouse/user_scripts/.gitkeepdeployments/signoz/clickhouse/users.xmldeployments/signoz/compose.yamldeployments/signoz/dashboards/README.mddeployments/signoz/dashboards/wavehouse-overview.jsondeployments/signoz/dashboards/wavehouse-runtime-internals.jsondeployments/signoz/docker-compose.yamldeployments/signoz/load-dashboards.shdeployments/signoz/otel-collector-config.yamldeployments/signoz/otel-collector-opamp-config.yamldocs/src/content/docs/configuration.mddocs/src/content/docs/deployment.mddocs/src/content/docs/development.mdinternal/config/config.gointernal/config/config_test.gointernal/observability/endpoint.gointernal/observability/endpoint_test.gointernal/observability/provider.gointernal/testutil/otlp.gotests/integration/otel_test.go
💤 Files with no reviewable changes (2)
- deployments/signoz/.env.example
- deployments/signoz/docker-compose.yaml
| { | ||
| "id": "cd634115-f28a-5eac-9139-47f084d17325", | ||
| "title": "Go — goroutines", | ||
| "description": "", | ||
| "panelTypes": "graph", | ||
| "timePreferance": "GLOBAL_TIME", | ||
| "query": { | ||
| "queryType": "builder", | ||
| "builder": { | ||
| "queryData": [ | ||
| { | ||
| "aggregations": [ | ||
| { | ||
| "metricName": "go.goroutine.count", | ||
| "reduceTo": "avg", | ||
| "spaceAggregation": "sum", | ||
| "temporality": null, | ||
| "timeAggregation": "latest" | ||
| } | ||
| ], | ||
| "dataSource": "metrics", | ||
| "disabled": false, | ||
| "expression": "A", | ||
| "filter": { | ||
| "expression": "" | ||
| }, | ||
| "groupBy": [], | ||
| "having": { | ||
| "expression": "" | ||
| }, | ||
| "legend": "goroutines", | ||
| "orderBy": [], | ||
| "queryName": "A", | ||
| "stepInterval": 60 | ||
| } | ||
| ], | ||
| "queryFormulas": [] | ||
| }, | ||
| "clickhouse_sql": [ | ||
| { | ||
| "name": "A", | ||
| "query": "", | ||
| "legend": "", | ||
| "disabled": false | ||
| } | ||
| ], | ||
| "promql": [ | ||
| { | ||
| "name": "A", | ||
| "query": "", | ||
| "legend": "", | ||
| "disabled": false | ||
| } | ||
| ], | ||
| "id": "a85cb74f-054f-54c2-abeb-dfc9c6818279" | ||
| }, | ||
| "yAxisUnit": "short" | ||
| }, |
There was a problem hiding this comment.
Add service.name filter to scope Go runtime metrics to WaveHouse.
The filter.expression is empty (line 92), so this widget aggregates go.goroutine.count from all services reporting to SigNoz, not just WaveHouse. Apply the same "expression": "service.name = 'wavehouse'" pattern used in the overview dashboard.
📊 Suggested fix
"filter": {
- "expression": ""
+ "expression": "service.name = 'wavehouse'"
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deployments/signoz/dashboards/wavehouse-runtime-internals.json` around lines
68 - 125, The panel's query is missing a service filter so the aggregation pulls
gos from all services; update the query builder entry for queryData where
"queryName": "A" (metricName "go.goroutine.count", legend "goroutines") to set
filter.expression to "service.name = 'wavehouse'". Ensure you add the exact
expression string into the existing filter object for that query (the same
pattern used elsewhere in the dashboard) so the panel only includes WaveHouse
metrics.
| { | ||
| "id": "e9fa7d35-09e8-54bc-a95f-6f6d43bf0e34", | ||
| "title": "Go — memory used (bytes)", | ||
| "description": "", | ||
| "panelTypes": "graph", | ||
| "timePreferance": "GLOBAL_TIME", | ||
| "query": { | ||
| "queryType": "builder", | ||
| "builder": { | ||
| "queryData": [ | ||
| { | ||
| "aggregations": [ | ||
| { | ||
| "metricName": "go.memory.used", | ||
| "reduceTo": "avg", | ||
| "spaceAggregation": "sum", | ||
| "temporality": null, | ||
| "timeAggregation": "latest" | ||
| } | ||
| ], | ||
| "dataSource": "metrics", | ||
| "disabled": false, | ||
| "expression": "A", | ||
| "filter": { | ||
| "expression": "" | ||
| }, | ||
| "groupBy": [ | ||
| { | ||
| "key": "go.memory.type", | ||
| "dataType": "string", | ||
| "type": "tag", | ||
| "isColumn": false | ||
| } | ||
| ], | ||
| "having": { | ||
| "expression": "" | ||
| }, | ||
| "legend": "{{go.memory.type}}", | ||
| "orderBy": [], | ||
| "queryName": "A", | ||
| "stepInterval": 60 | ||
| } | ||
| ], | ||
| "queryFormulas": [] | ||
| }, | ||
| "clickhouse_sql": [ | ||
| { | ||
| "name": "A", | ||
| "query": "", | ||
| "legend": "", | ||
| "disabled": false | ||
| } | ||
| ], | ||
| "promql": [ | ||
| { | ||
| "name": "A", | ||
| "query": "", | ||
| "legend": "", | ||
| "disabled": false | ||
| } | ||
| ], | ||
| "id": "2678fb88-02cb-50b5-9456-a15e6229f024" | ||
| }, | ||
| "yAxisUnit": "bytes" | ||
| }, |
There was a problem hiding this comment.
Add service.name filter to scope Go memory metrics to WaveHouse.
Same issue as the goroutines widget — empty filter.expression (line 150) causes this to aggregate memory across all services.
📊 Suggested fix
"filter": {
- "expression": ""
+ "expression": "service.name = 'wavehouse'"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| "id": "e9fa7d35-09e8-54bc-a95f-6f6d43bf0e34", | |
| "title": "Go — memory used (bytes)", | |
| "description": "", | |
| "panelTypes": "graph", | |
| "timePreferance": "GLOBAL_TIME", | |
| "query": { | |
| "queryType": "builder", | |
| "builder": { | |
| "queryData": [ | |
| { | |
| "aggregations": [ | |
| { | |
| "metricName": "go.memory.used", | |
| "reduceTo": "avg", | |
| "spaceAggregation": "sum", | |
| "temporality": null, | |
| "timeAggregation": "latest" | |
| } | |
| ], | |
| "dataSource": "metrics", | |
| "disabled": false, | |
| "expression": "A", | |
| "filter": { | |
| "expression": "" | |
| }, | |
| "groupBy": [ | |
| { | |
| "key": "go.memory.type", | |
| "dataType": "string", | |
| "type": "tag", | |
| "isColumn": false | |
| } | |
| ], | |
| "having": { | |
| "expression": "" | |
| }, | |
| "legend": "{{go.memory.type}}", | |
| "orderBy": [], | |
| "queryName": "A", | |
| "stepInterval": 60 | |
| } | |
| ], | |
| "queryFormulas": [] | |
| }, | |
| "clickhouse_sql": [ | |
| { | |
| "name": "A", | |
| "query": "", | |
| "legend": "", | |
| "disabled": false | |
| } | |
| ], | |
| "promql": [ | |
| { | |
| "name": "A", | |
| "query": "", | |
| "legend": "", | |
| "disabled": false | |
| } | |
| ], | |
| "id": "2678fb88-02cb-50b5-9456-a15e6229f024" | |
| }, | |
| "yAxisUnit": "bytes" | |
| }, | |
| { | |
| "id": "e9fa7d35-09e8-54bc-a95f-6f6d43bf0e34", | |
| "title": "Go — memory used (bytes)", | |
| "description": "", | |
| "panelTypes": "graph", | |
| "timePreferance": "GLOBAL_TIME", | |
| "query": { | |
| "queryType": "builder", | |
| "builder": { | |
| "queryData": [ | |
| { | |
| "aggregations": [ | |
| { | |
| "metricName": "go.memory.used", | |
| "reduceTo": "avg", | |
| "spaceAggregation": "sum", | |
| "temporality": null, | |
| "timeAggregation": "latest" | |
| } | |
| ], | |
| "dataSource": "metrics", | |
| "disabled": false, | |
| "expression": "A", | |
| "filter": { | |
| "expression": "service.name = 'wavehouse'" | |
| }, | |
| "groupBy": [ | |
| { | |
| "key": "go.memory.type", | |
| "dataType": "string", | |
| "type": "tag", | |
| "isColumn": false | |
| } | |
| ], | |
| "having": { | |
| "expression": "" | |
| }, | |
| "legend": "{{go.memory.type}}", | |
| "orderBy": [], | |
| "queryName": "A", | |
| "stepInterval": 60 | |
| } | |
| ], | |
| "queryFormulas": [] | |
| }, | |
| "clickhouse_sql": [ | |
| { | |
| "name": "A", | |
| "query": "", | |
| "legend": "", | |
| "disabled": false | |
| } | |
| ], | |
| "promql": [ | |
| { | |
| "name": "A", | |
| "query": "", | |
| "legend": "", | |
| "disabled": false | |
| } | |
| ], | |
| "id": "2678fb88-02cb-50b5-9456-a15e6229f024" | |
| }, | |
| "yAxisUnit": "bytes" | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deployments/signoz/dashboards/wavehouse-runtime-internals.json` around lines
126 - 190, The query for the Go memory panel is missing a service filter so it
aggregates across all services; update the queryData entry with queryName "A"
(the aggregation for metric "go.memory.used" and legend "{{go.memory.type}}") by
setting its filter.expression to restrict to WaveHouse, e.g. add a filter like
service.name="WaveHouse" (or a case-insensitive regex if needed) so the panel
only shows memory for the WaveHouse service.
| { | ||
| "id": "596b38da-a5d3-5aae-96de-054fd50cfbf9", | ||
| "title": "Go — allocation rate (bytes/s)", | ||
| "description": "", | ||
| "panelTypes": "graph", | ||
| "timePreferance": "GLOBAL_TIME", | ||
| "query": { | ||
| "queryType": "builder", | ||
| "builder": { | ||
| "queryData": [ | ||
| { | ||
| "aggregations": [ | ||
| { | ||
| "metricName": "go.memory.allocations", | ||
| "reduceTo": "avg", | ||
| "spaceAggregation": "sum", | ||
| "temporality": null, | ||
| "timeAggregation": "rate" | ||
| } | ||
| ], | ||
| "dataSource": "metrics", | ||
| "disabled": false, | ||
| "expression": "A", | ||
| "filter": { | ||
| "expression": "" | ||
| }, | ||
| "groupBy": [], | ||
| "having": { | ||
| "expression": "" | ||
| }, | ||
| "legend": "bytes/s", | ||
| "orderBy": [], | ||
| "queryName": "A", | ||
| "stepInterval": 60 | ||
| } | ||
| ], | ||
| "queryFormulas": [] | ||
| }, | ||
| "clickhouse_sql": [ | ||
| { | ||
| "name": "A", | ||
| "query": "", | ||
| "legend": "", | ||
| "disabled": false | ||
| } | ||
| ], | ||
| "promql": [ | ||
| { | ||
| "name": "A", | ||
| "query": "", | ||
| "legend": "", | ||
| "disabled": false | ||
| } | ||
| ], | ||
| "id": "c39d7174-e9d2-5b0b-b43e-cfb2b575d403" | ||
| }, | ||
| "yAxisUnit": "Bps" | ||
| }, |
There was a problem hiding this comment.
Add service.name filter to scope Go allocation metrics to WaveHouse.
Empty filter at line 215 — same pattern as above.
📊 Suggested fix
"filter": {
- "expression": ""
+ "expression": "service.name = 'wavehouse'"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| "id": "596b38da-a5d3-5aae-96de-054fd50cfbf9", | |
| "title": "Go — allocation rate (bytes/s)", | |
| "description": "", | |
| "panelTypes": "graph", | |
| "timePreferance": "GLOBAL_TIME", | |
| "query": { | |
| "queryType": "builder", | |
| "builder": { | |
| "queryData": [ | |
| { | |
| "aggregations": [ | |
| { | |
| "metricName": "go.memory.allocations", | |
| "reduceTo": "avg", | |
| "spaceAggregation": "sum", | |
| "temporality": null, | |
| "timeAggregation": "rate" | |
| } | |
| ], | |
| "dataSource": "metrics", | |
| "disabled": false, | |
| "expression": "A", | |
| "filter": { | |
| "expression": "" | |
| }, | |
| "groupBy": [], | |
| "having": { | |
| "expression": "" | |
| }, | |
| "legend": "bytes/s", | |
| "orderBy": [], | |
| "queryName": "A", | |
| "stepInterval": 60 | |
| } | |
| ], | |
| "queryFormulas": [] | |
| }, | |
| "clickhouse_sql": [ | |
| { | |
| "name": "A", | |
| "query": "", | |
| "legend": "", | |
| "disabled": false | |
| } | |
| ], | |
| "promql": [ | |
| { | |
| "name": "A", | |
| "query": "", | |
| "legend": "", | |
| "disabled": false | |
| } | |
| ], | |
| "id": "c39d7174-e9d2-5b0b-b43e-cfb2b575d403" | |
| }, | |
| "yAxisUnit": "Bps" | |
| }, | |
| { | |
| "id": "596b38da-a5d3-5aae-96de-054fd50cfbf9", | |
| "title": "Go — allocation rate (bytes/s)", | |
| "description": "", | |
| "panelTypes": "graph", | |
| "timePreferance": "GLOBAL_TIME", | |
| "query": { | |
| "queryType": "builder", | |
| "builder": { | |
| "queryData": [ | |
| { | |
| "aggregations": [ | |
| { | |
| "metricName": "go.memory.allocations", | |
| "reduceTo": "avg", | |
| "spaceAggregation": "sum", | |
| "temporality": null, | |
| "timeAggregation": "rate" | |
| } | |
| ], | |
| "dataSource": "metrics", | |
| "disabled": false, | |
| "expression": "A", | |
| "filter": { | |
| "expression": "service.name = 'wavehouse'" | |
| }, | |
| "groupBy": [], | |
| "having": { | |
| "expression": "" | |
| }, | |
| "legend": "bytes/s", | |
| "orderBy": [], | |
| "queryName": "A", | |
| "stepInterval": 60 | |
| } | |
| ], | |
| "queryFormulas": [] | |
| }, | |
| "clickhouse_sql": [ | |
| { | |
| "name": "A", | |
| "query": "", | |
| "legend": "", | |
| "disabled": false | |
| } | |
| ], | |
| "promql": [ | |
| { | |
| "name": "A", | |
| "query": "", | |
| "legend": "", | |
| "disabled": false | |
| } | |
| ], | |
| "id": "c39d7174-e9d2-5b0b-b43e-cfb2b575d403" | |
| }, | |
| "yAxisUnit": "Bps" | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deployments/signoz/dashboards/wavehouse-runtime-internals.json` around lines
191 - 248, The Go allocation metric panel (query id
"c39d7174-e9d2-5b0b-b43e-cfb2b575d403", queryData entry with "queryName": "A"
and metric "go.memory.allocations") has an empty filter; add a service filter to
scope it to WaveHouse by populating the filter.expression with the appropriate
service.name condition (e.g., restrict to the WaveHouse service), leaving the
rest of the aggregation and legend intact.
| { | ||
| "id": "8828b51f-f0fc-56b2-a674-21d95d230501", | ||
| "title": "HTTP — request / response body size p99 (bytes)", | ||
| "description": "", | ||
| "panelTypes": "graph", | ||
| "timePreferance": "GLOBAL_TIME", | ||
| "query": { | ||
| "queryType": "builder", | ||
| "builder": { | ||
| "queryData": [ | ||
| { | ||
| "aggregations": [ | ||
| { | ||
| "metricName": "http.server.request.body.size.bucket", | ||
| "reduceTo": "avg", | ||
| "spaceAggregation": "p99", | ||
| "temporality": null, | ||
| "timeAggregation": "rate" | ||
| } | ||
| ], | ||
| "dataSource": "metrics", | ||
| "disabled": false, | ||
| "expression": "A", | ||
| "filter": { | ||
| "expression": "" | ||
| }, | ||
| "groupBy": [], | ||
| "having": { | ||
| "expression": "" | ||
| }, | ||
| "legend": "request p99", | ||
| "orderBy": [], | ||
| "queryName": "A", | ||
| "stepInterval": 60 | ||
| }, | ||
| { | ||
| "aggregations": [ | ||
| { | ||
| "metricName": "http.server.response.body.size.bucket", | ||
| "reduceTo": "avg", | ||
| "spaceAggregation": "p99", | ||
| "temporality": null, | ||
| "timeAggregation": "rate" | ||
| } | ||
| ], | ||
| "dataSource": "metrics", | ||
| "disabled": false, | ||
| "expression": "B", | ||
| "filter": { | ||
| "expression": "" | ||
| }, | ||
| "groupBy": [], | ||
| "having": { | ||
| "expression": "" | ||
| }, | ||
| "legend": "response p99", | ||
| "orderBy": [], | ||
| "queryName": "B", | ||
| "stepInterval": 60 | ||
| } | ||
| ], | ||
| "queryFormulas": [] | ||
| }, | ||
| "clickhouse_sql": [ | ||
| { | ||
| "name": "A", | ||
| "query": "", | ||
| "legend": "", | ||
| "disabled": false | ||
| } | ||
| ], | ||
| "promql": [ | ||
| { | ||
| "name": "A", | ||
| "query": "", | ||
| "legend": "", | ||
| "disabled": false | ||
| } | ||
| ], | ||
| "id": "ed4527e0-8071-5d8f-9d80-a3d2139b6dfa" | ||
| }, | ||
| "yAxisUnit": "bytes" | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
Add service.name filter to HTTP payload size metrics.
The HTTP body size metrics (lines 516, 541) have empty filters and will aggregate request/response sizes from all services. This is particularly important since http.server.* metrics are standard OTel semantic conventions that multiple services might emit.
📊 Suggested fix
Apply the filter to both queries in this widget:
"filter": {
- "expression": ""
+ "expression": "service.name = 'wavehouse'"
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| "id": "8828b51f-f0fc-56b2-a674-21d95d230501", | |
| "title": "HTTP — request / response body size p99 (bytes)", | |
| "description": "", | |
| "panelTypes": "graph", | |
| "timePreferance": "GLOBAL_TIME", | |
| "query": { | |
| "queryType": "builder", | |
| "builder": { | |
| "queryData": [ | |
| { | |
| "aggregations": [ | |
| { | |
| "metricName": "http.server.request.body.size.bucket", | |
| "reduceTo": "avg", | |
| "spaceAggregation": "p99", | |
| "temporality": null, | |
| "timeAggregation": "rate" | |
| } | |
| ], | |
| "dataSource": "metrics", | |
| "disabled": false, | |
| "expression": "A", | |
| "filter": { | |
| "expression": "" | |
| }, | |
| "groupBy": [], | |
| "having": { | |
| "expression": "" | |
| }, | |
| "legend": "request p99", | |
| "orderBy": [], | |
| "queryName": "A", | |
| "stepInterval": 60 | |
| }, | |
| { | |
| "aggregations": [ | |
| { | |
| "metricName": "http.server.response.body.size.bucket", | |
| "reduceTo": "avg", | |
| "spaceAggregation": "p99", | |
| "temporality": null, | |
| "timeAggregation": "rate" | |
| } | |
| ], | |
| "dataSource": "metrics", | |
| "disabled": false, | |
| "expression": "B", | |
| "filter": { | |
| "expression": "" | |
| }, | |
| "groupBy": [], | |
| "having": { | |
| "expression": "" | |
| }, | |
| "legend": "response p99", | |
| "orderBy": [], | |
| "queryName": "B", | |
| "stepInterval": 60 | |
| } | |
| ], | |
| "queryFormulas": [] | |
| }, | |
| "clickhouse_sql": [ | |
| { | |
| "name": "A", | |
| "query": "", | |
| "legend": "", | |
| "disabled": false | |
| } | |
| ], | |
| "promql": [ | |
| { | |
| "name": "A", | |
| "query": "", | |
| "legend": "", | |
| "disabled": false | |
| } | |
| ], | |
| "id": "ed4527e0-8071-5d8f-9d80-a3d2139b6dfa" | |
| }, | |
| "yAxisUnit": "bytes" | |
| } | |
| ] | |
| } | |
| { | |
| "id": "8828b51f-f0fc-56b2-a674-21d95d230501", | |
| "title": "HTTP — request / response body size p99 (bytes)", | |
| "description": "", | |
| "panelTypes": "graph", | |
| "timePreferance": "GLOBAL_TIME", | |
| "query": { | |
| "queryType": "builder", | |
| "builder": { | |
| "queryData": [ | |
| { | |
| "aggregations": [ | |
| { | |
| "metricName": "http.server.request.body.size.bucket", | |
| "reduceTo": "avg", | |
| "spaceAggregation": "p99", | |
| "temporality": null, | |
| "timeAggregation": "rate" | |
| } | |
| ], | |
| "dataSource": "metrics", | |
| "disabled": false, | |
| "expression": "A", | |
| "filter": { | |
| "expression": "service.name = 'wavehouse'" | |
| }, | |
| "groupBy": [], | |
| "having": { | |
| "expression": "" | |
| }, | |
| "legend": "request p99", | |
| "orderBy": [], | |
| "queryName": "A", | |
| "stepInterval": 60 | |
| }, | |
| { | |
| "aggregations": [ | |
| { | |
| "metricName": "http.server.response.body.size.bucket", | |
| "reduceTo": "avg", | |
| "spaceAggregation": "p99", | |
| "temporality": null, | |
| "timeAggregation": "rate" | |
| } | |
| ], | |
| "dataSource": "metrics", | |
| "disabled": false, | |
| "expression": "B", | |
| "filter": { | |
| "expression": "service.name = 'wavehouse'" | |
| }, | |
| "groupBy": [], | |
| "having": { | |
| "expression": "" | |
| }, | |
| "legend": "response p99", | |
| "orderBy": [], | |
| "queryName": "B", | |
| "stepInterval": 60 | |
| } | |
| ], | |
| "queryFormulas": [] | |
| }, | |
| "clickhouse_sql": [ | |
| { | |
| "name": "A", | |
| "query": "", | |
| "legend": "", | |
| "disabled": false | |
| } | |
| ], | |
| "promql": [ | |
| { | |
| "name": "A", | |
| "query": "", | |
| "legend": "", | |
| "disabled": false | |
| } | |
| ], | |
| "id": "ed4527e0-8071-5d8f-9d80-a3d2139b6dfa" | |
| }, | |
| "yAxisUnit": "bytes" | |
| } | |
| ] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deployments/signoz/dashboards/wavehouse-runtime-internals.json` around lines
492 - 576, The two metric query blocks for "A"
(http.server.request.body.size.bucket, legend "request p99") and "B"
(http.server.response.body.size.bucket, legend "response p99") currently have
empty filter.expression values and will aggregate across all services; update
both queryData entries to set filter.expression to restrict by service.name
(e.g. service.name = "<YOUR_SERVICE>" or a dashboard variable like
${service_name}) so each aggregation only includes the intended service.
EricAndrechek
left a comment
EricAndrechek
left a comment
There was a problem hiding this comment.
Please refactor into two PRs for the two issues, this is too large
## Summary Closes #121. Rebuilds `deployments/signoz/` from upstream SigNoz `v0.122.0` (single-node, version-pinned, no `:latest`), adds dashboards as code with an upsert loader, and wires a `make dev-obs` workflow. The hand-rolled `deployments/signoz/docker-compose.yaml` had bit-rotted: pinned `:latest` everywhere, used the wrong CLI shape for `signoz-otel-collector`'s migrator (`code: 62 Syntax error` on an empty `--clickhouse-cluster=""`), and the consolidated `signoz/signoz` image had superseded the old `query-service` + `frontend` split. - **`deployments/signoz/compose.yaml`** — upstream-mirrored single-node stack: - `signoz/signoz:v0.122.0` (consolidated UI/API on host `:3301`) - `signoz/signoz-otel-collector:v0.144.3` (OTLP gRPC `:4317`, HTTP `:4318`, health `:13133` via `bash /dev/tcp`) - `signoz-telemetrystore-migrator` one-shot (gates the UI + collector on `service_completed_successfully`) - `clickhouse-server:25.5.6` with vendored `cluster.xml` / `users.xml` / `custom-function.xml` - `zookeeper:3.7.1` - `init-clickhouse` one-shot fetches the `histogramQuantile` UDF binary (real upstream SHA256s for all four platform/arch combos SigNoz publishes; `set -euo pipefail` so wget failures abort) - `SIGNOZ_TOKENIZER_JWT_SECRET` is per-checkout: compose uses `${SIGNOZ_TOKENIZER_JWT_SECRET:?}` to fail fast with a remediation pointer on raw `docker compose`; the Makefile generates a random secret on first need via `openssl rand -hex 32` and caches it in `deployments/signoz/.signoz-secret.local` (gitignored, mode 0600). - **`deployments/signoz/dashboards/`** — two version-controlled dashboards (`wavehouse-overview.json`, `wavehouse-runtime-internals.json`); widgets scoped to `service.name=wavehouse` so they don't aggregate other services that may also report to the same SigNoz. `load-dashboards.sh` upserts by title — auth is **token-only** because SigNoz v0.122.0 moved password login to `/api/v2/sessions/email_password` and requires an org UUID that isn't externally discoverable; the JWT the SPA keeps in `localStorage` under `AUTH_TOKEN` is one DevTools click away. - **`Makefile`** — `##@ Observability` targets: `signoz-up/down/logs/wipe/dashboards`, plus `dev-obs` (`make dev` + `signoz-up` + WaveHouse pointed at the collector + dashboards auto-load when `SIGNOZ_TOKEN` is in env). - **`deployments/compose/standalone.signoz.yaml`** — override that wires the containerized `standalone.yaml` WaveHouse into the local SigNoz stack via `host.docker.internal:4317`, caps `WH_MQ_MAX_BYTES_GB=2` for small dev disks (OrbStack VM defaults to ~28 GB — the WaveHouse default of 50 GB fails JetStream init), and gates WaveHouse on a ClickHouse healthcheck. Split out of #135. The companion direct-to-cloud OTLP work is in #136. ## Test plan - [ ] `make signoz-up` brings the stack healthy (UI on `:3301`, OTLP gRPC on `:4317`) - [ ] First-user creation via the UI at http://localhost:3301, then `export SIGNOZ_TOKEN=...` (from `localStorage['AUTH_TOKEN']`) and `make signoz-dashboards` upserts both dashboards (first run creates, re-run updates by ID) - [ ] Bring WaveHouse up against it: `docker compose -p wavehouse -f deployments/compose/standalone.yaml -f deployments/compose/standalone.signoz.yaml up -d --build` - [ ] 5 min of mixed traffic populates panels on both dashboards (~93 RPS across 8+ routes, 200/400/404 mix, valid+large+malformed ingest) - [ ] `make signoz-down` followed by `make signoz-up` resumes cleanly (admin account preserved; SQLite volume not wiped) - [ ] `make signoz-wipe` rotates the JWT secret and starts from scratch - [ ] Reviewer: open http://localhost:3301/dashboard and spot-check WaveHouse panels render against your own run 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com> Co-authored-by: Eric Andrechek <eric@wave-rf.com>
Summary
deployments/signoz/compose.yaml) and Make targets (signoz-up/down/logs/wipe/dashboards, plusdev-obsformake dev+ SigNoz).Overview,Runtime & Internals) and ships a loader (load-dashboards.sh) that upserts them by title against the running stack.Closes #97 (cloud OTLP), #121 (SigNoz dev-stack cleanup +
dev-obs).Test plan
make signoz-upbrings the stack healthy (SigNoz UI on :3301, OTLP gRPC on :4317).make signoz-dashboardsupserts both dashboards (first run creates, re-run updates by ID); guard fails fast withoutSIGNOZ_TOKEN.docker compose -f standalone.yaml -f standalone.signoz.yaml up -dbrings WaveHouse + ClickHouse up and exports OTLP to the SigNoz collector viahost.docker.internal:4317.Summary by CodeRabbit
New Features
make dev-obscommand and pre-configured dashboards for monitoring traces, metrics, and logsDocumentation