Adding OpenChoreo trace module for Moesif#71
Open
ruks wants to merge 1 commit into
Open
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughIntroduces a new observability tracing module for Moesif that enables OpenTelemetry Collector deployment via Helm, with automatic trace enrichment using Kubernetes metadata, environment-based routing, and export to Moesif endpoints. Includes documentation, Helm chart manifests, collector configuration templates, and CI module declaration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
observability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yaml (2)
33-40: Consider adding a default route for unmatched traces.If a trace's
openchoreo.dev/environmentattribute doesn't match any configured environment, it will be silently dropped. Consider adding a fallback pipeline or logging for unrouted traces to aid debugging.♻️ Example: Add default route
connectors: routing: + default_pipelines: [traces/default] table: {{- range .Values.moesif.environments }} - context: resource statement: route() where resource.attributes["openchoreo.dev/environment"] == {{ . | quote }} pipelines: [ traces/{{ . }} ] {{- end }}Then add a default pipeline at the end of the pipelines section:
traces/default: receivers: [ routing ] exporters: [ debug ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@observability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yaml` around lines 33 - 40, The routing table under connectors -> routing in the configMap currently only creates routes for each .Values.moesif.environments entry and will drop traces missing openchoreo.dev/environment; add a fallback entry to the routing.table that matches when no environment matched (e.g., a default statement or an else-like route) that sends to a fallback pipeline (e.g., traces/default) and also add a corresponding traces/default pipeline in the pipelines section that attaches the routing receiver and a debug or logging exporter so unrouted traces are captured for debugging; update the templates that generate the route() statements and the pipelines block (referencing the routing.table entries, the route() expression, resource.attributes["openchoreo.dev/environment"], and pipelines: [ traces/{{ . }} ]) to include this default route and pipeline.
47-47: Redundantorin conditional.The
orfunction requires multiple conditions, but only one is provided here. The same issue occurs on line 97.♻️ Simplify the conditionals
Line 47:
- {{- if or (eq .Values.global.installationMode "singleCluster") }} + {{- if eq .Values.global.installationMode "singleCluster" }}Line 97:
- {{- if or (eq .Values.global.installationMode "singleCluster") }} + {{- if eq .Values.global.installationMode "singleCluster" }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@observability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yaml` at line 47, The conditional uses the Helm `or` function with only one operand; replace occurrences of `{{- if or (eq .Values.global.installationMode "singleCluster") }}` (and the similar occurrence later) with a direct `if` using `eq` so the check becomes `if eq .Values.global.installationMode "singleCluster"`; update both spots where `or` wraps a single `eq` expression to remove the redundant `or` call.observability-tracing-moesif/helm/Chart.yaml (1)
17-21: Align theopentelemetry-collectorchart version with other observability modules.The
observability-tracing-moesifmodule pins version0.140.0, whileobservability-tracing-openobserveandobservability-tracing-opensearchuse0.146.1— a gap of 6 minor versions. Consistent versions across observability modules reduce maintenance overhead and ensure parity in features and bug fixes.♻️ Suggested version alignment
dependencies: - name: opentelemetry-collector repository: https://open-telemetry.github.io/opentelemetry-helm-charts - version: 0.140.0 + version: 0.146.1 condition: opentelemetry-collector.enabled🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@observability-tracing-moesif/helm/Chart.yaml` around lines 17 - 21, The opentelemetry-collector dependency in Chart.yaml is pinned at version 0.140.0 while other observability charts use 0.146.1; update the dependency entry for the opentelemetry-collector (look for the dependencies block and the - name: opentelemetry-collector entry) to use the same version as the other modules (0.146.1) so all observability modules are aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@observability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yaml`:
- Around line 81-89: The batch processor is conditionally defined (batch) when
$.Values.moesif.auth_mode == "api-key" but never added to the traces/in pipeline
processors list ($processors) so it is inactive; update the traces/in pipeline
(traces/in) to include the batch processor in its processors array when
auth_mode == "api-key" (i.e., conditionally append "batch" alongside the other
processor names so the defined batch processor is actually applied to incoming
traces).
In `@observability-tracing-moesif/helm/values.yaml`:
- Around line 48-57: The values.yaml currently sets
opentelemetryCollectorCustomizations.tailSampling.enabled to true while the
README and sample moesif-tracing-values.yaml state the default is false; update
the README (Configuration Parameters table and any sample snippets) to reflect
that tailSampling.enabled defaults to true (or if you prefer to change the code,
flip the values.yaml default to false) — locate references to
opentelemetryCollectorCustomizations, tailSampling, and enabled in the docs and
examples and make them consistent with the chosen default.
- Around line 59-62: The values.yaml is missing a default for
moesif.environments which the configMap.yaml template iterates over; add a
moesif section with a sensible default environments entry (e.g., an empty list
or a single default environment object) so templates that use
.Values.moesif.environments render correctly when users don't provide overrides;
update values.yaml to include the moesif key and an environments: [] (or a
minimal default env object) to prevent empty exporters/routing/pipelines in
configMap.yaml.
---
Nitpick comments:
In `@observability-tracing-moesif/helm/Chart.yaml`:
- Around line 17-21: The opentelemetry-collector dependency in Chart.yaml is
pinned at version 0.140.0 while other observability charts use 0.146.1; update
the dependency entry for the opentelemetry-collector (look for the dependencies
block and the - name: opentelemetry-collector entry) to use the same version as
the other modules (0.146.1) so all observability modules are aligned.
In
`@observability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yaml`:
- Around line 33-40: The routing table under connectors -> routing in the
configMap currently only creates routes for each .Values.moesif.environments
entry and will drop traces missing openchoreo.dev/environment; add a fallback
entry to the routing.table that matches when no environment matched (e.g., a
default statement or an else-like route) that sends to a fallback pipeline
(e.g., traces/default) and also add a corresponding traces/default pipeline in
the pipelines section that attaches the routing receiver and a debug or logging
exporter so unrouted traces are captured for debugging; update the templates
that generate the route() statements and the pipelines block (referencing the
routing.table entries, the route() expression,
resource.attributes["openchoreo.dev/environment"], and pipelines: [ traces/{{ .
}} ]) to include this default route and pipeline.
- Line 47: The conditional uses the Helm `or` function with only one operand;
replace occurrences of `{{- if or (eq .Values.global.installationMode
"singleCluster") }}` (and the similar occurrence later) with a direct `if` using
`eq` so the check becomes `if eq .Values.global.installationMode
"singleCluster"`; update both spots where `or` wraps a single `eq` expression to
remove the redundant `or` call.
🪄 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: e2f54ac1-abd8-4cd4-adce-d5a28c032f0d
⛔ Files ignored due to path filters (1)
observability-tracing-moesif/helm/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
observability-tracing-moesif/README.mdobservability-tracing-moesif/helm/.helmignoreobservability-tracing-moesif/helm/Chart.yamlobservability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yamlobservability-tracing-moesif/helm/templates/opentelemetry-collector/httproute.yamlobservability-tracing-moesif/helm/values.yamlobservability-tracing-moesif/module.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
observability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yaml (1)
49-49: Simplify redundant single-operandorchecks.
if or (eq ... )is equivalent toif eq ...here. Simplifying improves readability.♻️ Proposed cleanup
- {{- if or (eq .Values.global.installationMode "singleCluster") }} + {{- if eq .Values.global.installationMode "singleCluster" }} ... - {{- if or (eq .Values.global.installationMode "singleCluster") }} + {{- if eq .Values.global.installationMode "singleCluster" }}Also applies to: 99-99
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@observability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yaml` at line 49, The Helm template contains redundant conditionals using an unnecessary single-operand or, e.g. the conditional block "if or (eq .Values.global.installationMode \"singleCluster\")" — replace these with the simpler "if eq .Values.global.installationMode \"singleCluster\"" (and apply the same simplification to the other occurrence around the same template noted at lines 99-99) to improve readability; update the conditional expressions in the opentelemetry-collector ConfigMap template accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@observability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yaml`:
- Around line 54-58: The current extractor under extract -> labels uses
key_regex: (.*) and from: pod which copies all pod labels (high-cardinality/leak
risk); change it to only map explicit allowed label keys by replacing the broad
key_regex with either a whitelist regex (e.g., only specific keys) or explicit
label entries and ensure tag_name mappings reference only the approved pod
labels (update the extract.labels block and any tag_name entries to list only
required keys instead of using key_regex: (.*) and from: pod).
In
`@observability-tracing-moesif/helm/templates/opentelemetry-collector/httproute.yaml`:
- Around line 30-31: The HTTPRoute backendRefs.name is hardcoded to
"opentelemetry-collector" which can mismatch this chart’s fullnameOverride
(e.g., "moesif-tracing-collector"); update the template that renders
backendRefs.name (in httproute.yaml) to derive the service name from the chart
values (use the same helper or value used to produce fullnameOverride) so
backendRefs.name and the Service created by the subchart match; apply the same
change to all instances (including the multiClusterReceiver backend entries
around the other occurrence).
---
Nitpick comments:
In
`@observability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yaml`:
- Line 49: The Helm template contains redundant conditionals using an
unnecessary single-operand or, e.g. the conditional block "if or (eq
.Values.global.installationMode \"singleCluster\")" — replace these with the
simpler "if eq .Values.global.installationMode \"singleCluster\"" (and apply the
same simplification to the other occurrence around the same template noted at
lines 99-99) to improve readability; update the conditional expressions in the
opentelemetry-collector ConfigMap template accordingly.
🪄 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: f27a3b78-4434-4e1a-a79c-0f01c4cb585e
⛔ Files ignored due to path filters (1)
observability-tracing-moesif/helm/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
observability-tracing-moesif/README.mdobservability-tracing-moesif/helm/.helmignoreobservability-tracing-moesif/helm/Chart.yamlobservability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yamlobservability-tracing-moesif/helm/templates/opentelemetry-collector/httproute.yamlobservability-tracing-moesif/helm/values.yamlobservability-tracing-moesif/module.yaml
✅ Files skipped from review due to trivial changes (4)
- observability-tracing-moesif/module.yaml
- observability-tracing-moesif/helm/.helmignore
- observability-tracing-moesif/helm/Chart.yaml
- observability-tracing-moesif/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- observability-tracing-moesif/helm/values.yaml
There was a problem hiding this comment.
♻️ Duplicate comments (2)
observability-tracing-moesif/helm/values.yaml (1)
45-58:⚠️ Potential issue | 🟠 MajorAdd a default
moesifblock to keep templates render-safe by default.
configMap.yamlreads.Values.moesif.environments/.Values.moesif.endpoint(for example, Line 24 and Line 26 there), but this file has nomoesifroot key. With default values, this can break rendering or generate empty routing/export sections.🐛 Proposed fix
opentelemetryCollectorCustomizations: debug: enabled: false tailSampling: enabled: true decisionWait: 10s numTraces: 100 expectedNewTracesPerSec: 10 decisionCache: sampledCacheSize: 10000 nonSampledCacheSize: 1000 spansPerSecond: 10 + +moesif: + environments: [] + endpoint: "https://api.moesif.net" + # auth_mode: "application-id" # or "api-key"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@observability-tracing-moesif/helm/values.yaml` around lines 45 - 58, Add a default `moesif` root block to values.yaml so templates that read `.Values.moesif.environments` and `.Values.moesif.endpoint` (e.g., configMap.yaml) don't fail during render; create a minimal safe default with keys like `enabled: false`, `endpoint: ""`, and `environments: []` (and any other keys your templates expect) so `configMap.yaml` and other templates referencing `.Values.moesif.*` can safely render even when Moesif is not configured.observability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yaml (1)
49-53:⚠️ Potential issue | 🟠 MajorAvoid extracting all pod labels into trace attributes.
Line 52 currently captures every pod label, which can create high-cardinality attributes and leak unintended metadata to exporters. Keep extraction to explicit allowed labels (at minimum the routing label).
🔒 Proposed fix
extract: labels: - - tag_name: $$1 - key_regex: (.*) + - tag_name: openchoreo.dev/environment + key: openchoreo.dev/environment from: pod🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@observability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yaml` around lines 49 - 53, The current extractor under extract.labels is using a wildcard key_regex "(.*)" which pulls every pod label into trace attributes; replace this with an explicit allowlist of labels (e.g., use key_regex like "^(routing_label|app|environment)$" or enumerate separate label entries) so only known low-cardinality keys are extracted; update the block referencing extract.labels / tag_name: $$1 / key_regex: (.*) / from: pod to either multiple specific entries or a restrictive regex that includes only the routing label and any other approved keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@observability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yaml`:
- Around line 49-53: The current extractor under extract.labels is using a
wildcard key_regex "(.*)" which pulls every pod label into trace attributes;
replace this with an explicit allowlist of labels (e.g., use key_regex like
"^(routing_label|app|environment)$" or enumerate separate label entries) so only
known low-cardinality keys are extracted; update the block referencing
extract.labels / tag_name: $$1 / key_regex: (.*) / from: pod to either multiple
specific entries or a restrictive regex that includes only the routing label and
any other approved keys.
In `@observability-tracing-moesif/helm/values.yaml`:
- Around line 45-58: Add a default `moesif` root block to values.yaml so
templates that read `.Values.moesif.environments` and `.Values.moesif.endpoint`
(e.g., configMap.yaml) don't fail during render; create a minimal safe default
with keys like `enabled: false`, `endpoint: ""`, and `environments: []` (and any
other keys your templates expect) so `configMap.yaml` and other templates
referencing `.Values.moesif.*` can safely render even when Moesif is not
configured.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 562ce831-c2b5-4f7a-819b-cde1f491b9e1
📒 Files selected for processing (3)
observability-tracing-moesif/README.mdobservability-tracing-moesif/helm/templates/opentelemetry-collector/configMap.yamlobservability-tracing-moesif/helm/values.yaml
✅ Files skipped from review due to trivial changes (1)
- observability-tracing-moesif/README.md
Signed-off-by: ruks <rukshan@wso2.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit