Add OTel two-tier migration + LLM observability demo - #92
Add OTel two-tier migration + LLM observability demo#92Blade-getsuga-tenshou wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesAdds a two-tier OpenTelemetry pipeline with Loki OTLP ingestion, Tempo Zipkin support, Prometheus remote write, and agent-to-gateway forwarding. Adds an instrumented FastAPI LLM demo with Ollama, LiteLLM, Kubernetes deployment, Grafana dashboards, and outage or prompt-injection scenarios. LLM observability and OpenTelemetry migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant llm_demo
participant LiteLLM
participant Ollama
participant otel_gateway
participant Tempo
participant Prometheus
Client->>llm_demo: POST /chat
llm_demo->>LiteLLM: Chat completion request
LiteLLM->>Ollama: local-llama request
Ollama-->>LiteLLM: Model response
LiteLLM-->>llm_demo: Chat completion response
llm_demo-->>Client: ChatResponse
llm_demo->>otel_gateway: OpenTelemetry signals
otel_gateway->>Tempo: Trace export
otel_gateway->>Prometheus: Metric remote write
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 12
🧹 Nitpick comments (3)
app/llm-demo/Dockerfile (1)
1-10: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRun the container as a non-root user.
No
USERdirective is set, so the container runs as root by default. Add a non-root user for the runtime stage; this is a small change for this app since it only needs to serve HTTP and has no need for elevated privileges.🔒️ Proposed fix to add a non-root user
FROM python:3.11-slim WORKDIR /app COPY requirements.txt . RUN pip install --no-cache-dir -r requirements.txt COPY main.py . +RUN useradd --create-home --uid 1000 appuser +USER appuser + EXPOSE 8080 CMD ["uvicorn", "main:app", "--host", "0.0.0.0", "--port", "8080"]🤖 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 `@app/llm-demo/Dockerfile` around lines 1 - 10, Add a dedicated unprivileged runtime user in the Dockerfile before the application starts, ensure /app and its files are accessible to that user, and set the USER directive so the existing uvicorn CMD runs without root privileges.makefile (1)
371-374: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnforce the
migrate-to-otelprerequisite as a real make dependency.The comment states
setup-llm-observability"Assumes migrate-to-otel has already run (needs svc/otel-gateway to exist)", but this is only documented, not enforced. If a user runsmake setup-llm-observabilitydirectly,svc/otel-gatewaydoes not exist, and every OTLP export from LiteLLM, OpenLIT, and the demo app tootel-gateway.monitoring:4318/4317fails silently (best-effort exporters do not fail the deployment). Addmigrate-to-otelas an explicit prerequisite.♻️ Proposed fix to enforce the dependency
-setup-llm-observability: setup-llm-secrets setup-llm-image setup-llm-ollama setup-llm-litellm setup-llm-app setup-llm-dashboard +setup-llm-observability: migrate-to-otel setup-llm-secrets setup-llm-image setup-llm-ollama setup-llm-litellm setup-llm-app setup-llm-dashboard🤖 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 `@makefile` around lines 371 - 374, Update the setup-llm-observability target prerequisites to include migrate-to-otel, ensuring it runs before the existing setup-llm-secrets through setup-llm-dashboard dependencies. Keep the target’s deployment message and existing dependency order otherwise unchanged.app/llm-demo/main.py (1)
54-54: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSet an explicit timeout on the OpenAI client used by the synchronous
/chatendpoint.
OpenAI(base_url=LITELLM_BASE_URL, api_key=LITELLM_API_KEY)leaves the client default timeout active. In openai 1.51.0, that default is 600 seconds. The synchronouschat()endpoint runs on Starlette's worker threadpool, so a stalled LiteLLM/Ollama backend can occupy a worker for the full timeout. Set a shortertimeout=...value appropriate for chat completions, or make the endpoint async and ensure the FastAPI workers have bounded concurrency.⏱️ Proposed fix to bound the upstream call
-client = OpenAI(base_url=LITELLM_BASE_URL, api_key=LITELLM_API_KEY) +client = OpenAI(base_url=LITELLM_BASE_URL, api_key=LITELLM_API_KEY, timeout=30.0)🤖 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 `@app/llm-demo/main.py` at line 54, Set an explicit, appropriately short timeout on the OpenAI client initialization used by the synchronous chat endpoint, updating the client construction around OpenAI and preserving the existing base URL and API key configuration. Ensure stalled upstream chat-completion requests cannot use the library’s 600-second default.
🤖 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 `@app/llm-demo/deployment.yaml`:
- Around line 29-41: Update the LITELLM_API_KEY configuration in the deployment
manifest to use a dedicated scoped LiteLLM virtual key rather than
litellm-secrets/LITELLM_MASTER_KEY. Provision or reference the demo-specific
secret and preserve the existing LITELLM_BASE_URL and other environment
variables.
In `@app/llm-demo/main.py`:
- Around line 116-117: Update the exception handler around the upstream LLM call
to return a generic 502 detail without interpolating the caught exception, and
explicitly chain the HTTPException from the original exception using raise ...
from e. Preserve the existing status code and server-side exception context.
In `@app/llm-demo/requirements.txt`:
- Around line 1-6: Update the FastAPI pin in the requirements dependency list to
a release that permits the required patched Starlette version, while preserving
the existing Starlette security constraint and other dependency pins.
In `@makefile`:
- Around line 290-302: Update setup-otel-tempo to depend on the existing
setup-tempo target instead of repeating its helm upgrade/install command, while
retaining the kubectl apply for zipkin-service-alias.yaml. Use the target
dependency syntax so setup-tempo runs first and preserve the existing alias
setup behavior.
In `@monitoring/chart-values/otel-collector-gateway.yaml`:
- Around line 76-98: Remove or disable the otel-migrated-kube-state-metrics and
otel-migrated-node-exporter scrape jobs until their corresponding
ServiceMonitors are disabled. Do not enable these kubernetes_sd_configs entries
while kube-prometheus-stack still scrapes the same targets, preventing duplicate
remote-written series; alternatively, route the migrated data to a separate
backend.
- Around line 46-48: Set the image tag in the gateway image configuration to the
validated collector version "0.156.0", alongside the existing repository
setting, so Helm does not fall back to a chart-controlled default.
In `@monitoring/chart-values/tempo.yaml`:
- Line 60: Update the Grafana Tempo datasource URL in the Tempo-related
datasource configuration to use port 3200, matching the http_listen_port setting
in tempo.yaml. Replace the stale port 3100 endpoint while preserving the
existing Tempo hostname and URL scheme.
In `@monitoring/dashboards/llm-dashboard.yaml`:
- Around line 157-174: Update the dashboard panel titled “Prompt/Response Log
Stream (for injection detection)” so it no longer presents Loki logs as
prompt-injection detection; preferably replace its Loki datasource and log
expression with a Tempo TraceQL query that inspects the trace attributes used by
the existing scenario, or otherwise rename and describe it explicitly as a
log-only indicator.
In `@scenarios/llm/run.sh`:
- Line 19: Update scenarios/llm/run.sh at lines 19-19 and 75-75 to create the
llm-demo and tempo port-forward log files with mktemp instead of predictable
/tmp paths, store the generated filenames, and remove both temporary files
during the script’s cleanup. Ensure each port-forward redirects to its
corresponding securely created log file.
- Around line 87-102: Update the Tempo trace-checking Python block in run.sh to
poll for the injection trace until a bounded deadline, preserving the existing
trace and message output once found. If the deadline expires without a matching
trace, print the no-match message and exit with a nonzero status so make
run-llm-scenarios fails; retain successful exit behavior when a match appears.
- Around line 12-15: Update the outage scenario setup around the Ollama
scale-down flow to capture the original replica count before changing
deployment/ollama. Extend cleanup() to restore that count when it is available,
while retaining the existing port-forward cleanup; ensure the normal restore
path clears or otherwise prevents duplicate restoration after it completes.
- Around line 24-57: Update run_outage_scenario to exercise a real configured
fallback: target the outage requests at a second model defined in
monitoring/chart-values/litellm.yaml and assert that the responses come from
that fallback. If no fallback model is configured, rename the scenario and its
messaging to describe only provider-outage failure behavior instead of claiming
fallback.
---
Nitpick comments:
In `@app/llm-demo/Dockerfile`:
- Around line 1-10: Add a dedicated unprivileged runtime user in the Dockerfile
before the application starts, ensure /app and its files are accessible to that
user, and set the USER directive so the existing uvicorn CMD runs without root
privileges.
In `@app/llm-demo/main.py`:
- Line 54: Set an explicit, appropriately short timeout on the OpenAI client
initialization used by the synchronous chat endpoint, updating the client
construction around OpenAI and preserving the existing base URL and API key
configuration. Ensure stalled upstream chat-completion requests cannot use the
library’s 600-second default.
In `@makefile`:
- Around line 371-374: Update the setup-llm-observability target prerequisites
to include migrate-to-otel, ensuring it runs before the existing
setup-llm-secrets through setup-llm-dashboard dependencies. Keep the target’s
deployment message and existing dependency order otherwise unchanged.
🪄 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 Plus
Run ID: ab2d35f5-690a-4463-a744-c4d5927b8428
📒 Files selected for processing (20)
.gitignoreLLM_OTEL_README.mdLOKI-DECISION.mdYACE-DECISION.mdapp/llm-demo/Dockerfileapp/llm-demo/deployment.yamlapp/llm-demo/main.pyapp/llm-demo/ollama.yamlapp/llm-demo/requirements.txtapp/llm-demo/secrets.example.yamlmakefilemonitoring/chart-values/litellm.yamlmonitoring/chart-values/loki-otlp.yamlmonitoring/chart-values/loki.yamlmonitoring/chart-values/otel-collector-agent.yamlmonitoring/chart-values/otel-collector-gateway.yamlmonitoring/chart-values/tempo.yamlmonitoring/chart-values/zipkin-service-alias.yamlmonitoring/dashboards/llm-dashboard.yamlscenarios/llm/run.sh
| - name: LITELLM_BASE_URL | ||
| value: "http://litellm-proxy.monitoring:4000/v1" | ||
| - name: LITELLM_API_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: litellm-secrets | ||
| key: LITELLM_MASTER_KEY | ||
| - name: DEFAULT_MODEL | ||
| value: "local-llama" | ||
| - name: OTEL_EXPORTER_OTLP_ENDPOINT | ||
| value: "http://otel-gateway.monitoring:4318" | ||
| - name: OPENLIT_ZERO_CODE | ||
| value: "true" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not use the LiteLLM master key as the demo app's client credential.
LITELLM_API_KEY is sourced from the same secret key (litellm-secrets/LITELLM_MASTER_KEY) that monitoring/chart-values/litellm.yaml configures as the proxy's masterkeySecretKey. The master key is a super-admin credential with full control over the LiteLLM proxy (key management, spend visibility, routing config), not a scoped client key. If the llm-demo pod, its logs, or its environment are exposed, the blast radius extends to full administrative control of the LiteLLM proxy rather than just chat-completion access. Use a scoped virtual key for the demo app instead.
🤖 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 `@app/llm-demo/deployment.yaml` around lines 29 - 41, Update the
LITELLM_API_KEY configuration in the deployment manifest to use a dedicated
scoped LiteLLM virtual key rather than litellm-secrets/LITELLM_MASTER_KEY.
Provision or reference the demo-specific secret and preserve the existing
LITELLM_BASE_URL and other environment variables.
| except Exception as e: | ||
| raise HTTPException(status_code=502, detail=f"upstream LLM call failed: {e}") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Do not return the raw upstream exception text to the API caller; chain the exception.
raise HTTPException(status_code=502, detail=f"upstream LLM call failed: {e}") embeds the raw exception string in the client-facing response, which can leak internal details (upstream URLs, connection diagnostics) about the LiteLLM proxy topology. It also drops the original exception context (Ruff B904), making server-side debugging harder from the raised traceback alone.
🛡️ Proposed fix to sanitize the response and preserve exception chaining
except Exception as e:
- raise HTTPException(status_code=502, detail=f"upstream LLM call failed: {e}")
+ raise HTTPException(status_code=502, detail="upstream LLM call failed") from eAs per static analysis hints, Ruff flags this pattern: "Within an except clause, raise exceptions with raise ... from err or raise ... from None".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except Exception as e: | |
| raise HTTPException(status_code=502, detail=f"upstream LLM call failed: {e}") | |
| except Exception as e: | |
| raise HTTPException(status_code=502, detail="upstream LLM call failed") from e |
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 116-116: Do not catch blind exception: Exception
(BLE001)
[warning] 117-117: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 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 `@app/llm-demo/main.py` around lines 116 - 117, Update the exception handler
around the upstream LLM call to return a generic 502 detail without
interpolating the caught exception, and explicitly chain the HTTPException from
the original exception using raise ... from e. Preserve the existing status code
and server-side exception context.
Source: Linters/SAST tools
| fastapi==0.115.0 | ||
| uvicorn[standard]==0.30.6 | ||
| openai==1.51.0 | ||
| pydantic==2.9.2 | ||
| openlit==1.33.8 | ||
| httpx==0.27.2 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
What is the latest patched version of starlette compatible with fastapi 0.115.0?
💡 Result:
FastAPI 0.115.0 specifies a Starlette dependency constraint of >=0.37.2, <0.39.0 [1]. While later releases of FastAPI expanded this range to include 0.40.0 and beyond to address security vulnerabilities like CVE-2024-47874 [2][3], FastAPI 0.115.0 itself is restricted to the <0.39.0 range [1]. Therefore, the latest patched version compatible with the explicit constraints of FastAPI 0.115.0 is Starlette 0.38.x (the highest available patch within the <0.39.0 range). It is highly recommended to upgrade to a newer version of FastAPI (such as 0.115.3 or later) if you require compatibility with Starlette 0.40.0 or higher to ensure you are protected against known vulnerabilities [2][4][3].
Citations:
- 1: https://pypi.org/project/fastapi/0.115.0/
- 2: https://packagefix.dev/fix/pypi/fastapi/migrate
- 3: ⬆️ Upgrade Starlette to
>=0.37.2,<0.41.0fastapi/fastapi#12431 - 4: ⬆️ Upgrade Starlette to
>=0.40.0,<0.42.0fastapi/fastapi#12469
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "requirements.txt:"
cat -n app/llm-demo/requirements.txt 2>/dev/null || true
echo
echo "repo files mentioning requirements/openai/fastapi/starlette/tqdm:"
rg -n --hidden --glob '!**/.git/**' 'requirements\.txt|fastapi|starlette|tqdm|uvicorn|openai' . | head -200
echo
echo "Python metadata/package availability probe (no repo execution):"
python3 - <<'PY'
import os, urllib.request
url = "https://pypi.org/pypi/fastapi/0.115.0/json"
try:
with urllib.request.urlopen(url, timeout=15) as r:
data = r.read().decode()
except Exception as e:
print("GET_FASTAPI_0_115_0:", e)
else:
import json
info = json.loads(data)
print("fastapi 0.115.0 requires_dist:")
print(info["info"].get("requires_dist"))
PYRepository: infracloudio/sre-stack
Length of output: 1241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Dockerfile context:"
cat -n app/llm-demo/Dockerfile 2>/dev/null || true
echo
echo "main.py FastAPI app context:"
sed -n '1,150p' app/llm-demo/main.py 2>/dev/null || true
echo
echo "Check available PyPI metadata via HTTP proxy/env-aware urllib:"
python3 - <<'PY'
import urllib.request
import json
url = "https://pypi.org/pypi/fastapi/0.115.0/json"
print("REQUEST:", url)
try:
req = urllib.request.Request(url, headers={"User-Agent": "review-verifier"})
with urllib.request.urlopen(req, timeout=20) as r:
data = r.read().decode()
info = json.loads(data)
print("STATUS:", r.status)
print("FastAPI 0.115.0 requires_dist:")
print(info["info"].get("requires_dist"))
except Exception as e:
print("ERROR:", type(e).__name__, e)
PYRepository: infracloudio/sre-stack
Length of output: 5202
Upgrade to a FastAPI version that allows patched Starlette.
fastapi==0.115.0 constrains the Starlette dependency to <0.39.0, so pinning starlette>=0.40.0 here cannot install while keeping the explicit FastAPI version. Move FastAPI to a release that permits a patched Starlette release if the Starlette vulnerability needs fixing.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[HIGH] 1-1: starlette 0.38.6: BadHost: Missing Host header validation poisons request.url.path, bypassing path-based security checks
(PYSEC-2026-161)
[HIGH] 1-1: starlette 0.38.6: Starlette has possible denial-of-service vector when parsing large files in multipart forms
(PYSEC-2026-1941)
[HIGH] 1-1: starlette 0.38.6: Starlette Denial of service (DoS) via multipart/form-data
(PYSEC-2026-1943)
[HIGH] 1-1: starlette 0.38.6: undefined
(PYSEC-2026-2280)
[HIGH] 1-1: starlette 0.38.6: undefined
(PYSEC-2026-2281)
[HIGH] 1-1: starlette 0.38.6: undefined
(PYSEC-2026-248)
[HIGH] 1-1: starlette 0.38.6: undefined
(PYSEC-2026-249)
[HIGH] 1-1: starlette 0.38.6: Starlette has possible denial-of-service vector when parsing large files in multipart forms
[HIGH] 1-1: starlette 0.38.6: Starlette: request.form() limits silently ignored for application/x-www-form-urlencoded enable DoS
[HIGH] 1-1: starlette 0.38.6: Starlette has missing Host header validation that poisons request.url.path, bypassing path-based security checks
[HIGH] 1-1: starlette 0.38.6: Starlette Denial of service (DoS) via multipart/form-data
[HIGH] 1-1: starlette 0.38.6: Starlette: Unvalidated request path concatenated into authority poisons request.url.hostname
[HIGH] 1-1: starlette 0.38.6: Starlette: SSRF and NTLM credential theft via UNC paths in StaticFiles on Windows
[HIGH] 1-1: starlette 0.38.6: Starlette: Arbitrary HTTP method dispatched to HTTPEndpoint attributes via getattr
[HIGH] 1-1: tqdm 4.9.0: undefined
(PYSEC-2017-74)
[HIGH] 1-1: tqdm 4.9.0: tqdm CLI arguments injection attack
(PYSEC-2026-1976)
[HIGH] 1-1: tqdm 4.9.0: tqdm CLI arguments injection attack
[HIGH] 1-1: tqdm 4.9.0: TDQM Arbitrary Code Execution
🤖 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 `@app/llm-demo/requirements.txt` around lines 1 - 6, Update the FastAPI pin in
the requirements dependency list to a release that permits the required patched
Starlette version, while preserving the existing Starlette security constraint
and other dependency pins.
| # Tempo ALREADY EXISTS in this repo (monitoring/chart-values/tempo.yaml) | ||
| # but its receivers don't include zipkin -- apply patches/PATCHES.md's | ||
| # zipkin receiver addition to that file FIRST, then this target installs | ||
| # it (or re-applies, if some other target already did) and adds a | ||
| # "zipkin" Service alias so Istio's already-configured tracing endpoint | ||
| # (zipkin.monitoring:9411, set by setup-istio) has something to actually | ||
| # reach -- zero changes to Istio itself. If a `setup-tempo` target | ||
| # already exists elsewhere in this makefile using the same values file, | ||
| # this is likely redundant with it -- check before assuming this is the | ||
| # only place Tempo gets installed. | ||
| setup-otel-tempo: | ||
| helm upgrade --install tempo grafana/tempo -n $(MONITORING_NS) -f monitoring/chart-values/tempo.yaml --wait --timeout 5m | ||
| kubectl apply -f monitoring/chart-values/zipkin-service-alias.yaml |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Consolidate setup-otel-tempo with the existing setup-tempo target.
setup-tempo (line 90) already installs release "tempo" from chart grafana/tempo using monitoring/chart-values/tempo.yaml. setup-otel-tempo reimplements the identical helm command with the same release name, chart, and values file. Depend on setup-tempo instead of duplicating the install command.
♻️ Proposed refactor to remove the duplicate helm invocation
-setup-otel-tempo:
- helm upgrade --install tempo grafana/tempo -n $(MONITORING_NS) -f monitoring/chart-values/tempo.yaml --wait --timeout 5m
- kubectl apply -f monitoring/chart-values/zipkin-service-alias.yaml
+setup-otel-tempo: setup-tempo
+ kubectl apply -f monitoring/chart-values/zipkin-service-alias.yaml📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Tempo ALREADY EXISTS in this repo (monitoring/chart-values/tempo.yaml) | |
| # but its receivers don't include zipkin -- apply patches/PATCHES.md's | |
| # zipkin receiver addition to that file FIRST, then this target installs | |
| # it (or re-applies, if some other target already did) and adds a | |
| # "zipkin" Service alias so Istio's already-configured tracing endpoint | |
| # (zipkin.monitoring:9411, set by setup-istio) has something to actually | |
| # reach -- zero changes to Istio itself. If a `setup-tempo` target | |
| # already exists elsewhere in this makefile using the same values file, | |
| # this is likely redundant with it -- check before assuming this is the | |
| # only place Tempo gets installed. | |
| setup-otel-tempo: | |
| helm upgrade --install tempo grafana/tempo -n $(MONITORING_NS) -f monitoring/chart-values/tempo.yaml --wait --timeout 5m | |
| kubectl apply -f monitoring/chart-values/zipkin-service-alias.yaml | |
| # Tempo ALREADY EXISTS in this repo (monitoring/chart-values/tempo.yaml) | |
| # but its receivers don't include zipkin -- apply patches/PATCHES.md's | |
| # zipkin receiver addition to that file FIRST, then this target installs | |
| # it (or re-applies, if some other target already did) and adds a | |
| # "zipkin" Service alias so Istio's already-configured tracing endpoint | |
| # (zipkin.monitoring:9411, set by setup-istio) has something to actually | |
| # reach -- zero changes to Istio itself. If a `setup-tempo` target | |
| # already exists elsewhere in this makefile using the same values file, | |
| # this is likely redundant with it -- check before assuming this is the | |
| # only place Tempo gets installed. | |
| setup-otel-tempo: setup-tempo | |
| kubectl apply -f monitoring/chart-values/zipkin-service-alias.yaml |
🤖 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 `@makefile` around lines 290 - 302, Update setup-otel-tempo to depend on the
existing setup-tempo target instead of repeating its helm upgrade/install
command, while retaining the kubectl apply for zipkin-service-alias.yaml. Use
the target dependency syntax so setup-tempo runs first and preserve the existing
alias setup behavior.
| image: | ||
| repository: otel/opentelemetry-collector-contrib | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 'setup-otel-gateway|otel-collector-gateway|image:|tag:' \
makefile monitoring/chart-valuesRepository: infracloudio/sre-stack
Length of output: 13683
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== monitoring/chart-values/otel-collector-gateway.yaml header and image =="
sed -n '1,80p' monitoring/chart-values/otel-collector-gateway.yaml
echo
echo "== monitoring/chart-values/otel-collector-agent.yaml image section =="
sed -n '1,35p' monitoring/chart-values/otel-collector-agent.yaml
echo
echo "== search for validated collector version references =="
rg -n '0\.156\.0|appVersion|collector-gateway|collector\.contrib|otel/opentelemetry-collector-contrib' monitoring/chart-values .github 2>/dev/null || trueRepository: infracloudio/sre-stack
Length of output: 5862
Pin the gateway image tag to the validated collector version.
The header documents 0.156.0, but image.tag is unset, so Helm uses the chart default. Add tag: "0.156.0" here so receiver/processor compatibility does not vary with chart updates.
Proposed fix
image:
repository: otel/opentelemetry-collector-contrib
+ tag: "0.156.0"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| image: | |
| repository: otel/opentelemetry-collector-contrib | |
| image: | |
| repository: otel/opentelemetry-collector-contrib | |
| tag: "0.156.0" |
🤖 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 `@monitoring/chart-values/otel-collector-gateway.yaml` around lines 46 - 48,
Set the image tag in the gateway image configuration to the validated collector
version "0.156.0", alongside the existing repository setting, so Helm does not
fall back to a chart-controlled default.
| "id": 6, | ||
| "title": "Prompt/Response Log Stream (for injection detection)", | ||
| "description": "Label selector depends on your k8sattributes processor config -- verify with `logcli labels` inside the cluster and adjust if needed.", | ||
| "type": "logs", | ||
| "gridPos": { | ||
| "h": 8, | ||
| "w": 24, | ||
| "x": 0, | ||
| "y": 24 | ||
| }, | ||
| "datasource": { | ||
| "type": "loki" | ||
| }, | ||
| "targets": [ | ||
| { | ||
| "expr": "{app=\"litellm-proxy\"} |~ \"(?i)ignore (all )?(previous|prior) instructions|system prompt|jailbreak\"" | ||
| } | ||
| ] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not present this Loki panel as prompt-injection detection.
The current pipeline does not write raw prompt or response content to LiteLLM logs. scenarios/llm/run.sh, Lines 67-73, queries Tempo trace attributes for this reason. This Loki query can produce false negatives for every injection attempt.
Replace this panel with a Tempo TraceQL query, or rename it as a log-only indicator.
🤖 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 `@monitoring/dashboards/llm-dashboard.yaml` around lines 157 - 174, Update the
dashboard panel titled “Prompt/Response Log Stream (for injection detection)” so
it no longer presents Loki logs as prompt-injection detection; preferably
replace its Loki datasource and log expression with a Tempo TraceQL query that
inspects the trace attributes used by the existing scenario, or otherwise rename
and describe it explicitly as a log-only indicator.
| cleanup() { | ||
| [ -n "${APP_PF_PID}" ] && kill "${APP_PF_PID}" 2>/dev/null || true | ||
| [ -n "${TEMPO_PF_PID}" ] && kill "${TEMPO_PF_PID}" 2>/dev/null || true | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore Ollama when the outage scenario exits early.
After Line 39 succeeds, any later command failure triggers set -e. The cleanup handler only stops port-forwards, so the script can leave deployment/ollama at zero replicas.
Record the original replica count before scaling down. Restore that count from cleanup until the normal restore completes.
Proposed fix
APP_PF_PID=""
TEMPO_PF_PID=""
+OLLAMA_REPLICAS=""
cleanup() {
+ if [ -n "${OLLAMA_REPLICAS}" ]; then
+ kubectl -n "${NAMESPACE}" scale deployment/ollama --replicas="${OLLAMA_REPLICAS}" >/dev/null 2>&1 || true
+ fi
[ -n "${APP_PF_PID}" ] && kill "${APP_PF_PID}" 2>/dev/null || true
[ -n "${TEMPO_PF_PID}" ] && kill "${TEMPO_PF_PID}" 2>/dev/null || true
}
@@
- kubectl -n "${NAMESPACE}" scale deployment/ollama --replicas=0
+ OLLAMA_REPLICAS="$(kubectl -n "${NAMESPACE}" get deployment/ollama -o jsonpath='{.spec.replicas}')"
+ kubectl -n "${NAMESPACE}" scale deployment/ollama --replicas=0
@@
- kubectl -n "${NAMESPACE}" scale deployment/ollama --replicas=1
+ kubectl -n "${NAMESPACE}" scale deployment/ollama --replicas="${OLLAMA_REPLICAS}"
kubectl -n "${NAMESPACE}" rollout status deployment/ollama --timeout=120s
+ OLLAMA_REPLICAS=""Also applies to: 38-56
🤖 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 `@scenarios/llm/run.sh` around lines 12 - 15, Update the outage scenario setup
around the Ollama scale-down flow to capture the original replica count before
changing deployment/ollama. Extend cleanup() to restore that count when it is
available, while retaining the existing port-forward cleanup; ensure the normal
restore path clears or otherwise prevents duplicate restoration after it
completes.
| trap cleanup EXIT | ||
|
|
||
| start_app_portforward() { | ||
| kubectl -n "${NAMESPACE}" port-forward svc/llm-demo 18080:8080 >/tmp/llm-demo-pf.log 2>&1 & |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Use secure temporary files for port-forward logs.
Predictable paths under /tmp allow a local user to pre-create a symlink or file before the script runs. Create each log file with mktemp and remove it during cleanup.
scenarios/llm/run.sh#L19-L19: replace/tmp/llm-demo-pf.logwith amktemp-created file.scenarios/llm/run.sh#L75-L75: replace/tmp/tempo-pf.logwith amktemp-created file.
📍 Affects 1 file
scenarios/llm/run.sh#L19-L19(this comment)scenarios/llm/run.sh#L75-L75
🤖 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 `@scenarios/llm/run.sh` at line 19, Update scenarios/llm/run.sh at lines 19-19
and 75-75 to create the llm-demo and tempo port-forward log files with mktemp
instead of predictable /tmp paths, store the generated filenames, and remove
both temporary files during the script’s cleanup. Ensure each port-forward
redirects to its corresponding securely created log file.
Source: Linters/SAST tools
| run_outage_scenario() { | ||
| log "SCENARIO: model provider outage + fallback" | ||
| start_app_portforward | ||
|
|
||
| echo "-- Step 1: baseline traffic against local-llama (healthy) --" | ||
| for i in 1 2 3; do | ||
| curl -s -X POST http://localhost:18080/chat \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"message": "Say hello in one sentence.", "model": "local-llama"}' | ||
| echo | ||
| sleep 1 | ||
| done | ||
|
|
||
| echo | ||
| echo "-- Step 2: inject the outage (scale Ollama to 0 replicas) --" | ||
| kubectl -n "${NAMESPACE}" scale deployment/ollama --replicas=0 | ||
| echo "Ollama scaled to 0. Waiting 15s for it to actually go down..." | ||
| sleep 15 | ||
|
|
||
| echo | ||
| echo "-- Step 3: send traffic during the outage --" | ||
| for i in 1 2 3; do | ||
| curl -s -X POST http://localhost:18080/chat \ | ||
| -H "Content-Type: application/json" \ | ||
| -d '{"message": "Say hello in one sentence.", "model": "local-llama"}' | ||
| echo | ||
| sleep 1 | ||
| done | ||
|
|
||
| echo | ||
| echo "-- Step 4: restore Ollama --" | ||
| kubectl -n "${NAMESPACE}" scale deployment/ollama --replicas=1 | ||
| kubectl -n "${NAMESPACE}" rollout status deployment/ollama --timeout=120s | ||
| echo "Ollama restored." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)litellm\.ya?ml$|run\.sh$|scenario|simple-shuffle|route|proxy|fastapi|pydantic|settings' | head -200
echo
echo "== scenarios/llm/run.sh =="
if [ -f scenarios/llm/run.sh ]; then
nl -ba scenarios/llm/run.sh | sed -n '1,140p'
fi
echo
echo "== search simple-shuffle/fallback/local-llama definitions =="
rg -n "simple-shuffle|fallback|local-llama|ollama|litellm|routes|model" scenarios monitoring chart 2>/dev/null || trueRepository: infracloudio/sre-stack
Length of output: 1411
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== run.sh =="
awk '{printf "%8d %s\n", NR, $0}' scenarios/llm/run.sh | sed -n '1,140p'
echo
echo "== litellm.yaml =="
awk '{printf "%8d %s\n", NR, $0}' monitoring/chart-values/litellm.yaml | sed -n '1,120p'
echo
echo "== exact references =="
rg -n "simple-shuffle|fallback|local-llama|ollama|fallback_route|litellm|routes|provider|model" monitoring/chart-values/litellm.yaml scenarios/llm/run.sh scenarios/llm 2>/dev/null || trueRepository: infracloudio/sre-stack
Length of output: 11660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== chart values model/route references =="
python3 - <<'PY'
from pathlib import Path
p = Path("monitoring/chart-values/litellm.yaml")
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if "routing_strategy" in line or "model_name:" in line or "model:" in line or "api_base:" in line:
print(f"{i}: {line}")
model_entries = []
in_entry = False
for line in text.splitlines():
if "model_name:" in line:
in_entry = True
if in_entry:
model_entries.append(line)
if "routing_strategy:" in line and in_entry:
break
print("== model block ==")
print("\n".join(model_entries))
PY
echo
echo "== scenario references outside script =="
awk '{printf "%s:%d: %s\n", FILENAME, NR, $0}' scenarios/llm monitoring/chart-values/litellm.yaml | rg 'fallback|simple-shuffle|local-llama|outage|provider|ollama|model_list|model:' || trueRepository: infracloudio/sre-stack
Length of output: 1364
Configure an actual fallback before claiming fallback behavior.
monitoring/chart-values/litellm.yaml only defines local-llama backed by Ollama with simple-shuffle routing. Send the outage traffic to a second configured model and assert its response, or rename this as a provider-outage failure scenario.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 45-45: i appears unused. Verify use (or export if used externally).
(SC2034)
🤖 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 `@scenarios/llm/run.sh` around lines 24 - 57, Update run_outage_scenario to
exercise a real configured fallback: target the outage requests at a second
model defined in monitoring/chart-values/litellm.yaml and assert that the
responses come from that fallback. If no fallback model is configured, rename
the scenario and its messaging to describe only provider-outage failure behavior
instead of claiming fallback.
| echo "${RESULT}" | python3 -c " | ||
| import json, sys | ||
| d = json.load(sys.stdin) | ||
| traces = d.get('traces', []) | ||
| if traces: | ||
| print(f'FOUND {len(traces)} matching trace(s):') | ||
| for t in traces: | ||
| print(f\" traceID={t.get('traceID')} duration={t.get('durationMs')}ms\") | ||
| for ss in t.get('spanSet', {}).get('spans', []): | ||
| for attr in ss.get('attributes', []): | ||
| if attr.get('key') == 'gen_ai.input.messages': | ||
| val = attr.get('value', {}).get('stringValue', '') | ||
| print(f' content: {val[:200]}') | ||
| else: | ||
| print('No matching traces found.') | ||
| " |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail the scenario when Tempo does not find the injection trace.
The no-match branch only prints a message. Python exits with status zero, so make run-llm-scenarios can succeed after a false negative. LLM_OTEL_README.md, Lines 59-66, confirms that this timing failure is expected under load.
Poll Tempo until a bounded deadline. Return a nonzero status if no matching trace appears.
Also applies to: 126-128
🤖 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 `@scenarios/llm/run.sh` around lines 87 - 102, Update the Tempo trace-checking
Python block in run.sh to poll for the injection trace until a bounded deadline,
preserving the existing trace and message output once found. If the deadline
expires without a matching trace, print the no-match message and exit with a
nonzero status so make run-llm-scenarios fails; retain successful exit behavior
when a match appears.
2 participants
Summary by CodeRabbit
New Features
Documentation