Add design proposal: Distributed tracing via OTLP and VictoriaTraces#38
Add design proposal: Distributed tracing via OTLP and VictoriaTraces#38scooby87 wants to merge 9 commits into
Conversation
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds a distributed tracing design proposal covering VictoriaTraces storage, staged OTLP ingestion, Grafana correlation, per-application opt-in, tenant isolation, testing, and rollout behavior. ChangesDistributed tracing proposal
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 a design proposal for integrating distributed tracing into the Cozystack monitoring stack using OTLP and VictoriaTraces. The feedback recommends making the replica counts configurable to prevent scheduling failures in resource-constrained environments, addressing cross-chart dependency issues when tenant apps have tracing enabled but no backend is configured, and supporting both VTCluster and VTSingle deployments to align with existing patterns.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| vtinsert: { replicaCount: 2 } | ||
| vtselect: { replicaCount: 2 } | ||
| vtstorage: | ||
| retentionPeriod: {{ .retentionPeriod | quote }} | ||
| replicaCount: 2 |
There was a problem hiding this comment.
Hardcoding replicaCount: 2 for the VictoriaTraces components in the template can lead to scheduling failures in single-node or resource-constrained environments (such as local development or small testing clusters) due to pod anti-affinity or resource limits. Making the replica counts configurable within the tracingStorages schema, or dynamically defaulting them based on the cluster topology, would improve deployment flexibility.
| vtinsert: { replicaCount: 2 } | |
| vtselect: { replicaCount: 2 } | |
| vtstorage: | |
| retentionPeriod: {{ .retentionPeriod | quote }} | |
| replicaCount: 2 | |
| vtinsert: { replicaCount: {{ .replicaCount | default 2 }} }\n vtselect: { replicaCount: {{ .replicaCount | default 2 }} }\n vtstorage:\n retentionPeriod: {{ .retentionPeriod | quote }}\n replicaCount: {{ .replicaCount | default 2 }} |
|
|
||
| ## Failure and edge cases | ||
|
|
||
| - Empty `tracingStorages` while a consumer expects a backend → loud render failure (mirrors `cozystack/cozystack#3181`), never a silent span black hole. |
There was a problem hiding this comment.
While failing the render of the monitoring chart on an empty tracingStorages list prevents a silent black hole within that release, tenant applications (which are separate Helm releases) won't be aware of this at render time. If a tenant app has tracing.enabled: true but tracingStorages is empty, the app will attempt to send traces to a non-existent collector or backend, potentially causing connection timeouts or DNS errors. Detailing how this cross-chart dependency is managed, or if the platform collector should always be present (perhaps with a dummy/noop exporter) to safely discard spans when no backend is configured, would make the architecture more robust.
| ## Open questions | ||
|
|
||
| - **VictoriaTraces maturity**: the CRDs ship in the operator, but is VictoriaTraces production-ready at the version Cozystack pins? If not, Grafana Tempo is the drop-in fallback (see Alternatives) — the collector and per-app surfaces are backend-agnostic, so only the backend template and datasource type change. | ||
| - **`VTCluster` vs `VTSingle`** as the default: cluster for HA parity with metrics/logs, or single for a lighter footprint on small clusters? |
There was a problem hiding this comment.
Regarding the choice between VTCluster and VTSingle: to align with Cozystack's existing patterns for VictoriaMetrics and VictoriaLogs, supporting both is highly recommended. A VTSingle deployment is ideal for edge, single-node, or development environments to minimize overhead, while VTCluster should be used for production environments requiring high availability and scalability.
…ture Direct-to-vtinsert MVP (mirrors logs) then an opt-in OpenTelemetry Collector gateway; ExternalName redirect via cozystack-basics; poller readiness gate on VTCluster; clarify WorkloadMonitor is operational-only so tracing opt-in lives in app values. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
design-proposals/distributed-tracing/README.md (2)
126-128: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftSpecify how RED/span metrics are produced.
Direct Stage A ingestion creates no metrics, and the proposal does not define a Collector spanmetrics connector, application metric source, exemplars, or Grafana
tracesToMetricsqueries. Grafana’s trace-to-metrics feature links to pre-existing metrics; it does not generate them automatically. (grafana.com)Also applies to: 193-195
🤖 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 `@design-proposals/distributed-tracing/README.md` around lines 126 - 128, Update the distributed tracing proposal’s RED/span metrics design to explicitly specify their source and production path, including whether they come from a Collector spanmetrics connector or application instrumentation. Define how exemplars are attached and provide the Grafana tracesToMetrics queries/configuration that consume the resulting metrics, covering both the trace-to-metrics and metrics-to-trace links.Source: MCP tools
72-90: 🩺 Stability & Availability | 🔵 TrivialDo not equate
vtstorage.replicaCountwith HA replication.VictoriaTraces cluster mode does not replicate data between storage nodes; losing one node can make complete queries fail. If “HA parity” is intended, document this limitation and use Collector replication to independent VictoriaTraces clusters or another supported durability design. (docs.victoriametrics.com)
Also applies to: 208-208
🤖 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 `@design-proposals/distributed-tracing/README.md` around lines 72 - 90, Update the VTCluster template documentation and configuration guidance around the tracingStorages render to state that vtstorage.replicaCount provides no data replication or HA durability. Document the limitation and direct HA deployments to replicate through Collectors into independent VictoriaTraces clusters or use another supported durability design; do not present the existing replicaCount settings as HA parity.Source: MCP tools
🤖 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 `@design-proposals/distributed-tracing/README.md`:
- Around line 72-103: The tracing template must distinguish an absent
tracingStorages value from an explicitly empty list and enforce the intended
behavior consistently. Update the VTCluster rendering template and related tests
so the chosen contract is encoded: preserve successful rendering when the list
is absent if that remains supported, but fail rendering for an explicitly empty
list; ensure tests cover both cases and the guard produces a clear failure.
- Around line 186-187: Update the “Backend storage exhausted” failure mode to
distinguish age-based retention from VictoriaTraces disk-retention limits,
referencing vtstorage.retentionDiskSpaceUsage or the equivalent retention flags.
Document that PVC exhaustion can block ingest before old traces are evicted, and
add capacity alerts plus a test covering PVC exhaustion.
- Around line 112-120: Update the Stage A design around the ExternalName
redirect to define how OTLP ports 4317/4318 and the OTLP/HTTP path are served by
vtinsert, whose defaults are port 10481 and /insert/opentelemetry/v1/traces.
Specify an explicit Service/listener port mapping or path-aware proxy, rather
than relying on ExternalName DNS aliasing alone, while preserving the existing
monitoring-enabled gating and tenant routing.
- Around line 114-120: Update the Stage B collector deployment design to
preserve trace affinity for tail sampling: run the sampling tier with a single
replica, or introduce trace-ID-aware routing/load balancing before multiple
Collector replicas. Do not rely on a regular Kubernetes Service alone to
distribute traffic to the Collector gateway.
- Around line 112-118: Update the distributed-tracing proposal so Stage A
explicitly injects VictoriaTraces AccountID and ProjectID headers using
authenticated workload identity, via the application, trusted proxy, or exporter
rather than relying on the ExternalName redirect. Extend Stage B’s Collector
design to map the authenticated tenant identity to those VictoriaTraces headers
in the OTLP exporter path, in addition to stamping the tenant resource
attribute.
---
Nitpick comments:
In `@design-proposals/distributed-tracing/README.md`:
- Around line 126-128: Update the distributed tracing proposal’s RED/span
metrics design to explicitly specify their source and production path, including
whether they come from a Collector spanmetrics connector or application
instrumentation. Define how exemplars are attached and provide the Grafana
tracesToMetrics queries/configuration that consume the resulting metrics,
covering both the trace-to-metrics and metrics-to-trace links.
- Around line 72-90: Update the VTCluster template documentation and
configuration guidance around the tracingStorages render to state that
vtstorage.replicaCount provides no data replication or HA durability. Document
the limitation and direct HA deployments to replicate through Collectors into
independent VictoriaTraces clusters or use another supported durability design;
do not present the existing replicaCount settings as HA parity.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2261fb92-b1e9-4672-ad8a-0b19c956a39c
📒 Files selected for processing (1)
design-proposals/distributed-tracing/README.md
VerdictLGTM with non-blocking notes This is a Draft design proposal (docs-only, single
The design is conservative, single-vendor, additive/opt-in, and reuses established conventions. Good doc. The notes below are for the design discussion, none block merging a Draft. Findings (non-blocking)[MINOR] Stage A read-side tenant isolation is under-specified. Security section frames per-tenant [MINOR] Jaeger-compatible datasource type is the one unverified external claim. [NIT] Leftover authoring instruction in committed doc. Line 1 is [NIT] Sketch Notes on the prior gemini-code-assist review
Phase 5b (upgrade / fresh-install impact)N/A — the diff touches only |
…y, disk retention - Stage A: document AccountID/ProjectID tenant-header injection and the real vtinsert OTLP contract (port 10481, /insert/opentelemetry/v1/traces); ExternalName aliases DNS only. - Stage B: otlphttp exporter sets tenant headers; tail-sampling trace affinity (single replica or loadbalancingexporter routing_key=traceID). - Backend: configurable replicaCount; retentionDiskSpaceUsage vs age retention; durability caveat (replicaCount != data replication); absent-vs-empty contract. - Grafana: span-metrics need a spanmetrics connector; read-side tenant isolation. - Security/Failure/Testing/Open questions updated accordingly. - Drop leftover authoring comment; add application.group managedMetadata label. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
Thanks for the reviews — IvanHunters and the bots. Pushed Addressed in the doc
Already covered (pre-existing)
Everything requiring live-cluster verification (Jaeger datasource type, exact operator field names like |
Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@design-proposals/distributed-tracing/README.md`:
- Around line 123-135: The Stage A-to-Stage B migration contract uses
incompatible ports and paths that an ExternalName alone cannot translate. Update
the Stage A and Stage B endpoint descriptions and all corresponding references
to choose one stable application-facing OTLP contract, adding explicit
proxy/listener port and path mapping in both stages, or explicitly require
applications to change endpoints during promotion. Ensure the guidance is
consistent across the nearby repeated sections.
- Around line 66-67: Update the retentionDiskSpaceUsage configuration and its
related references to accept only percentage values, since rendering uses
retentionMaxDiskSpaceUsagePercent. Remove byte-value examples and documentation,
and validate or clearly constrain inputs to percent formats consistently at the
additional referenced locations.
- Around line 125-131: Update the Stage B gateway design around the otlphttp
exporter so AccountID/ProjectID are injected per request from authenticated
tenant identity, not configured as static headers. Specify a trusted per-tenant
proxy or a Collector mechanism such as headers_setter using validated request
metadata, and ensure clients cannot spoof these routing headers; retain the
tenant resource attribute for query-time labeling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: afb3914c-e395-4e33-a027-b5da1dd9a7d8
📒 Files selected for processing (1)
design-proposals/distributed-tracing/README.md
…act, headers_setter - retentionDiskUsagePercent is percent-only (maps to retentionMaxDiskSpaceUsagePercent); drop the ambiguous bytes-or-percent field. - Fix the Stage A->B endpoint contradiction: fix the app-facing contract to a port-explicit OTLP/gRPC :4317 Service (bare ExternalName can't carry port/path); gRPC chosen because vtinsert and collector OTLP/HTTP paths differ. - Stage B tenant headers via headers_setter extension (from_context), not static otlphttp.headers which cannot derive per-tenant AccountID/ProjectID. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
|
Pushed
These were all consequences of the first-pass half-fixes; the design is now internally consistent. Remaining live-cluster specifics (exact operator field/flag names, Jaeger datasource type) stay flagged as confirm-before-implementation. |
…ification Blocking fixes from /branch-review, verified against the pinned operator CRD: - VTCluster sub-spec keys are spec.insert/select/storage (prefix dropped), NOT vtinsert/vtselect/vtstorage; rendered workloads/services keep the vt prefix. - Disk cap field is spec.storage.retentionMaxDiskSpaceUsageBytes (bytes); no percent CR field exists. values field is now retentionDiskUsageBytes (bytes); earlier percent-only mapping was inverted. - Topology: monitoring installs into cozy-monitoring; ExternalName redirects live in cozy-monitoring and resolve to *.tenant-root.svc.cluster.local. - Nits: storageSize values key (avoid triple-storage stutter); WorkloadMonitor wording tightened + redis listed; confirm-before-impl caveat lists OTLP wire details + service-name prefix. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
Keep the substantive points; remove the review-attribution asides so the document reads as a standalone design rather than a review-response artifact. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
VerdictLGTM with non-blocking notes A well-researched, technically accurate design proposal for the third observability signal; verification against the actual Findings[MINOR] Checked against [MINOR] Verified against the real [MINOR] Security section ( The new OTLP Collector Deployment and the new per-app sidecar/agent containers (Kafka, RabbitMQ, MariaDB, Redis, Postgres) run inside Claim mismatches[PARTIAL] PR body states Caveats
Recommended follow-ups
|
…ence Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
…x ExternalName wording - Resolve read-side tenant isolation open question: structural isolation (isolated per-tenant stack + same-namespace vtselect datasource + NetworkPolicy, account 0), matching logs/metrics; vtselect has no authz so a datasource header pin is routing, not a security boundary; shared backend needs vmauth (deferred). Reconcile Grafana + Security sections. - Resolve VTCluster-vs-VTSingle: per-entry mode: cluster|single (default cluster). - Fix contradiction on the no-backend failure mode: Service has no backing endpoints, not 'ExternalName resolves to nothing' (design uses a port-explicit Service, not a bare ExternalName). - Soften the vtinsert-* service-name prefix to confirm-before-implementation. - Align write-path AccountID/ProjectID framing with the current account-0 + tenant-label convention. - Add revised-date suffix. Signed-off-by: Alexey Artamonov <aleksei.artamonov@aenix.io>
d83042a to
3cb278c
Compare
2 participants
Summary
Adds a design proposal for distributed tracing in the Cozystack monitoring stack — the missing third observability signal alongside metrics (VictoriaMetrics) and logs (VictoriaLogs).
The proposal centers on VictoriaTraces (its
VTCluster/VTSingleCRDs already ship in the victoria-metrics-operator Cozystack deploys), so no new operator is introduced. It mirrors the existing VictoriaLogs deployment, datasource-provisioning, and values conventions one-for-one.Design highlights
cozy-monitoring, following themonitoring-agentspattern.tracingStoragesvalues list →VTCluster, lifted from theVLClustertemplate; retention configurable, default 14d.tracing.enabledtoggle (off by default), modelled on foundationdb'smonitoring.enabled; native OTLP where the engine supports it (ClickHouse, NATS), sidecar/agent otherwise.Grafana Tempo is documented as the drop-in fallback if VictoriaTraces proves immature.
Rendered:
design-proposals/distributed-tracing/README.mdStatus: Review — open for design review.
Summary by CodeRabbit