feat: add Jaeger distributed tracing support #62
Conversation
Add configuration for distributed tracing using Jaeger OTLP: - Configure Traefik with OTLP tracing support - Configure OpenFGA telemetry with trace exports - Configure Heimdall with tracing environment variables - Update Chart.lock with openfga 0.2.44 and project-service 0.4.4 - Add Jaeger installation and usage documentation to README All tracing features are disabled by default and can be enabled via values configuration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Issue: LFXV2-606 Co-Authored-By: Claude <[email protected]> Signed-off-by: Trevor Bramwell <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughBumps Helm chart version and adds OTLP/Jaeger tracing configuration for fga-operator, openfga, and heimdall; replaces the README Documentation section with a Jaeger integration guide; and adds three terms to the spell-check dictionary. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Heimdall
participant OpenFGA
participant fga_operator as "fga-operator"
participant OTLP as "OTLP Exporter"
participant Jaeger as "Jaeger Collector"
participant UI as "Jaeger UI"
User->>Heimdall: HTTP request
activate Heimdall
note right of Heimdall #dfefff: Trace created/propagated\n(OTEL_PROPAGATORS)
Heimdall->>OpenFGA: Downstream call (context propagated)
activate OpenFGA
OpenFGA-->>Heimdall: Response
deactivate OpenFGA
Heimdall-->>User: Response
deactivate Heimdall
par Export traces (async)
Heimdall->>OTLP: Send spans (gRPC/OTLP to jaeger-collector.observability:4317)
OpenFGA->>OTLP: Send spans (gRPC/OTLP)
fga_operator->>OTLP: Send spans (gRPC/OTLP)
end
OTLP->>Jaeger: OTLP traces (insecure endpoint)
Jaeger-->>UI: Indexed traces
User->>UI: View traces
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR adds Jaeger distributed tracing support to the LFX Platform Helm chart. The changes enable distributed tracing capabilities across the platform by integrating Jaeger with Traefik, OpenFGA, and Heimdall components.
Key changes include:
- Added comprehensive Jaeger documentation with installation and configuration instructions
- Configured OTLP tracing settings for Traefik, OpenFGA, and Heimdall in values.yaml
- Incremented chart version from 0.2.19 to 0.2.20
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Chart.yaml | Bumped chart version to reflect new tracing features |
| values.yaml | Added tracing configuration blocks for Traefik, OpenFGA, and Heimdall with OTLP settings |
| README.md | Added comprehensive Jaeger section with prerequisites, installation, configuration, and access instructions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
charts/lfx-platform/README.md (1)
173-187: Document production storage limitations.The installation uses
--set storage.type=memory, which is appropriate for development/testing but will lose all traces on restart. Consider adding a note that production deployments should use persistent storage (e.g., Elasticsearch, Cassandra).Add a note after line 187:
> **Note:** This configuration uses in-memory storage suitable for development only. > For production deployments, configure persistent storage using Elasticsearch or Cassandra.charts/lfx-platform/values.yaml (1)
134-142: Consider documenting sample ratio impact.The
sampleRatio: 1.0(line 142) means 100% of traces will be captured, which is appropriate for development but may generate excessive data in production. Consider adding a comment to help users understand this setting.Add a comment:
telemetry: trace: enabled: false otlp: endpoint: "jaeger-collector.observability:4317" tls: enabled: false + # sampleRatio: 1.0 means 100% sampling (suitable for dev/testing) + # Consider reducing (e.g., 0.1 for 10%) in production environments sampleRatio: 1.0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
charts/lfx-platform/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
charts/lfx-platform/Chart.yaml(1 hunks)charts/lfx-platform/README.md(1 hunks)charts/lfx-platform/values.yaml(3 hunks)
🔇 Additional comments (6)
charts/lfx-platform/Chart.yaml (1)
8-8: LGTM!The chart version bump from 0.2.19 to 0.2.20 is appropriate for adding Jaeger tracing support as a new feature.
charts/lfx-platform/README.md (4)
159-163: LGTM!The Jaeger section introduction clearly describes its purpose and deployment model (separate observability namespace).
164-171: LGTM!The prerequisite steps correctly add and update the Jaeger Helm repository.
189-219: LGTM!The Helm values examples correctly demonstrate how to enable tracing for each component and match the structure in values.yaml.
235-243: LGTM!The Jaeger UI access instructions are clear and use the correct port-forward syntax.
charts/lfx-platform/values.yaml (1)
64-71: Verify OTLP endpoint matches Jaeger collector service.Ensure the configured endpoint (
jaeger-collector.observability:4317) aligns with your Jaeger collector’s actual service name and namespace (README recommends installing Jaeger into namespaceobservability).
Fixes the misspelling of propagator(s) and the typo on the path to the lfx-platfrom chart. Issue: LFXV2-606 Signed-off-by: Trevor Bramwell <[email protected]>
Signed-off-by: Trevor Bramwell <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
charts/lfx-platform/README.md (1)
226-227: Align upgrade command paths with install instructions.Both upgrade snippets still reference
charts/lfx-platform, while the install example above uses./charts/lfx-platform. Please add the leading./so users can copy/paste consistently.Also applies to: 232-233
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
charts/lfx-platform/README.md(1 hunks)charts/lfx-platform/values.yaml(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-01T17:10:08.245Z
Learnt from: bramwelt
PR: linuxfoundation/lfx-v2-helm#62
File: charts/lfx-platform/values.yaml:64-73
Timestamp: 2025-10-01T17:10:08.245Z
Learning: When Jaeger is deployed with `allInOne.enabled=true`, it creates a `jaeger-collector` service endpoint (e.g., `jaeger-collector.observability:4317`) in the deployment namespace, even when `collector.enabled=false`. The All-In-One mode bundles the collector functionality within the single deployment.
Applied to files:
charts/lfx-platform/values.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: MegaLinter
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.cspell.json (1)
36-36: “tracecontext” is correct; consider related W3C tokens if present.If README/values reference “traceparent”/“tracestate” or “b3”, you may want to whitelist those too to avoid CI noise. Only add them if flagged by the verification above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.cspell.json(2 hunks)
🔇 Additional comments (1)
.cspell.json (1)
16-18: Whitelist remaining Helm template and config tokens in .cspell.json
cspell flagged the following unknown words in your markdown and YAML:
• nindent
• autheliapem
• displayname
• groupsio
• uninviteAdd these to your
.cspell.jsonif they’re expected terms, then rerun cspell to confirm no new flags.
Signed-off-by: Trevor Bramwell <[email protected]>
1 participant
This pull request introduces distributed tracing support to the LFX Platform Helm chart by adding Jaeger integration. The changes include documentation on installing and configuring Jaeger, as well as updates to Helm values for enabling tracing in Traefik, OpenFGA, and Heimdall. The chart version is also incremented.
Jaeger integration and distributed tracing support:
README.md, documenting prerequisites, installation, configuration, and access instructions for Jaeger distributed tracing. It also explains how to enable tracing for Traefik, OpenFGA, and Heimdall.Tracing configuration in Helm values:
values.yamlto add tracing configuration blocks fortraefik, including OTLP settings, environment variables for propagators, and Jaeger endpoint configuration.openfgasection invalues.yamlto include tracing/telemetry configuration, such as enabling tracing, OTLP endpoint, TLS settings, and sampling ratio.heimdallenvironment variables invalues.yamlto support tracing, including enabling tracing, specifying Jaeger as the propagator, and configuring OTLP exporter settings.Chart version update:
Chart.yamlfrom0.2.21to0.2.22to reflect these new features.🤖 Generated with Claude Code
Issue: LFXV2-606