[otel-agent] Use zstd compression for all signals - #52758
Conversation
|
@codex review |
7a7b1a1 to
044b22b
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a7b1a136d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Pin zstd for logs (matching metrics/traces) with an overridable level-3 default. | ||
| // compression_kind is set explicitly so a configured additional_endpoints can't | ||
| // silently fall back to gzip; zstd_compression_level is the level used for zstd. | ||
| pkgconfig.Set("logs_config.compression_kind", pkgconfigsetup.DefaultLogCompressionKind, pkgconfigmodel.SourceDefault) |
There was a problem hiding this comment.
When DDOT runs with logs_config.additional_endpoints configured (for example via DD_LOGS_CONFIG_ADDITIONAL_ENDPOINTS or synced core config), this SourceDefault value still makes LogsConfigKeys.compressionKind() treat logs_config.compression_kind as not configured, so it triggers the additional-endpoints compatibility fallback to gzip in comp/logs/agent/config/config_keys.go. That means logs stop using zstd in exactly the additional-endpoint case the new comment says this should prevent; set the DDOT zstd pin at a non-default source that still allows user overrides if needed.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 044b22b1f9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Pin zstd for logs (matching metrics/traces) with an overridable level-3 default. | ||
| // compression_kind is set explicitly so a configured additional_endpoints can't | ||
| // silently fall back to gzip; zstd_compression_level is the level used for zstd. | ||
| pkgconfig.Set("logs_config.compression_kind", pkgconfigsetup.DefaultLogCompressionKind, pkgconfigmodel.SourceDefault) |
There was a problem hiding this comment.
When logs_config.additional_endpoints is present and no explicit logs_config.compression_kind is provided, LogsConfigKeys.compressionKind() falls back to gzip unless IsConfigured("logs_config.compression_kind") is true. This assignment uses SourceDefault, which IsConfigured ignores, so DDOT logs with additional endpoints still use gzip even though this change intends to pin logs to zstd and the new E2E does not cover that configuration. Set this at a non-default/runtime source or adjust the fallback so additional-endpoint setups are covered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in e69c344. I removed the logs_config.compression_kind set entirely: as you noted, a SourceDefault value isn't IsConfigured(), so it could never prevent the additional_endpoints→gzip fallback. Logs already default to zstd via the registered DefaultLogCompressionKind, and that gzip fallback for additional endpoints is intentional (third-party endpoint compatibility), so we don't force it.
There was a problem hiding this comment.
Correction to my earlier reply — I've reversed this fix in af1aa56 after looking at how the core forwarder and the logs pipeline actually handle additional_endpoints:
- The gzip fallback (PR [AGNTLOG-9] Add compression config for different pipelines #35625) is a conservative default for non-Datadog intakes (its own release note: "compatibility issues with non-Datadog intakes"), not a technical limitation. It triggers on the mere presence of
additional_endpoints. - For DDOT,
additional_endpointsare other Datadog endpoints (multi-region / dual-ship / MRF), which accept zstd. The metrics forwarder already fans a single zstd-compressed payload to all domains (main + additional), and the logs pipeline likewise uses one compressor for all destinations (pipeline.gobuilds it fromendpoints.Main.CompressionKind).
So instead of accepting the gzip fallback, I now pin logs_config.compression_kind=zstd at SourceFile so IsConfigured() is true and the fallback is bypassed — every log endpoint ships zstd, consistent with metrics. It stays overridable via DD_LOGS_CONFIG_COMPRESSION_KIND=gzip for the rare case of a genuinely non-Datadog log endpoint. Added a unit test asserting logs_config.compression_kind is IsConfigured(). Thanks for the nudge — your original flag was right.
| // compression_kind is set explicitly so a configured additional_endpoints can't | ||
| // silently fall back to gzip; zstd_compression_level is the level used for zstd. | ||
| pkgconfig.Set("logs_config.compression_kind", pkgconfigsetup.DefaultLogCompressionKind, pkgconfigmodel.SourceDefault) | ||
| pkgconfig.Set("logs_config.zstd_compression_level", 3, pkgconfigmodel.SourceDefault) |
There was a problem hiding this comment.
If a user sets the existing OTel Datadog exporter logs.compression_level, ddc.Logs.CompressionLevel is still written to logs_config.compression_level, but after this line forces zstd the logs pipeline reads logs_config.zstd_compression_level instead. That means configured log compression levels that were honored with gzip are silently ignored and logs always use the new zstd default level unless users know to set a Datadog-Agent-specific zstd key. Translate the configured level to the zstd key when zstd is selected, or otherwise document/remove the now-ignored mapping.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e69c344. The exporter's logs.compression_level is a gzip level (0–9, per its own field docs), so I deliberately did not repurpose it as a zstd level. Instead I documented that it only applies when the active compressor is gzip; under the zstd default the level is logs_config.zstd_compression_level (default 3, overridable via DD_LOGS_CONFIG_ZSTD_COMPRESSION_LEVEL). Note this isn't a regression introduced here — DDOT logs already defaulted to zstd before this PR (the registered default), so logs.compression_level was already inert under the default kind; this change just makes the behaviour explicit and documented.
Go Package Import DifferencesBaseline: 711b8af
|
|
🎯 Code Coverage (details) 🔗 Commit SHA: 7fb2b2c | Docs | Datadog PR Page | Give us feedback! |
Files inventory check summaryFile checks results against ancestor 711b8afd: Results for datadog-agent_7.83.0~devel.git.641.7fb2b2c.pipeline.129819916-1_amd64.deb:No change detected Results for datadog-iot-agent_7.83.0~devel.git.641.7fb2b2c.pipeline.129819916-1_amd64.deb:No change detected |
Static quality checks✅ Please find below the results from static quality gates 33 successful checks with minimal change (< 2 KiB)
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 711b8af Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_security_idle | memory utilization | +0.51 | [+0.45, +0.57] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.32 | [+0.28, +0.36] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.29 | [+0.26, +0.32] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | +0.19 | [+0.15, +0.23] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | -0.02 | [-0.12, +0.07] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_logs | % cpu utilization | -0.13 | [-0.98, +0.72] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.32 | [-0.45, -0.20] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | -0.35 | [-0.61, -0.10] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 148.45MiB ≤ 154MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 735.27KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 496.56MiB ≤ 512MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.13MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 4 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 185.72MiB ≤ 195MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 264.02MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 355.41 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 3 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 378.36MiB ≤ 430MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.94GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 71.01MiB ≤ 75MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 27.73 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 306.23MiB ≤ 330MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 71.13 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 281.92MiB ≤ 310MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 21.38 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 283.35MiB ≤ 320MiB | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
Replicate Execution Details
We run multiple replicates for each experiment/variant. However, we allow replicates to be automatically retried if there are any failures, up to 8 times, at which point the replicate is marked dead and we are unable to run analysis for the entire experiment. We call each of these attempts at running replicates a replicate execution. This section lists all replicate executions that failed due to the target crashing or being oom killed.
Note: In the below tables we bucket failures by experiment, variant, and failure type. For each of these buckets we list out the replicate indexes that failed with an annotation signifying how many times said replicate failed with the given failure mode. In the below example the baseline variant of the experiment named experiment_with_failures had two replicates that failed by oom kills. Replicate 0, which failed 8 executions, and replicate 1 which failed 6 executions, all with the same failure mode.
| Experiment | Variant | Replicates | Failure | Logs | Debug Dashboard |
|---|---|---|---|---|---|
| experiment_with_failures | baseline | 0 (x8) 1 (x6) | Oom killed | Debug Dashboard |
The debug dashboard links will take you to a debugging dashboard specifically designed to investigate replicate execution failures.
❌ Retried Profiling Replicate Execution Failures (ddprof)
Note: Profiling replicas may still be executing. See the debug dashboard for up to date status.
| Experiment | Variant | Replicates | Failure | Debug Dashboard |
|---|---|---|---|---|
| quality_gate_idle_all_features | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle_all_features | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_logs | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | baseline | 10 | Oom killed | Debug Dashboard |
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
a5055d3 to
e69c344
Compare
The DDOT collector (otel-agent) previously used a different compressor per signal: zlib for metrics, gzip for traces, and zstd for logs. Switch all signals to zstd for a consistent compression algorithm. - Metrics: use the config-driven metrics compressor (metricscompression/fx) instead of the hardcoded zlib module; defaults to zstd level 3, overridable via serializer_zstd_compressor_level. - Logs: pin zstd and default zstd_compression_level to 3 (overridable via logs_config.zstd_compression_level). - Traces: use the existing fx-zstd module (zstd at BestSpeed); the trace compression level is intentionally fixed. The v2 metrics intake is preserved (use_v3_api.series.enabled stays false); moving series to v3 is a separate effort. Changes are scoped to cmd/otel-agent and do not affect the core Agent, trace-agent, host-profiler, or OSS exporter. Tests: - Unit guard that metrics and logs resolve to the same algorithm and the level stays overridable. - Integration test verifies traces ship valid zstd. - E2E test asserts metrics, traces, and logs reach fakeintake as zstd. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
e69c344 to
af1aa56
Compare
|
This pull request has been automatically marked as stale because it has not had activity in the past 15 days. It will be closed in 30 days if no further activity occurs. If this pull request is still relevant, adding a comment or pushing new commits will keep it open. Also, you can always reopen the pull request if you missed the window. Thank you for your contributions! |
Merging main surfaced two breakages (the diff was clean, but referenced symbols/deps changed underneath it): - pkgconfigsetup.DefaultCompressorKind / DefaultLogCompressionKind moved to the pkg/config/setup/constants package; reference them there. - Regenerate BUILD.bazel for the fx-otel->fx and fx-gzip->fx-zstd import swaps and the new DataDog/zstd test dependency (gazelle). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
What does this PR do?
Makes the DDOT collector (
otel-agent) compress every signal withzstd, replacing the previous per-signal mix (zlib for metrics, gzip for traces, zstd for logs):metricscompression/fx) instead of the hardcoded zlib module; defaults to zstd level 3, overridable viaserializer_zstd_compressor_level.logs_config.zstd_compression_levelto 3 (overridable).fx-zstdmodule (zstd atBestSpeed); the trace level is intentionally fixed.Also adds tests that guard against future per-signal compression divergence.
Motivation
The DDOT exporter used a different compression algorithm per signal, which is inconsistent and harder to reason about. Standardizing on
zstdgives a consistent algorithm across metrics/traces/logs and a better compression ratio, with the level configurable per signal.Describe how you validated your changes
dda inv otel-agent.buildpasses;dda inv linter.go→ 0 issues on changed packages;gofmtclean.cmd/otel-agent/config): asserts metrics and logs resolve to the same algorithm (zstd) at level 3, that the level stays env-overridable, and that host metadata uses that same serializer compressor (so it is zstd too).comp/otelcol/otlp/integrationtest): now decodes trace payloads as zstd — verifies traces ship valid zstd end-to-end. Passes locally (dda inv otel-agent.integration-test).TestOTelAgentComplete/TestOTLPCompression): asserts metrics, traces, and logs reach fakeintake withContent-Encoding: zstd(requires the e2e infra to run).Additional Notes
otel-agent. The core Agent, trace-agent, host-profiler, and OSS exporter are unaffected — they use their own compression modules (metricscompression/fx,fx-zstd,fx-gzip,fx-otelrespectively).use_v3_api.series.enabledstaysfalse); moving series to v3 is a separate effort./api/v2/series) acceptsContent-Encoding: zstd— the core Agent uses zstd→v3 for series by default, so this exact combination isn't exercised today. The new e2e test validates it against fakeintake.pkg/trace/writer/stats.go, shared with the trace-agent; out of scope here, separate conversation). The orchestrator/K8s-objects sub-exporter already uses zstd and is disabled by default. Internal exporter telemetry (metricsclient, COAT/gateway gauges) is scraped Prometheus/OTel-meter telemetry, not an intake submission.🤖 Generated with Claude Code