feat(dpu): otel tracing support - #4886
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughThe collector configurations now accept DPU workload traces through a local OTLP/gRPC receiver, add workload resource attribution and batching, and export traces through ChangesDPU workload tracing
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to Tracing may fail when workloads and the collector do not share the localhost network namespace, and oversized trace batches could be rejected and dropped because no maximum is configured. The PR is mergeable with explicit owner awareness and follow-up on endpoint networking and receiver batch limits. Sequence Diagram(s)sequenceDiagram
participant DPUWorkload
participant NicoOTelcol
participant SiteOTelReceiver
DPUWorkload->>NicoOTelcol: Send OTLP/gRPC spans to 127.0.0.1:4317
NicoOTelcol->>NicoOTelcol: Enrich, label, and batch workload traces
NicoOTelcol->>SiteOTelReceiver: Export traces through otlp/site
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bluefield/otel/otel_config.yaml (2)
416-425: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFail the release when workload pipeline validation fails.
bluefield/otel/otelcol-wrapper(Lines [27]-[34]) logs a warning and starts without${ADDITIONAL_CONFIGS}when validation fails. A bad receiver or processor reference here can therefore disable workload tracing while the collector remains running. Validate this configuration and the rendered Helm configuration in CI. Fail the release when validation fails.As per path instructions, this BlueField YAML review checks failure recovery and observability coverage.
🤖 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 `@bluefield/otel/otel_config.yaml` around lines 416 - 425, Update validation around the traces/workloads pipeline and the bluefield/otel/otelcol-wrapper flow so invalid receiver or processor references cause a non-zero failure instead of starting without ADDITIONAL_CONFIGS. Add CI checks for both the BlueField YAML and the rendered Helm configuration, and ensure any validation failure fails the release.Source: Path instructions
248-250: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDefine the trace batch upper bound.
send_batch_sizetriggers a flush but does not cap the outgoing batch. If 8,192 spans is the required maximum, addsend_batch_max_size: 8192tobatch/tracesin both configuration files. Otherwise, document thatsend_batch_sizeis only a flush threshold.🤖 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 `@bluefield/otel/otel_config.yaml` around lines 248 - 250, Add send_batch_max_size: 8192 to the batch/traces processor in bluefield/otel/otel_config.yaml (lines 248-250) and bluefield/charts/nico-otelcol/files/otel_config.yaml (lines 251-253), ensuring 8,192 spans is the enforced outgoing batch maximum.Source: Path instructions
🤖 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.
Nitpick comments:
In `@bluefield/otel/otel_config.yaml`:
- Around line 416-425: Update validation around the traces/workloads pipeline
and the bluefield/otel/otelcol-wrapper flow so invalid receiver or processor
references cause a non-zero failure instead of starting without
ADDITIONAL_CONFIGS. Add CI checks for both the BlueField YAML and the rendered
Helm configuration, and ensure any validation failure fails the release.
- Around line 248-250: Add send_batch_max_size: 8192 to the batch/traces
processor in bluefield/otel/otel_config.yaml (lines 248-250) and
bluefield/charts/nico-otelcol/files/otel_config.yaml (lines 251-253), ensuring
8,192 spans is the enforced outgoing batch maximum.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 041b860a-6cd4-4e46-8436-48d210db1f3b
📒 Files selected for processing (2)
bluefield/charts/nico-otelcol/files/otel_config.yamlbluefield/otel/otel_config.yaml
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4886.docs.buildwithfern.com/infra-controller |
akorobkov-nvda
added
helm charts
bluefield
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/observability/tracing.md (1)
522-526: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPrefer trace-scoped OTLP variables.
OTEL_EXPORTER_OTLP_ENDPOINTandOTEL_EXPORTER_OTLP_PROTOCOLconfigure all OTLP signals. The documented collector change adds a traces pipeline. If a workload also emits metrics or logs, this example sends those signals to an endpoint that this section does not configure for them. Use the trace-scoped variables, or state that the endpoint is limited to trace-only workloads. (opentelemetry.io)Proposed fix
-OTEL_EXPORTER_OTLP_ENDPOINT="http://127.0.0.1:4317" -OTEL_EXPORTER_OTLP_PROTOCOL="grpc" +OTEL_EXPORTER_OTLP_TRACES_ENDPOINT="http://127.0.0.1:4317" +OTEL_EXPORTER_OTLP_TRACES_PROTOCOL="grpc" - - name: OTEL_EXPORTER_OTLP_ENDPOINT + - name: OTEL_EXPORTER_OTLP_TRACES_ENDPOINT ... - - name: OTEL_EXPORTER_OTLP_PROTOCOL + - name: OTEL_EXPORTER_OTLP_TRACES_PROTOCOLAlso applies to: 530-537
🤖 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 `@docs/observability/tracing.md` around lines 522 - 526, Update the tracing documentation examples using OTEL_EXPORTER_OTLP_ENDPOINT and OTEL_EXPORTER_OTLP_PROTOCOL to use the trace-scoped OTEL_EXPORTER_OTLP_TRACES_ENDPOINT and OTEL_EXPORTER_OTLP_TRACES_PROTOCOL variables, ensuring the configuration applies only to traces.
🤖 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 `@docs/observability/tracing.md`:
- Around line 522-526: Update the shell example’s OTEL_EXPORTER_OTLP_ENDPOINT
and OTEL_EXPORTER_OTLP_PROTOCOL assignments to use export so both variables are
inherited by child processes.
- Around line 528-543: Update the Kubernetes DPF tracing documentation around
the OTLP endpoint example to state that the collector uses hostNetwork and
127.0.0.1:4317 is reachable only by workloads sharing the node network
namespace, such as ovnkube-node. Document that loopback does not authenticate
callers within that namespace, and add the supported Service endpoint for
workloads that cannot use host networking.
---
Nitpick comments:
In `@docs/observability/tracing.md`:
- Around line 522-526: Update the tracing documentation examples using
OTEL_EXPORTER_OTLP_ENDPOINT and OTEL_EXPORTER_OTLP_PROTOCOL to use the
trace-scoped OTEL_EXPORTER_OTLP_TRACES_ENDPOINT and
OTEL_EXPORTER_OTLP_TRACES_PROTOCOL variables, ensuring the configuration applies
only to traces.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: bf4572c0-968f-49fc-9806-673b88f65041
📒 Files selected for processing (1)
docs/observability/tracing.md
04321f2 to
da06f41
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Related issues
Type of Change
Testing