fix(telemetry): report tracesUrl and explain trace disconnects - #534
Merged
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances the ClickHouse probing mechanism to capture and report specific connection errors when traces are down, propagating them to the telemetry payload. The feedback highlights a potential bug where full URLs in CLICKHOUSE_HOST could result in malformed probe URLs, and suggests robust URL parsing to handle this. Additionally, it is recommended to fully drain the HTTP response body in httpProbeErr to prevent connection leaks, along with adding the necessary imports.
mayankpande88
previously approved these changes
Jul 16, 2026
The agent never populated tracesUrl, and never said why traces were down. tracesUrl: the legacy sink passed clickhouse_url (CLICKHOUSE_HOST) into get_trace_url(); the Go port hardcoded that path to "" on the premise that we no longer run a local ClickHouse. main.go does read and probe CLICKHOUSE_HOST, so the premise was stale and the field shipped empty for every otel_clickhouse cluster. That left agent_service.go's `TracesUrl != nil` gate permanently closed, so the trace-table config was never written and Last9 clusters resolved `otel_traces` instead of `otel.traces`. Add isClickHouseEnabled as the otel_clickhouse counterpart of isJaegerEnabled, and report the host. ClickHouse is checked last in traceURL rather than first as in the legacy: the legacy order returns the ClickHouse host even when TRACE_TABLE makes the provider bigquery, and the backend then quotes that host as a BigQuery table. It is also deliberately not used by traceStatus — gating tracesEnabled on the URL would turn traces off for TRACES_ENABLED=true-without-host, a config the agent supports for external ClickHouse it cannot probe. tracesConnectionError: the field was declared (agent_service.go) and rendered (agentHealth.jsx renderReason) but emitted by nobody, so a Disconnected Traces pill never showed a reason. probeClickhouse now returns one alongside the status; httpProbe is split over a new httpProbeErr so the failure survives, leaving its other callers untouched. Credentials in a URL-form CLICKHOUSE_HOST are stripped before the reason ships, since it renders verbatim in the UI. The field intentionally omits `omitempty`: the collector merges activity_stats into connection_status with jsonb `||`, so an omitted key leaves the previous value in place. A recovered ClickHouse must post an explicit "" to clear the stale reason. Fixes #34231 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mayankpande88
force-pushed
the
fix/agent-traces-url-and-connection-error
branch
from
July 16, 2026 05:17
aed1622 to
5cf4e2b
Compare
Contributor
|
📦 Image Tags Updated |
mangglesh
approved these changes
Jul 16, 2026
mayankpande88
approved these changes
Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related telemetry gaps: the agent never reported
tracesUrl, and never explained why traces were disconnected.1.
tracesUrlwas always empty forotel_clickhouseclusters. The legacy sink passedclickhouse_url(CLICKHOUSE_HOST) intoget_trace_url()(nudgebee_sink.py:1381). The Go port hardcoded that path to"", with a comment claiming "we don't run a local ClickHouse, so it's always ''" — stale, sincemain.godoes read and probeCLICKHOUSE_HOST.That left
agent_service.go:235'sTracesUrl != nilgate permanently closed, soTraceProviderConfig["otel_traces"]was never written and Last9 clusters resolved tableotel_tracesinstead ofotel.traces.tool_trace_clickhouse.go:136missed the samelast9.iocheck. Standard ClickHouse was unaffected —GetTracesTableNamesdefaults to the same value.Adds
isClickHouseEnabledas theotel_clickhousecounterpart ofisJaegerEnabled, and reports the host.2.
tracesConnectionErrorwas declared and rendered, but emitted by nobody. Declared atagent_service.go:94, read atagentHealth.jsx:198, rendered byrenderReason(!isTracesManagerConnected, tracesError). SincerenderReasonneedsdisconnected && errorand the value was permanently'', the "Reason - ..." line under a Disconnected Traces pill has never once rendered.probeClickhousenow returns a reason alongside its status.tracesConnectionError""ClickHouse ping failed at ch.svc:8123: connection refusedCLICKHOUSE_HOSTunsetCLICKHOUSE_HOST is not set: no traces backend is configuredTRACES_ENABLED=false""— off on purpose isn't a failure""No backend or UI change needed; both consumers already exist.
Type of change
Chart version
Runner-only Go change, no chart templates touched. The closest precedent — #517, which also added a telemetry field — didn't bump either. Shout if that's wrong and I'll add one.
Test plan
go build ./...,go vet, and the full agent suite pass (relevant: thehttpProbesplit below touches every probe caller)redactUserinfo, reason-clears-on-recovery, and a marshalling test pinning the no-omitemptycontracthelm lint/ct lint/helm templatenot applicable (no chart changes).Review Notes → Risks & Counterarguments
httpProberefactor touches every probe caller. Split into a thinboolwrapper over a newhttpProbeErrthat preserves the error, so Grafana/OpenCost/logs callers are untouched by construction. Full suite passes. Behaviour is identical, but it's the widest-blast-radius hunk here.Two deliberate deviations from legacy parity, both documented in-code:
traceURL, not first. The legacy order returns the ClickHouse host even whenTRACE_TABLEmakes the providerbigquery, and the backend then backtick-quotes that host as a BigQuery table name.isClickHouseEnabledis deliberately not used bytraceStatus. GatingtracesEnabledon the URL would turn traces off forTRACES_ENABLED=true-without-host — a configmain.go:1288explicitly supports for external ClickHouse the agent can't probe.The no-
omitemptyontracesConnectionErroris load-bearing. The collector merges intoconnection_statuswith jsonb||(telemetry_handler.py:267), so keys are sticky. An omitted key would strand a stale reason in the DB forever, resurfacing under any later failure that produces no message. A recovered ClickHouse must post an explicit""to clear it. There's a marshalling test guarding this, because it's exactly what a future "add omitempty" cleanup would silently break.Newly-activated backend branch. Populating
tracesUrlmakesagent_service.go:235pass its gate for the first time and start writingTraceProviderConfig["otel_traces"]. For standard ClickHouse the written value equals the existing default, so no behaviour change; Last9 correctly switches tootel.traces. I traced the consumers but haven't exercised this against a live agent.Credential exposure was a real risk here. The reason string lands in
connection_statusJSON and renders verbatim in the UI, andCLICKHOUSE_HOSTmay be a full URL.redactUserinfostripsuser:pass@andprobeCauseunwraps*url.Errorso the request URL never reaches the string. Belt-and-braces — net/http already strips passwords — but the username survived that.Pre-existing bug, deliberately not fixed:
probeClickhousebuildsfmt.Sprintf("http://%s:%s/ping", host, port), which breaks for a URL-formCLICKHOUSE_HOST(http://https://x:8123/ping);pkg/clickhouse.normalizeHostexists precisely for that. Those clusters already reported traces disconnected — the change is that they now get a "ping failed" reason that's true but points at the wrong culprit. In practice Last9-style setups sidestep it viaTRACES_ENABLED=true. Worth a follow-up routing the probe throughnormalizeHost.Related issues
Fixes nudgebee/nudgebee-enterprise#34231
Refs nudgebee/nudgebee-enterprise#34172 — possibly related (Traces page null for
clickhouse otel), deliberately not linked as a fix. This only changes real behaviour for Last9 hosts, so it's unlikely to be that root cause unless that env is Last9-backed.🤖 Generated with Claude Code