Repository navigation
Add spanmetricsconnector for RED metrics from Datadog traces - #3044
Conversation
The datadogreceiver contrib component is already compiled in and
documented (README's "Ingesting Datadog traces, metrics, and logs")
for accepting Datadog Agent traces, metrics, and logs directly into
HyperDX. There's no way to turn those ingested traces into span-derived
RED metrics (call counts, error rates, latency) once they land, since
spanmetricsconnector isn't part of the build.
This is a common downstream need for anyone using Datadog Agent traces
today: the same span data commonly needs both a "calls" counter and a
duration histogram broken out by service/operation/status, computed
once at the collector rather than per-dashboard-query. Latency
specifically benefits from spanmetricsconnector's exponential
histogram support (histogram.exponential.max_size), which HyperDX's
ClickHouse schema already stores natively via the dedicated
otel_metrics_exponential_histogram table - no schema changes needed on
that side.
Purely additive and opt-in, like the existing routingconnector: being
compiled in adds no runtime behavior on its own, only availability for
a user's own pipeline config (CUSTOM_OTELCOL_CONFIG_FILE in OpAMP mode,
or a standalone config) to wire it in, the same pattern the README
already documents for datadogreceiver itself ("add a datadog receiver
block to your collector config").
Verified against this repo's own documented build process (README's
"Step 3: Verify"): `docker build -f docker/otel-collector/Dockerfile
--target dev` succeeds, and the resulting binary's `components` output
confirms spanmetricsconnector v0.155.0 is compiled in.
|
@arj22 is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
🦋 Changeset detectedLatest commit: 83f867f The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Greptile SummaryThe PR compiles the OpenTelemetry span-metrics connector and documents opt-in standalone and OpAMP configurations for deriving RED metrics from traces.
Confidence Score: 4/5The PR is not yet safe to merge because the standalone configuration can silently disable native Datadog metric ingestion. The explicit metrics receiver list replaces the standalone pipeline's existing receivers, so users who already configured Datadog and copy the example retain trace-derived metrics but lose Datadog's native metrics route. Files Needing Attention: packages/otel-collector/README.md
|
| Filename | Overview |
|---|---|
| packages/otel-collector/README.md | Documents standalone and OpAMP span-metrics wiring, but the standalone metrics receiver override drops configured native Datadog metric ingestion. |
| packages/otel-collector/builder-config.yaml | Adds the pinned spanmetricsconnector module to the custom collector build. |
| .changeset/spanmetrics-connector.md | Records the additive connector capability as a minor collector package release. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
DD[Datadog receiver] --> T[Traces pipeline]
T --> SM[span_metrics connector]
SM --> M[Metrics pipeline]
M --> CH[ClickHouse]
DD -. native metrics route replaced by example .-> M
Reviews (10): Last reviewed commit: "Merge branch 'main' into feat/add-spanme..." | Re-trigger Greptile
PR Review3 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 3 minor 3 posted as inline comment(s) on the changed lines. Severity is the reviewer's own estimate and is used for ordering, not filtering. |
Deep ReviewScope: 3 files — Intent: Add the OTel ✅ No critical issues found. The only executable change is a one-line, additive module entry in 🟡 P2 — recommended
🔵 P3 nitpicks (1)
Reviewers (6): correctness, testing, maintainability, project-standards, previous-comments, orchestrator-verification (in-repo cross-reference and temporality-SQL checks). Testing gaps:
|
The README's "Included components" section is the canonical 1:1 inventory of every compiled component, but the Connectors table only listed forward and routing, leaving the newly-compiled spanmetricsconnector undiscoverable via the doc that's supposed to advertise it.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Verified against opampController.ts and config.yaml's own comment: in OpAMP supervisor mode, receivers/exporters on the default traces/metrics pipelines are set dynamically and overwrite (not merge with) whatever CUSTOM_OTELCOL_CONFIG_FILE adds there - the same mechanism already documented for why processors: is kept out of the remote config. Adding spanmetrics to those pipelines in OpAMP mode is silently dropped, no error. Documented the working pattern (separate, non-colliding pipeline names) for OpAMP mode, alongside the direct approach that does work in standalone mode.
| - gomod: | ||
| github.com/open-telemetry/opentelemetry-collector-contrib/connector/routingconnector | ||
| v__OTEL_COLLECTOR_VERSION__ | ||
| - gomod: |
There was a problem hiding this comment.
🔵 minor — No smoke test for the documented spanmetrics wiring, though the fixture harness already exists
smoke-tests/otel-collector/ already runs an otel-collector-custom service that loads custom-pipeline-config.yaml via CUSTOM_OTELCOL_CONFIG_FILE and asserts pipeline wiring took effect (custom-pipeline-processors.bats). A parallel fixture + bats test that emits a span and asserts a calls/duration row lands in ClickHouse would cost a few lines and would have caught the missing connectors: block above; add one rather than relying on components output, which only proves the module was compiled in.
There was a problem hiding this comment.
Fair point, real suggestion — didn't add the full bats smoke test yet (it wasn't as clearly a must-have as the two config bugs above, and a real cross-signal trace→metric test is a bit more involved than the existing same-signal fixtures). What I did instead, given the same underlying goal of "prove the wiring actually works" rather than just components output: validated both corrected snippets against the real compiled binary via otelcontribcol validate --config, including reproducing both failure modes from your other comments verbatim before fixing them. That covers config-shape correctness but not an actual emitted span landing as a calls/duration row in ClickHouse, which your suggested test would catch. Happy to add the bats test as a follow-up if you'd still like it — didn't want to assume the scope without checking first.
Three real bugs in the examples added last commit, all confirmed by running otelcontribcol validate --config against the actual compiled binary rather than just fixing on faith: - Missing top-level `connectors:` block. A connector referenced only in a pipeline's receivers/exporters fails to start: "service::pipelines::traces: references exporter \"spanmetrics\" which is not configured" - reproduced verbatim. - OpAMP example referenced the `datadog` receiver unconditionally, but it only exists when ENABLE_DATADOG_RECEIVER=true. Reproduced the exact failure this causes too: an undefined receiver fails the entire config's validation, not just the pipeline that names it. - Missing `processors: [memory_limiter, batch]`, the convention every other user-added pipeline in this repo follows. Also promoted the section from a Datadog-nested `###` to its own `##` - spanmetrics works with any trace source, not just Datadog, and was unfindable for OTLP-only users nested under the Datadog auth subsection. Both corrected shapes pass `otelcontribcol validate --config` cleanly.
Three more real issues, all verified against the actual compiled binary rather than fixed on faith: - spanmetrics aggregates unbounded by default, and memory_limiter doesn't protect its internal dimension map (that only gates incoming pipeline data, not a connector's own separate state) - on a Datadog-firehose source, high-cardinality span names would grow that map until OOM regardless of memory_limiter's own setting. Added aggregation_cardinality_limit to both examples. Confirmed the field name is real (not guessed) by validating it against the compiled binary, and separately confirmed `validate` does real strict field checking by watching it reject a made-up field name. - With no `namespace`, the connector emits bare `calls`/`duration` metric names into the same shared otel_metrics_sum/otel_metrics_histogram tables every other source writes to, keyed by MetricName - a real collision risk. Added `namespace: traces.span.metrics` to both examples, matching the naming convention already reverse-engineered from a real Chronosphere gateway config. - The claim that processors: [memory_limiter, batch] matters on all four example pipelines didn't hold: standalone traces/metrics already inherit it from the base config (re-declaring is a no-op, per this README's own "only re-declare what you're retuning" convention), and OpAMP's traces/spanmetrics has no ClickHouse exporter, so batching does nothing there. Removed the redundant re-declaration from the standalone example and rescoped the prose to what's actually true. Verified the standalone example still merges and validates correctly without the redundant processors line, using the same multi --config invocation the real container uses (base config.yaml + standalone-config.yaml + this example).
- Standalone example: re-list `datadog` in both traces/metrics receiver lists, since confmap replaces pipeline lists wholesale rather than appending - previously silently dropped Datadog metric ingestion for anyone following the earlier datadogreceiver section too. Verified fixed shape validates cleanly via otelcontribcol validate --config (exit 0, with datadog receiver actually defined). - Add metrics_expiration: 5m alongside aggregation_cardinality_limit in both examples - the connector never expires aggregations by default, so a cap alone converts an OOM risk into permanent silent data loss for new series once a burst fills the map. Verified as a real field via validate (exit 0). - Drop `batch` from OpAMP example's traces/spanmetrics pipeline (keep memory_limiter only) - that pipeline's only exporter is the connector itself, not network I/O, so batching adds latency/memory for no benefit. Verified still validates cleanly with batch removed. - Changeset: patch -> minor, matching the datadogreceiver precedent (CHANGELOG.md 2.30.0 - a compiled-in opt-in component addition is a minor bump). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…rics examples Both spanmetrics examples referenced `datadog` unconditionally in a receiver list, guarded only by a comment - copying either example verbatim without separately enabling/configuring Datadog broke config validation entirely (`references receiver "datadog" which is not configured`, failing every pipeline, not just the one referencing it). Removed `datadog` from both executable examples and moved the conditional-add guidance into a comment on the line it actually applies to. Verified both corrected examples validate cleanly (exit 0) against the real compiled binary, using the same multi --config merge order the container actually uses (bootstrap + standalone-config + custom for the standalone case; bootstrap + custom + a reproduction of opampController.ts's real remote-config shape for the OpAMP case). Also re-verified the disputed "silently dropped, no error" OpAMP pitfall claim by building that exact bootstrap+custom+remote merge with spanmetrics wired into the default traces/metrics pipelines: it validates with exit 0, and a separate control test (a connector used as an exporter but never as a receiver) does produce a real error (`connector "spanmetrics" used as exporter in [traces] pipeline but not used in any supported receiver pipeline`), confirming validate does perform genuine connector-usage checks and that the merged, fully-unreferenced case is a real silent no-op, not a validation gap. No prose change needed there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| processors: [memory_limiter, batch] | ||
| ``` | ||
|
|
||
| Both shapes above are verified against the real compiled binary via |
There was a problem hiding this comment.
🔵 minor — Documented wiring is only verified by validate --config, though a smoke-test harness for exactly this pattern already exists
otelcontribcol validate --config proves the YAML parses and every component name resolves; it does not prove a span produces a calls/duration row in ClickHouse, which is the whole point of the section. The repo already has the harness for this: smoke-tests/otel-collector/custom-pipeline-processors.bats + custom-pipeline-config.yaml + the otel-collector-custom service (smoke-tests/otel-collector/docker-compose.yaml:83, wired into setup_suite.bash) cover the analogous documented CUSTOM_OTELCOL_CONFIG_FILE pattern end-to-end. Add a fixture that emits a span via emit_otel_data and asserts a row in otel_metrics_sum with MetricName = 'traces.span.metrics.calls' (an assert_query.sql/expected.snap pair under data/), which would also catch the temporality/expiry interaction above.
There was a problem hiding this comment.
Same ask as the earlier open thread on this section (databaseId 3901295654) — leaving this one open alongside it rather than resolving, since it's the same deferred item (a bats fixture asserting a calls/duration row lands in ClickHouse), not a separate piece of work. Tracked in the PR description's "Open item" section.
Verified each claim against the pinned v0.155.0 connector source (github.com/open-telemetry/opentelemetry-collector-contrib, tag connector/spanmetricsconnector/v0.155.0) and the real compiled binary, not just the review's say-so: - Use `span_metrics`, not `spanmetrics`, as the connector name in new examples. Confirmed via `internal/metadata/generated_status.go`: Type = "span_metrics" is canonical, DeprecatedType = "spanmetrics" is a legacy alias (still works, registered as such in `components` output) - new docs shouldn't teach the deprecated name. - Fixed the `namespace` rationale: confirmed via factory.go that `DefaultNamespace = "traces.span.metrics"` is already the default, so omitting it does NOT produce bare `calls`/`duration` names as previously claimed. Reworded to "pins the default for explicitness." - Replaced `metrics_expiration` with `series_expiration` for the cardinality-cap pairing. Confirmed via config.go doc comments and connector.go's ExpireSeries call sites: `metrics_expiration` only retires a whole metric object once no spans arrive at all for that service+operation; `series_expiration` is the one that evicts individual stale dimension combinations from the cardinality-bounded map while the metric stays otherwise active - which is what the described cardinality-burst scenario actually needs. - Added `aggregation_temporality: AGGREGATION_TEMPORALITY_DELTA`. Cumulative (the connector default) deletes a series' accumulator on expiration, so its next occurrence restarts the `calls` counter from 1 - a real counter reset that HyperDX's own cumulative-rate SQL (packages/common-utils/src/core/renderChartConfig.ts, confirmed by reading it: `greatest(Value - lagInFrame(Value), 0)`) doesn't detect as a reset, it just clamps the delta to 0. For a low-traffic series that keeps expiring between spans, every value stays 1 and every computed delta is 0 forever - a real "increase" chart under-report, not a hypothetical one. Delta temporality passes Value straight through with no diffing, sidestepping this entirely, and matches Chronosphere's own real-world gateway config. - Added `histogram.exponential.max_size: 160` to both examples - confirmed via connector.go's initHistogramMetrics that the default (no `histogram.exponential` set) is an *explicit*-bucket histogram, not the exponential one the PR description's ClickHouse otel_metrics_exponential_histogram claim depends on. Without this field the examples didn't actually deliver the benefit the PR describes. - Broadened the OpAMP example's `datadog` guard comment: it previously said "if and only if ENABLE_DATADOG_RECEIVER=true", which is wrong for a user who defines their own `datadog` receiver via CUSTOM_OTELCOL_CONFIG_FILE instead of the env var - confmap merges receiver maps key-by-key regardless of how a key got there. All five fields verified together against the real compiled binary via `otelcontribcol validate --config` (both standalone and OpAMP merge shapes, exit 0). The standalone shape was also actually started (not just validated): reaches "Everything is ready. Begin running and processing data." with the span_metrics connector live. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ics section - Standalone example put the conditional `datadog` guard on `metrics: receivers:`, but Datadog *trace* data only reaches `span_metrics` via `traces: receivers:` (span_metrics is an exporter on the traces pipeline, not metrics). Adding `datadog` to metrics.receivers only affects Datadog's own native-metrics ingestion, unrelated to span-derived metrics - moved the guard and made `traces:` set `receivers:` explicitly instead of leaving it implicit. Verified both the bare example (exit 0) and the same shape with a real `datadog` receiver defined and added to `traces.receivers` (exit 0) against the compiled binary. - Reworded the "Datadog traces have no other way to produce these" claim - HyperDX already computes RED metrics at query time from the traces table for any trace source (packages/app/src/serviceDashboard.ts's getExpressions/isSpanKindServer). The real motivation is precomputing once at the collector plus the exponential-histogram latency series. - Updated the Connectors summary table's `spanmetrics` row to `span_metrics` for consistency with the rest of the section. - Dropped the two "Chronosphere's own gateway collector" appeals (including a dangling ", again" referencing an unshown review thread) - both claims already stand on the in-repo/upstream reasoning stated right before them. Also re-verified two disputed bot claims from this same review round against a freshly rebuilt image before touching anything: - "`span_metrics` is not a valid connector type, there's no aliasing mechanism" - false. `components` output lists `span_metrics` as the registered name; `validate --config` accepts it (error messages for a bad field even correctly attribute it to `'spanmetricsconnector.Config'`); the connector's own factory.go calls `xconnector.WithDeprecatedTypeAlias(metadata.DeprecatedType)`, which is an explicit aliasing mechanism registering `spanmetrics` as a working deprecated alias alongside the canonical `span_metrics`. Verified both names validate successfully independently. - "`series_expiration` is not a real field" - false. Present in the pinned v0.155.0 config.go with its own doc comment and validation branch, used in connector.go via `ExpireSeries()`; validate accepts it, and a deliberate typo of it (along with another made-up field) is correctly rejected in the same error message, confirming validate's key-checking is real and this field passes because it exists, not because validation is lax. No doc changes made for either - the existing content was already correct. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| processors: [memory_limiter, batch] | ||
| ``` | ||
|
|
||
| Both shapes above are verified against the real compiled binary via |
There was a problem hiding this comment.
🔵 minor — The documented wiring has no smoke test, though the existing CUSTOM_OTELCOL_CONFIG_FILE bats harness covers exactly this case
smoke-tests/otel-collector/custom-pipeline-processors.bats plus smoke-tests/otel-collector/custom-pipeline-config.yaml already do the whole job this section needs: mount a custom config via CUSTOM_OTELCOL_CONFIG_FILE on the otel-collector-custom compose service, emit OTLP data, and assert the resulting rows in ClickHouse. Adding a fixture that emits a span and asserts a traces.span.metrics.calls row lands in otel_metrics_sum would run the documented config against the real binary in CI — which is what the two config errors above needed, since validate on the actual binary rejects both. Copy the existing fixture pattern rather than relying on the manual otelcontribcol validate claim in this paragraph.
There was a problem hiding this comment.
Same deferred item as the two earlier smoke-test threads on this PR - not a new piece of work, tracked in the PR description's "Open item" section. Leaving open alongside them rather than resolving.
|
@wrn14897 could you take a look at this one when you get a chance? Should the new Computing span metrics section move to its own doc, or is inline fine given other sections are also long? |
|
hey @arj22, I will take a look at it. Thanks for the contribution! |
| metrics: | ||
| receivers: [otlp/hyperdx, span_metrics] |
There was a problem hiding this comment.
When a standalone user has configured the Datadog receiver and copies this example, the explicit metrics.receivers list replaces the existing list without preserving datadog, causing native Datadog metrics to stop reaching ClickHouse even though trace-derived metrics continue working.
| - gomod: | ||
| github.com/open-telemetry/opentelemetry-collector-contrib/connector/routingconnector | ||
| v__OTEL_COLLECTOR_VERSION__ | ||
| - gomod: |
There was a problem hiding this comment.
🔵 minor — New connector and both documented configs have no automated coverage, though the smoke suite exists precisely for this
smoke-tests/otel-collector/ already boots a standalone collector with CUSTOM_OTELCOL_CONFIG_FILE (the otel-collector-custom service, docker-compose.yaml:79-101) for the stated purpose of verifying "the swap pattern documented in packages/otel-collector/README.md works end-to-end", and metrics.bats already asserts metric rows land in ClickHouse via wait_for_rows. Add a fixture mirroring the documented standalone block plus a bats test that emits a span and waits for rows in otel_metrics_sum (traces.span.metrics.calls) and otel_metrics_exponential_histogram; without it, nothing catches the connector being dropped from the manifest or the config key/field names (span_metrics, series_expiration, aggregation_cardinality_limit) changing on the next OTEL_COLLECTOR_VERSION bump, since the README's "Upgrading the OTel Collector version" checklist doesn't mention re-verifying this section either.
| # than adding to it. | ||
| receivers: [otlp/hyperdx] | ||
| exporters: [clickhouse, span_metrics] | ||
| metrics: |
There was a problem hiding this comment.
🔵 minor — Standalone example silently drops the datadog receiver from the metrics pipeline
The traces: entry carries a "add datadog here too … this list replaces the base one" comment, but metrics: receivers: [otlp/hyperdx, span_metrics] does not — and the Datadog section above (README.md:146-148) tells standalone users to add datadog to the traces, metrics, and logs/in receivers. A Datadog user copying this block replaces their metrics receiver list and silently stops ingesting Datadog native metrics; the prose at README.md:243-245 ("putting datadog on metrics: receivers: instead … does nothing for span-derived metrics") makes that outcome more likely, since it reads as a reason to leave it off. Repeat the same warning comment on the metrics: list.
| receivers: [otlp/hyperdx] | ||
| exporters: [span_metrics] | ||
| processors: [memory_limiter] | ||
| metrics/spanmetrics: |
There was a problem hiding this comment.
🔵 minor — Span-derived metrics never reach the PromQL remote-write path, which the section doesn't mention
Both examples route span metrics only to the ClickHouse exporter. On deployments where PromQL is enabled, the remote-write pipeline receives only otlp/hyperdx (opampController.ts:348-352, and docker/otel-collector/config.standalone.promql.yaml:19-22), so traces.span.metrics.* is queryable from the metrics source but absent from PromQL charts with no error anywhere. Add a sentence noting that a user who wants these series in PromQL must also add span_metrics as a receiver on a pipeline exporting to prometheusremotewrite.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary
spanmetricsconnectorto HyperDX's custom collector build.@hyperdx/otel-collector.Test plan
spanmetricsconnector v0.155.0is present in the resulting binary.Implementation notes / OpAMP wiring details
In OpAMP mode,
receivers/exporterson the defaulttraces/metricspipelines are supplied dynamically and overwrite the corresponding values fromCUSTOM_OTELCOL_CONFIG_FILE. Because of that, span-metrics wiring must use separate pipeline names rather than modifying the default pipelines.The documented config sets a few fields beyond the bare minimum:
namespace(pins the connector's own default for explicitness),histogram.exponential.max_size(opts into the exponential histogram this PR is motivated by, vs. the connector's default explicit-bucket histogram),aggregation_cardinality_limit+series_expiration(bounds the connector's own in-memory state, whichmemory_limiterdoesn't cover), andaggregation_temporality: DELTA(cumulative temporality plus expiration can reset a low-traffic series' counter in a way HyperDX's rate SQL doesn't detect; delta avoids the issue).Two specific claims came up more than once across review and were checked directly against the pinned v0.155.0 source and the compiled binary — both turned out to be incorrect, noted here so they aren't re-litigated: the connector's canonical config key is
span_metrics(spanmetricsis a working deprecated alias, not the other way around), andseries_expirationis a real, distinct field frommetrics_expiration.Follow-up
An end-to-end smoke test that emits a span and asserts generated
calls/durationmetrics reach ClickHouse would provide additional coverage beyond collector config validation.