optional opentelemetry tracing supported by helm#1981
Open
esara wants to merge 1 commit intoHolmesGPT:masterfrom
Open
optional opentelemetry tracing supported by helm#1981esara wants to merge 1 commit intoHolmesGPT:masterfrom
esara wants to merge 1 commit intoHolmesGPT:masterfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
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:
WalkthroughAdds optional OpenTelemetry (OTLP) env-var injection and configuration to the Holmes Helm chart, an experimental AG‑UI server toggle to select the startup command, and documents OTLP usage in the Kubernetes installation guide. No exported/public code entities changed. ChangesOpenTelemetry and AG‑UI Server Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
❌ Deploy Preview for holmes-docs failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/installation/kubernetes-installation.md`:
- Around line 136-167: The docs claim the chart runs under
"opentelemetry-instrument" but the actual Helm template still uses the direct
Python startup command, so auto-instrumentation won't occur; either update the
docs to state that enabling otlp (otlp.enabled / otlp.endpoint) only sets env
vars and that the container must be started with opentelemetry-instrument or
modify the Helm templates to conditionally replace the startup command with
"opentelemetry-instrument" when otlp.enabled is true (ensure the command string
and any args in the deployment/daemonset/container spec are updated), and
mention required image deps if you choose the docs route.
In `@helm/holmes/templates/holmes.yaml`:
- Around line 93-116: The OTEL_PYTHON_FASTAPI_EXCLUDED_URLS env var currently
uses {{ .Values.otlp.excludedUrls | default "/healthz,/readyz" }} which treats
an explicit empty string as unset; change it to explicitly check for the
presence of the value so an empty string is honored: render
OTEL_PYTHON_FASTAPI_EXCLUDED_URLS as the quoted .Values.otlp.excludedUrls when
that key is set (even if empty), otherwise fall back to "/healthz,/readyz" (e.g.
use an if block testing .Values.otlp.excludedUrls), and remove or conditionally
emit OTEL_EXPORTER_OTLP_PROTOCOL and OTEL_EXPORTER_OTLP_INSECURE env vars
(currently rendered from .Values.otlp.protocol and .Values.otlp.insecure) if the
runtime tracer ignores them so you don’t expose unused knobs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d563a19f-6c25-4cda-81e5-55505f81a297
📒 Files selected for processing (4)
Dockerfiledocs/installation/kubernetes-installation.mdhelm/holmes/templates/holmes.yamlhelm/holmes/values.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
helm/holmes/templates/holmes.yaml (1)
114-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
excludedUrls: ""still falls back to the default exclusion list
{{- if .Values.otlp.excludedUrls }}evaluates tofalsefor an empty string, so an explicitexcludedUrls: ""(intended to trace all paths) silently applies/healthz,/readyz. The previous suggestion of using{{- if hasKey .Values.otlp "excludedUrls" }}would correctly distinguish between "key not present" and "key set to empty string."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/holmes/templates/holmes.yaml` around lines 114 - 120, The helm template currently treats an explicit empty string for .Values.otlp.excludedUrls as "unset"; change the conditional to check for the key presence instead of truthiness by using hasKey on .Values.otlp for "excludedUrls" so that OTEL_PYTHON_FASTAPI_EXCLUDED_URLS uses the provided value (even an empty string) when the key exists, and only falls back to "/healthz,/readyz" when the key is absent; update the conditional around OTEL_PYTHON_FASTAPI_EXCLUDED_URLS to use hasKey .Values.otlp "excludedUrls" and keep the value assignment as .Values.otlp.excludedUrls | quote when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/installation/kubernetes-installation.md`:
- Line 158: The docs line claiming 'excludedUrls: ""' will trace all paths is
inaccurate; update the documentation to remove the empty-string claim and
clarify that the Helm template conditional {{- if .Values.otlp.excludedUrls }}
treats an empty string as false so the default "/healthz,/readyz" remains;
specifically, edit the documentation text referencing excludedUrls to state the
correct behavior (that omitting the value or setting it to an empty string will
result in the default /healthz,/readyz) and remove the suggestion that ""
enables tracing of all paths so it matches the actual logic used by the Helm
template.
---
Duplicate comments:
In `@helm/holmes/templates/holmes.yaml`:
- Around line 114-120: The helm template currently treats an explicit empty
string for .Values.otlp.excludedUrls as "unset"; change the conditional to check
for the key presence instead of truthiness by using hasKey on .Values.otlp for
"excludedUrls" so that OTEL_PYTHON_FASTAPI_EXCLUDED_URLS uses the provided value
(even an empty string) when the key exists, and only falls back to
"/healthz,/readyz" when the key is absent; update the conditional around
OTEL_PYTHON_FASTAPI_EXCLUDED_URLS to use hasKey .Values.otlp "excludedUrls" and
keep the value assignment as .Values.otlp.excludedUrls | quote when present.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e875e54-48e7-430f-9888-c5ff183a7eb6
📒 Files selected for processing (4)
Dockerfiledocs/installation/kubernetes-installation.mdhelm/holmes/templates/holmes.yamlhelm/holmes/values.yaml
✅ Files skipped from review due to trivial changes (1)
- helm/holmes/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/installation/kubernetes-installation.md`:
- Around line 147-165: The markdown lint flags are caused by fenced code blocks;
replace the two fenced blocks (the YAML block that begins with "otlp:" and the
bash block that contains "helm upgrade --install holmesgpt robusta/holmes -f
values.yaml") with indented code blocks (indent each code line by four spaces)
so the file uses indented code fencing expected by MD046 and removes the lint
warnings.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac69b309-7c4c-44aa-a563-767833c6c2db
📒 Files selected for processing (3)
docs/installation/kubernetes-installation.mdhelm/holmes/templates/holmes.yamlhelm/holmes/values.yaml
✅ Files skipped from review due to trivial changes (1)
- helm/holmes/values.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- helm/holmes/templates/holmes.yaml
esara
force-pushed
the
otel
branch
2 times, most recently
from
1497116 to
7809f50
Compare
Signed-off-by: Endre Sara <endresara@gmail.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
New Features
Documentation