Otel migration#90
Open
Blade-getsuga-tenshou wants to merge 3 commits into
Open
Conversation
- demo-1-before.sh: full stack deploy (Robot Shop + HotROD + monitoring) - demo-2-migrate-otel.sh: 8-step live OTel migration - otel-agent-values.yaml: DaemonSet — filelog, hostmetrics, kubeletstats - otel-gateway-values.yaml: Gateway — OTLP fan-out to Tempo/Prometheus/Loki 3.x - otel-gateway-prom-scrape-values.yaml: Gateway + prometheusreceiver - loki-v3-values.yaml: Loki 3.x with OTLP ingest + structured metadata - robot-shop-local-values.yaml: 4-node k3d cluster overrides - application.yaml: dashboard log queries updated for OTel stream labels - OTEL_MIGRATION.md: full scope documentation
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughAdds a complete OpenTelemetry Collector migration lab to ChangesOTel Collector Migration Lab
Sequence Diagram(s)sequenceDiagram
participant App as Application / HotROD / Robot Shop
participant Agent as otel-agent DaemonSet
participant Gateway as otel-gateway Deployment
participant Tempo as Tempo
participant Loki as Loki 3.x OTLP
participant Prom as Prometheus Remote Write
App->>Agent: OTLP traces/metrics/logs
Note over Agent: filelog (pod logs), hostmetrics, kubeletstats
Agent->>Agent: k8sattributes, memory_limiter, batch
Agent->>Gateway: OTLP gRPC 4317
Gateway->>Gateway: k8sattributes, resource/loki-labels, batch
Gateway->>Tempo: OTLP gRPC traces
Gateway->>Loki: OTLP HTTP logs
Gateway->>Prom: prometheusremotewrite metrics
Note over Gateway: k8sobjects → Loki (K8s events)
Note over Gateway: k8s_cluster → Prom (cluster metrics)
Note over Gateway: prometheus receiver → Prom (scraped targets)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![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
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
monitoring/dashboards/application.yaml (1)
1110-1191: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the Loki panels bound to the dashboard
$namespacevariable.These queries now hard-code
robot-shop, but the dashboard already exposes a multi-select$namespacevariable. After this change, the Prometheus panels follow the selected namespace while all of these log panels stay pinned torobot-shop. Usek8s_namespace_name=~"${namespace:regex}"or equivalent instead.Also applies to: 1231-1231, 1269-1269, 1405-1405, 1442-1442, 1575-1575, 1612-1612, 1649-1649, 1686-1686
🤖 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/application.yaml` around lines 1110 - 1191, The Loki log queries are still hard-coded to the robot-shop namespace, so they don’t follow the dashboard’s selected namespace. Update each affected query in the application dashboard panels to use the existing $namespace variable (for example via k8s_namespace_name=~"${namespace:regex}" or equivalent) instead of the fixed robot-shop value. Apply this consistently across the log panel expressions that match the listed refIds/panels so they stay aligned with the namespace filter used elsewhere in the dashboard.
🟡 Minor comments (11)
demo-1-before.sh-141-142 (1)
141-142: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the hard-coded MySQL seeder job name.
mysql-seeedlooks misspelled and does not line up with themysqlseedercomponent naming inapp/robot-shop/robot-shop-local-values.yaml, so this wait is likely to burn the full timeout on every run even when seeding succeeded.🤖 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 `@demo-1-before.sh` around lines 141 - 142, The job wait in the MySQL seeder step is using a hard-coded, misspelled job name that does not match the actual seeder component name. Update the wait logic in the script around wait_for_job to use the correct mysqlseeder job identifier from the robot-shop values so the check targets the real Kubernetes job instead of timing out unnecessarily.OTel/02-lgtm-stack/install.sh-33-35 (1)
33-35: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the printed next-step path.
After changing into
OTel/02-lgtm-stack,./03-otel-collector/install.shis not reachable from the current directory. The printed command should use../03-otel-collector/install.shor a repo-root path.🤖 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 `@OTel/02-lgtm-stack/install.sh` around lines 33 - 35, The printed next-step command in the install script points to an unreachable relative path after changing into OTel/02-lgtm-stack; update the final echo in the script to reference the correct location for 03-otel-collector/install.sh, using either ../03-otel-collector/install.sh or an equivalent repo-root path so the next step is runnable from the current directory.OTel/00-prereqs/install-macos.sh-30-31 (1)
30-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the printed next-step path.
After changing into
OTel/00-prereqs,./01-cluster/up.shis the wrong relative path. This should point to../01-cluster/up.shor a repo-root path.🤖 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 `@OTel/00-prereqs/install-macos.sh` around lines 30 - 31, The next-step message in the macOS prereqs script prints an incorrect relative path after changing into OTel/00-prereqs. Update the echo in install-macos.sh so it points to the real cluster startup script via ../01-cluster/up.sh (or an equivalent repo-root path), keeping the existing output text otherwise unchanged.OTel/01-cluster/up.sh-17-18 (1)
17-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the printed next-step path.
After Line 3 changes into
OTel/01-cluster,./02-lgtm-stack/install.shpoints to a non-existent sibling under that directory. The guidance should use../02-lgtm-stack/install.sh(or print a repo-root path) so copy/paste works.🤖 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 `@OTel/01-cluster/up.sh` around lines 17 - 18, The next-step message in up.sh points to the wrong relative path after the script changes into OTel/01-cluster, so copy/paste will fail. Update the echoed guidance in the same echo block so it references ../02-lgtm-stack/install.sh (or an equivalent repo-root path) instead of the current sibling path.OTel/02-lgtm-stack/install.sh-15-31 (1)
15-31: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPin the Helm chart versions in
OTel/02-lgtm-stack/install.sh.
These releases float to whatever Helm serves that day, but the lab values already depend on chart-specific behavior (for example, Loki 7.x defaults). Pinning the versions keeps the quick-start reproducible and avoids schema/default drift breaking later steps.🤖 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 `@OTel/02-lgtm-stack/install.sh` around lines 15 - 31, The Helm releases in install.sh are unpinned, so the charts can change behavior unexpectedly between runs. Update the helm upgrade --install commands for kps, loki, and tempo to specify fixed chart versions, using the existing install flow and matching the chart-specific assumptions in the lab values. Keep the changes localized to the three Helm install blocks so the quick-start remains reproducible.OTel/02-lgtm-stack/prometheus-values.yaml-48-57 (1)
48-57: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive Loki an explicit
uid.
tracesToLogsV2.datasourceUidresolves by UID, and Grafana generates one automatically when it’s omitted here. Setuid: lokiso the Tempo → Loki link andLogs for this spanflow work reliably.Suggested fix
- name: Loki type: loki + uid: loki url: http://loki.observability.svc:3100 access: proxy🤖 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 `@OTel/02-lgtm-stack/prometheus-values.yaml` around lines 48 - 57, The Loki datasource block is missing an explicit UID, so Grafana may generate one automatically and break the Tempo-to-Loki link. Update the datasource definition for Loki in the Prometheus/Grafana values to include a stable uid of loki, and keep it aligned with any tracesToLogsV2.datasourceUid references so the Logs for this span flow resolves reliably.OTel/03-otel-collector/install.sh-47-48 (1)
47-48: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the printed next-step path.
Because Line 3 changes into
OTel/03-otel-collector/,./04-logs-migration/apply.shpoints to a nonexistent child directory. The next step is a sibling folder, so this should be../04-logs-migration/apply.sh.Suggested fix
-echo "Next: ./04-logs-migration/apply.sh" +echo "Next: ../04-logs-migration/apply.sh"🤖 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 `@OTel/03-otel-collector/install.sh` around lines 47 - 48, The printed next-step path in the install flow is incorrect because the script changes into OTel/03-otel-collector before echoing it, so the current relative path points to a non-existent child directory. Update the next-step message in install.sh to reference the sibling migration step using the correct relative path. Keep the change limited to the echoed follow-up instruction near the Tempo/Grafana output so users can run the next script directly.OTel/docs/demo-script.md-3-14 (1)
3-14: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winState the expected working directory before the commands.
This file uses relative paths like
06-validation/demo.yamland07-prometheus-scrape-migration/, but unlikeOTel/README.mdit never says to run them fromOTel/. Copy-pasting from repo root orOTel/docs/will fail.Suggested fix
A 20–25 minute walkthrough. Talking points on the left, commands to run on the right. Keep Grafana (http://localhost:3000) open in one tab and a Terminal window visible. + +Run the commands below from the `OTel/` directory. Before starting:🤖 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 `@OTel/docs/demo-script.md` around lines 3 - 14, The demo script uses relative paths like 06-validation/demo.yaml and 07-prometheus-scrape-migration/, so add an explicit note near the start of the walkthrough that the commands should be run from the OTel/ working directory. Update the intro text in demo-script.md to mirror the guidance used in OTel/README.md, making it clear before the kubectl commands where the terminal should be positioned.OTel/08-yace-decision/README.md-19-21 (1)
19-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon’t imply Kubernetes enrichment for CloudWatch metrics.
k8sattributesonly adds metadata when telemetry can be associated with a Kubernetes resource, andawscloudwatchreceiveroutput usually doesn’t have that context. Update the README and theapply.shmessage so they don’t suggest AWS metrics will be enriched the same way as pod-native telemetry.🤖 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 `@OTel/08-yace-decision/README.md` around lines 19 - 21, The README text and the apply.sh status message are implying that AWS CloudWatch metrics will get the same Kubernetes enrichment as pod-native telemetry, which is misleading. Update the wording around the decision guidance and the apply.sh output to clarify that k8sattributes only enriches telemetry when Kubernetes resource context is available, and that awscloudwatchreceiver output usually won’t have that context. Use the existing README decision copy and the apply.sh message so the guidance no longer suggests AWS metrics will be enriched the same way as Kubernetes-native signals.OTel/05-metrics-migration/apply.sh-33-33 (1)
33-33: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the next-step path.
After Line 3 changes into
OTel/05-metrics-migration,./06-validation/deploy-demo.shresolves to a nonexistent child directory. This should point to the sibling step, e.g.../06-validation/deploy-demo.sh.🤖 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 `@OTel/05-metrics-migration/apply.sh` at line 33, The next-step path in apply.sh is incorrect because after changing into OTel/05-metrics-migration, ./06-validation/deploy-demo.sh points to a nonexistent child directory. Update the step reference in the apply.sh script to use the sibling path to the validation step, and verify the transition path after the directory change resolves correctly to deploy-demo.sh.OTel/04-logs-migration/apply.sh-34-34 (1)
34-34: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPrint a next-step path that works after the script exits.
This script's
cdonly affects the subprocess, so./05-metrics-migration/apply.shis usually wrong from the caller's shell. Print an explicit repo-root path or../05-metrics-migration/apply.shinstead.🤖 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 `@OTel/04-logs-migration/apply.sh` at line 34, The next-step message in apply.sh is using a path that only works from inside the script’s subprocess, so update the printed instruction to reference a path that is valid from the caller’s shell. Adjust the next-step output near the existing “Next:” message to use an explicit repo-root-relative path or the sibling script path format (for example via the apply.sh handoff text) so users can run 05-metrics-migration/apply.sh after this script exits.
🧹 Nitpick comments (3)
OTEL_MIGRATION.md (1)
25-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language tags to the fenced blocks.
These fences are already tripping MD040. Adding a language (
text,bash, etc.) will clear the docs lint warnings without changing the content.Also applies to: 34-51, 138-140, 146-160, 225-230
🤖 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 `@OTEL_MIGRATION.md` around lines 25 - 30, Add language tags to the fenced code blocks in OTEL_MIGRATION.md to satisfy MD040: update each affected fenced block around the diagram sections and similar examples to use an explicit language like text or bash, without changing the displayed content. Locate the markdown fences in the migration doc and apply the same fix consistently across all referenced blocks so the docs lint warning is cleared.Source: Linters/SAST tools
monitoring/chart-values/otel-gateway-prom-scrape-values.yaml (1)
166-187: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDisable
debugin this default logs pipeline too.With
otlphttp/lokialready enabled,debugturns the gateway into a second log sink by echoing every record to stdout. That is useful for troubleshooting, but expensive and noisy as the default step-07 configuration.🤖 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-gateway-prom-scrape-values.yaml` around lines 166 - 187, The default logs pipeline still includes the debug exporter, which makes the gateway echo every log record to stdout in addition to sending it to Loki. Update the logs pipeline in the otel gateway values so the service pipeline for logs uses only the intended exporters, removing debug from the default configuration while keeping the otlphttp/loki path intact.monitoring/chart-values/otel-gateway-values.yaml (1)
122-148: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDisable
debugin the default logs pipeline.This gateway already exports logs to Loki; leaving
debugenabled prints every log record to collector stdout as well, so gateway log volume scales with application traffic. Prefer a separate troubleshooting overlay when you needdebug.🤖 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-gateway-values.yaml` around lines 122 - 148, Remove the debug exporter from the default logs pipeline in the otel-gateway values so the gateway does not duplicate every log record to stdout; keep the existing Loki export path in the service.pipelines.logs configuration and, if needed, add debug only in a separate troubleshooting overlay rather than in the base chart values.
🤖 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 `@demo-1-before.sh`:
- Around line 11-12: The setup script does not fail fast because `set -uo
pipefail` still allows errors from `cd` and later setup steps to continue, and
the MySQL wait target is misspelled. Update the script near the initial shell
setup and the MySQL readiness check so it uses fail-fast behavior with `set -euo
pipefail`, ensures the `cd` failure stops execution, and corrects the
`mysql-seeed` identifier to the actual `mysql-seeder` name used by the manifest.
In `@demo-2-migrate-otel.sh`:
- Around line 172-177: The HotROD step is waiting for any running pod instead of
the deployment rollout, so the env change may not be applied before traffic
resumes. Update the rollout synchronization after the kubectl set env call to
wait on the hotrod Deployment/ReplicaSet becoming ready rather than using
wait_for_pod "hotrod" "hotrod", and keep the check tied to the HotROD deployment
and its startup env handling in the deployment manifest.
- Around line 18-19: Add fail-fast handling at the top of the migration script
by including set -e alongside the existing shell options and making the cd guard
stop execution if the directory change fails, so later Helm/Kubernetes steps do
not continue in a bad state. Then, in the HotROD update flow after kubectl set
env, replace the current Running-only readiness check with a kubectl rollout
status wait for deployment/hotrod in the hotrod namespace, so the script does
not proceed until the new rollout is actually serving.
In `@OTel/00-prereqs/install-macos.sh`:
- Around line 11-18: The Docker readiness check in install-macos.sh only
verifies that the docker CLI exists, but it should also verify the daemon is
running before continuing. Update the install flow around the existing docker
command check to also run a quick daemon probe such as docker info or docker ps,
and if it fails, print a clear message and stop before kind setup proceeds. Keep
the logic localized to the Docker Desktop install/check block so the script only
continues when both the CLI and daemon are available.
In `@OTel/02-lgtm-stack/loki-values.yaml`:
- Around line 32-37: The Loki PVC in the values config is being enabled without
ensuring a backing StorageClass exists, so the claim can remain Pending and
block startup. Update the setup flow referenced by the cluster bootstrap and
install scripts so a default StorageClass/provisioner is created before Helm
applies the Loki release, and keep the PVC settings in loki-values aligned with
that provisioner. Use the existing Loki persistence block and the
cluster/install entrypoints as the main places to add the StorageClass setup and
readiness check.
In `@OTel/03-otel-collector/install.sh`:
- Around line 7-20: The Helm install flow in install.sh is currently unpinned
and can drift with upstream defaults, so update the helm upgrade --install
invocations for both otel-gateway and otel-agent to use an explicit chart
version and make sure the referenced values files pin the collector image tags
via image.repository/image.tag settings. Also fix the follow-up echo path so it
points to the 04-logs-migration script from the parent directory, and keep the
change localized to the install.sh steps that reference helm and the final Next
message.
In `@OTel/04-logs-migration/gateway-values.yaml`:
- Around line 84-87: The step-04 metrics pipeline in the gateway values is still
exporting OTLP metrics to Tempo, which should not receive metrics. Update the
gateway configuration so the metrics overlay stays on debug or remove the
metrics pipeline entirely until step 05 switches it to Prometheus remote_write;
use the metrics pipeline block in gateway-values.yaml to locate and adjust the
receiver/processor/exporter setup.
In `@OTel/05-metrics-migration/gateway-values.yaml`:
- Around line 81-83: The Prometheus relabeling config in gateway-values.yaml is
too broad because resource_to_telemetry_conversion.enabled under the
gateway/collector settings turns all resource attributes into labels. Update
this block so only the specific useful attributes are promoted, and avoid
enabling blanket conversion in the gateway pipeline; keep the change localized
around the resource_to_telemetry_conversion section and any pod-label extraction
settings used by the collector tiers.
In `@OTel/06-validation/demo.yaml`:
- Around line 1-3: The demo is bypassing the agent and the trace/log correlation
is not actually supported as written. Update the workloads in demo.yaml so the
Jobs and logger route OTLP through the agent’s local endpoint instead of
otel-gateway.observability.svc:4317, and make the log producer emit logs
correlated with the same spans from demo-traces rather than synthetic trace_id
values. Also align the file header/comments with the actual data path so the
06-validation flow accurately describes agent-based trace→logs validation.
In `@OTel/06-validation/deploy-demo.sh`:
- Around line 5-6: The deployment script currently treats the demo as ready
after only waiting on `demo-logger`, and the `|| true` masks failures, so the
success banner can appear too early. Update `deploy-demo.sh` to wait for all
demo emitters to become available, including `demo-logger`, `demo-traces`, and
`demo-metrics`, and remove the failure suppression so rollout problems are
surfaced. Use the existing `kubectl wait` flow in the script as the place to add
the additional readiness checks before printing any “Demo deployed” success
message.
In `@OTel/08-yace-decision/option-b-cloudwatch-receiver/apply.sh`:
- Around line 6-9: The current guard in apply.sh only checks that the otel-aws
secret exists, but it should also verify the required keys are present before
proceeding. Update the kubectl validation block to inspect the otel-aws secret
for AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and AWS_REGION, and fail fast with
a clear error if any are missing. Use the existing apply.sh secret check as the
entry point so the script stops before the awscloudwatchreceiver is deployed
with an incomplete configuration.
In
`@OTel/08-yace-decision/option-b-cloudwatch-receiver/gateway-overlay-values.yaml`:
- Around line 1-2: The metrics pipeline overlay is replacing the step 07
receiver list and unintentionally dropping prometheus from
`config.service.pipelines.metrics.receivers`, which breaks scrape ingestion.
Update `gateway-overlay-values.yaml` so the step 07 path preserves the existing
`prometheus` receiver when adding `awscloudwatch` (or split the step 05 and step
07 overlays so the cloudwatch receiver is layered separately). Make the change
in the overlay that feeds `gateway-values.yaml` and keep the receiver list
additive rather than overwriting the step 07 configuration.
---
Outside diff comments:
In `@monitoring/dashboards/application.yaml`:
- Around line 1110-1191: The Loki log queries are still hard-coded to the
robot-shop namespace, so they don’t follow the dashboard’s selected namespace.
Update each affected query in the application dashboard panels to use the
existing $namespace variable (for example via
k8s_namespace_name=~"${namespace:regex}" or equivalent) instead of the fixed
robot-shop value. Apply this consistently across the log panel expressions that
match the listed refIds/panels so they stay aligned with the namespace filter
used elsewhere in the dashboard.
---
Minor comments:
In `@demo-1-before.sh`:
- Around line 141-142: The job wait in the MySQL seeder step is using a
hard-coded, misspelled job name that does not match the actual seeder component
name. Update the wait logic in the script around wait_for_job to use the correct
mysqlseeder job identifier from the robot-shop values so the check targets the
real Kubernetes job instead of timing out unnecessarily.
In `@OTel/00-prereqs/install-macos.sh`:
- Around line 30-31: The next-step message in the macOS prereqs script prints an
incorrect relative path after changing into OTel/00-prereqs. Update the echo in
install-macos.sh so it points to the real cluster startup script via
../01-cluster/up.sh (or an equivalent repo-root path), keeping the existing
output text otherwise unchanged.
In `@OTel/01-cluster/up.sh`:
- Around line 17-18: The next-step message in up.sh points to the wrong relative
path after the script changes into OTel/01-cluster, so copy/paste will fail.
Update the echoed guidance in the same echo block so it references
../02-lgtm-stack/install.sh (or an equivalent repo-root path) instead of the
current sibling path.
In `@OTel/02-lgtm-stack/install.sh`:
- Around line 33-35: The printed next-step command in the install script points
to an unreachable relative path after changing into OTel/02-lgtm-stack; update
the final echo in the script to reference the correct location for
03-otel-collector/install.sh, using either ../03-otel-collector/install.sh or an
equivalent repo-root path so the next step is runnable from the current
directory.
- Around line 15-31: The Helm releases in install.sh are unpinned, so the charts
can change behavior unexpectedly between runs. Update the helm upgrade --install
commands for kps, loki, and tempo to specify fixed chart versions, using the
existing install flow and matching the chart-specific assumptions in the lab
values. Keep the changes localized to the three Helm install blocks so the
quick-start remains reproducible.
In `@OTel/02-lgtm-stack/prometheus-values.yaml`:
- Around line 48-57: The Loki datasource block is missing an explicit UID, so
Grafana may generate one automatically and break the Tempo-to-Loki link. Update
the datasource definition for Loki in the Prometheus/Grafana values to include a
stable uid of loki, and keep it aligned with any tracesToLogsV2.datasourceUid
references so the Logs for this span flow resolves reliably.
In `@OTel/03-otel-collector/install.sh`:
- Around line 47-48: The printed next-step path in the install flow is incorrect
because the script changes into OTel/03-otel-collector before echoing it, so the
current relative path points to a non-existent child directory. Update the
next-step message in install.sh to reference the sibling migration step using
the correct relative path. Keep the change limited to the echoed follow-up
instruction near the Tempo/Grafana output so users can run the next script
directly.
In `@OTel/04-logs-migration/apply.sh`:
- Line 34: The next-step message in apply.sh is using a path that only works
from inside the script’s subprocess, so update the printed instruction to
reference a path that is valid from the caller’s shell. Adjust the next-step
output near the existing “Next:” message to use an explicit repo-root-relative
path or the sibling script path format (for example via the apply.sh handoff
text) so users can run 05-metrics-migration/apply.sh after this script exits.
In `@OTel/05-metrics-migration/apply.sh`:
- Line 33: The next-step path in apply.sh is incorrect because after changing
into OTel/05-metrics-migration, ./06-validation/deploy-demo.sh points to a
nonexistent child directory. Update the step reference in the apply.sh script to
use the sibling path to the validation step, and verify the transition path
after the directory change resolves correctly to deploy-demo.sh.
In `@OTel/08-yace-decision/README.md`:
- Around line 19-21: The README text and the apply.sh status message are
implying that AWS CloudWatch metrics will get the same Kubernetes enrichment as
pod-native telemetry, which is misleading. Update the wording around the
decision guidance and the apply.sh output to clarify that k8sattributes only
enriches telemetry when Kubernetes resource context is available, and that
awscloudwatchreceiver output usually won’t have that context. Use the existing
README decision copy and the apply.sh message so the guidance no longer suggests
AWS metrics will be enriched the same way as Kubernetes-native signals.
In `@OTel/docs/demo-script.md`:
- Around line 3-14: The demo script uses relative paths like
06-validation/demo.yaml and 07-prometheus-scrape-migration/, so add an explicit
note near the start of the walkthrough that the commands should be run from the
OTel/ working directory. Update the intro text in demo-script.md to mirror the
guidance used in OTel/README.md, making it clear before the kubectl commands
where the terminal should be positioned.
---
Nitpick comments:
In `@monitoring/chart-values/otel-gateway-prom-scrape-values.yaml`:
- Around line 166-187: The default logs pipeline still includes the debug
exporter, which makes the gateway echo every log record to stdout in addition to
sending it to Loki. Update the logs pipeline in the otel gateway values so the
service pipeline for logs uses only the intended exporters, removing debug from
the default configuration while keeping the otlphttp/loki path intact.
In `@monitoring/chart-values/otel-gateway-values.yaml`:
- Around line 122-148: Remove the debug exporter from the default logs pipeline
in the otel-gateway values so the gateway does not duplicate every log record to
stdout; keep the existing Loki export path in the service.pipelines.logs
configuration and, if needed, add debug only in a separate troubleshooting
overlay rather than in the base chart values.
In `@OTEL_MIGRATION.md`:
- Around line 25-30: Add language tags to the fenced code blocks in
OTEL_MIGRATION.md to satisfy MD040: update each affected fenced block around the
diagram sections and similar examples to use an explicit language like text or
bash, without changing the displayed content. Locate the markdown fences in the
migration doc and apply the same fix consistently across all referenced blocks
so the docs lint warning is cleared.
🪄 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: 84f86377-7c0e-413b-b625-19ab5c1a8d26
📒 Files selected for processing (37)
OTEL_MIGRATION.mdOTel/00-prereqs/install-macos.shOTel/01-cluster/kind-cluster.yamlOTel/01-cluster/up.shOTel/02-lgtm-stack/install.shOTel/02-lgtm-stack/loki-values.yamlOTel/02-lgtm-stack/prometheus-values.yamlOTel/02-lgtm-stack/tempo-values.yamlOTel/03-otel-collector/agent-values.yamlOTel/03-otel-collector/gateway-values.yamlOTel/03-otel-collector/install.shOTel/04-logs-migration/agent-values.yamlOTel/04-logs-migration/apply.shOTel/04-logs-migration/gateway-values.yamlOTel/05-metrics-migration/agent-values.yamlOTel/05-metrics-migration/apply.shOTel/05-metrics-migration/gateway-values.yamlOTel/06-validation/demo.yamlOTel/06-validation/deploy-demo.shOTel/07-prometheus-scrape-migration/apply.shOTel/07-prometheus-scrape-migration/gateway-values.yamlOTel/08-yace-decision/README.mdOTel/08-yace-decision/option-a-keep-yace/apply.shOTel/08-yace-decision/option-a-keep-yace/yace-values.yamlOTel/08-yace-decision/option-b-cloudwatch-receiver/apply.shOTel/08-yace-decision/option-b-cloudwatch-receiver/gateway-overlay-values.yamlOTel/README.mdOTel/docs/architecture.mdOTel/docs/demo-script.mdapp/robot-shop/robot-shop-local-values.yamldemo-1-before.shdemo-2-migrate-otel.shmonitoring/chart-values/loki-v3-values.yamlmonitoring/chart-values/otel-agent-values.yamlmonitoring/chart-values/otel-gateway-prom-scrape-values.yamlmonitoring/chart-values/otel-gateway-values.yamlmonitoring/dashboards/application.yaml
Comment on lines
+11
to
+12
| set -uo pipefail | ||
| cd "$(dirname "$0")" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target script with line numbers around the cited region and the wait string.
git ls-files demo-1-before.sh
wc -l demo-1-before.sh
sed -n '1,40p' demo-1-before.sh
sed -n '130,155p' demo-1-before.sh
# Look for the suspected job-name typo and related usages.
rg -n 'mysql-seeed|mysql-seed|wait.*mysql' demo-1-before.shRepository: infracloudio/sre-stack
Length of output: 2387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the wait_for_job implementation and the actual MySQL job name in manifests.
sed -n '30,55p' demo-1-before.sh
rg -n 'mysql-seeed|mysql-seed|job.*mysql|kind:\s*Job|name:\s*mysql' . --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**'Repository: infracloudio/sre-stack
Length of output: 6167
Fail fast on setup errors.
set -uo pipefailstill lets a failedcd/make/helm/kubectlstep continue against the wrong directory or a half-built cluster; switch toset -euo pipefailand fail oncd.mysql-seeedlooks misspelled; the manifest ismysql-seeder, so this wait will always time out and then continue anyway.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 12-12: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 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 `@demo-1-before.sh` around lines 11 - 12, The setup script does not fail fast
because `set -uo pipefail` still allows errors from `cd` and later setup steps
to continue, and the MySQL wait target is misspelled. Update the script near the
initial shell setup and the MySQL readiness check so it uses fail-fast behavior
with `set -euo pipefail`, ensures the `cd` failure stops execution, and corrects
the `mysql-seeed` identifier to the actual `mysql-seeder` name used by the
manifest.
Source: Linters/SAST tools
Comment on lines
+18
to
+19
| set -uo pipefail | ||
| cd "$(dirname "$0")" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg '^demo-2-migrate-otel\.sh$|^app/hotrod/base/hotrod/deployment\.yaml$|^demo-1-before\.sh$'
printf '\n--- demo-2-migrate-otel.sh ---\n'
cat -n demo-2-migrate-otel.sh | sed -n '1,260p'
printf '\n--- demo-1-before.sh ---\n'
cat -n demo-1-before.sh | sed -n '1,220p'
printf '\n--- app/hotrod/base/hotrod/deployment.yaml ---\n'
cat -n app/hotrod/base/hotrod/deployment.yaml | sed -n '1,220p'Repository: infracloudio/sre-stack
Length of output: 25318
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "kubectl set env|wait .*Running|rollout status|cd \"\\$\\(dirname \"\\$0\"\\)\"|set -u.*pipefail|set -euo pipefail" demo-2-migrate-otel.sh demo-1-before.shRepository: infracloudio/sre-stack
Length of output: 313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in [Path("demo-2-migrate-otel.sh"), Path("demo-1-before.sh"), Path("app/hotrod/base/hotrod/deployment.yaml")]:
if p.exists():
print(f"\n### {p} lines {sum(1 for _ in p.open())}")
PYRepository: infracloudio/sre-stack
Length of output: 283
Fail fast and wait for the HotROD rollout.
- Add
set -eand guardcdat the top so a failed directory change or Helm/Kubernetes command stops the migration instead of continuing in a partially applied state. - After
kubectl set env, wait forkubectl rollout status deployment/hotrod -n hotrod; the currentRunningcheck can pass while the old pod is still serving the previous exporter endpoint.
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 19-19: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 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 `@demo-2-migrate-otel.sh` around lines 18 - 19, Add fail-fast handling at the
top of the migration script by including set -e alongside the existing shell
options and making the cd guard stop execution if the directory change fails, so
later Helm/Kubernetes steps do not continue in a bad state. Then, in the HotROD
update flow after kubectl set env, replace the current Running-only readiness
check with a kubectl rollout status wait for deployment/hotrod in the hotrod
namespace, so the script does not proceed until the new rollout is actually
serving.
Source: Linters/SAST tools
Comment on lines
+172
to
+177
| kubectl set env deployment/hotrod -n hotrod \ | ||
| JAEGER_ENDPOINT- \ | ||
| OTEL_EXPORTER_OTLP_ENDPOINT=http://otel-gateway.monitoring.svc.cluster.local:4318 | ||
|
|
||
| sleep 5 | ||
| wait_for_pod "hotrod" "hotrod" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Wait for the HotROD rollout, not just any running pod.
kubectl set env creates a new ReplicaSet, but wait_for_pod "hotrod" returns as soon as the old pod is still Running. That makes Step 7 race with the restart and can send traffic before OTEL_EXPORTER_OTLP_ENDPOINT is actually switched, even though app/hotrod/base/hotrod/deployment.yaml:19-25 reads that env var at startup.
Suggested fix
kubectl set env deployment/hotrod -n hotrod \
JAEGER_ENDPOINT- \
OTEL_EXPORTER_OTLP_ENDPOINT=http://otel-gateway.monitoring.svc.cluster.local:4318
-sleep 5
-wait_for_pod "hotrod" "hotrod"
+kubectl rollout status deployment/hotrod -n hotrod
kubectl get pods -n hotrod📝 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.
Suggested change
| kubectl set env deployment/hotrod -n hotrod \ | |
| JAEGER_ENDPOINT- \ | |
| OTEL_EXPORTER_OTLP_ENDPOINT=http://otel-gateway.monitoring.svc.cluster.local:4318 | |
| sleep 5 | |
| wait_for_pod "hotrod" "hotrod" | |
| kubectl set env deployment/hotrod -n hotrod \ | |
| JAEGER_ENDPOINT- \ | |
| OTEL_EXPORTER_OTLP_ENDPOINT=http://otel-gateway.monitoring.svc.cluster.local:4318 | |
| kubectl rollout status deployment/hotrod -n hotrod | |
| kubectl get pods -n hotrod |
🤖 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 `@demo-2-migrate-otel.sh` around lines 172 - 177, The HotROD step is waiting
for any running pod instead of the deployment rollout, so the env change may not
be applied before traffic resumes. Update the rollout synchronization after the
kubectl set env call to wait on the hotrod Deployment/ReplicaSet becoming ready
rather than using wait_for_pod "hotrod" "hotrod", and keep the check tied to the
HotROD deployment and its startup env handling in the deployment manifest.
Comment on lines
+11
to
+18
| echo "==> Installing Docker Desktop (if missing)" | ||
| # Docker Desktop is required for kind. If you already have OrbStack or colima, | ||
| # you can skip this — but the rest of the script assumes 'docker' resolves. | ||
| if ! command -v docker >/dev/null 2>&1; then | ||
| brew install --cask docker | ||
| echo "Docker installed. Open Docker Desktop once so its daemon starts, then re-run this script." | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Verify the Docker daemon, not just the CLI.
This passes as soon as docker is on PATH, but kind still fails if Docker Desktop is installed and not running yet. A quick docker info/docker ps check here would catch that before the user reaches cluster creation.
Suggested fix
echo "==> Installing Docker Desktop (if missing)"
# Docker Desktop is required for kind. If you already have OrbStack or colima,
# you can skip this — but the rest of the script assumes 'docker' resolves.
if ! command -v docker >/dev/null 2>&1; then
brew install --cask docker
echo "Docker installed. Open Docker Desktop once so its daemon starts, then re-run this script."
exit 0
fi
+
+if ! docker info >/dev/null 2>&1; then
+ echo "Docker is installed but the daemon is not running. Start Docker Desktop (or your Docker runtime) and re-run this script." >&2
+ exit 1
+fi📝 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.
Suggested change
| echo "==> Installing Docker Desktop (if missing)" | |
| # Docker Desktop is required for kind. If you already have OrbStack or colima, | |
| # you can skip this — but the rest of the script assumes 'docker' resolves. | |
| if ! command -v docker >/dev/null 2>&1; then | |
| brew install --cask docker | |
| echo "Docker installed. Open Docker Desktop once so its daemon starts, then re-run this script." | |
| exit 0 | |
| fi | |
| echo "==> Installing Docker Desktop (if missing)" | |
| # Docker Desktop is required for kind. If you already have OrbStack or colima, | |
| # you can skip this — but the rest of the script assumes 'docker' resolves. | |
| if ! command -v docker >/dev/null 2>&1; then | |
| brew install --cask docker | |
| echo "Docker installed. Open Docker Desktop once so its daemon starts, then re-run this script." | |
| exit 0 | |
| fi | |
| if ! docker info >/dev/null 2>&1; then | |
| echo "Docker is installed but the daemon is not running. Start Docker Desktop (or your Docker runtime) and re-run this script." >&2 | |
| exit 1 | |
| fi |
🤖 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 `@OTel/00-prereqs/install-macos.sh` around lines 11 - 18, The Docker readiness
check in install-macos.sh only verifies that the docker CLI exists, but it
should also verify the daemon is running before continuing. Update the install
flow around the existing docker command check to also run a quick daemon probe
such as docker info or docker ps, and if it fails, print a clear message and
stop before kind setup proceeds. Keep the logic localized to the Docker Desktop
install/check block so the script only continues when both the CLI and daemon
are available.
Comment on lines
+32
to
+37
| # Loki writes WAL + chunks to /var/loki. With persistence disabled the | ||
| # container's root filesystem is read-only, so it must have *some* writable | ||
| # volume — give it a 2Gi PVC backed by kind's local-path provisioner. | ||
| persistence: | ||
| enabled: true | ||
| size: 2Gi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## files\n'
git ls-files | rg '^(OTel/02-lgtm-stack/loki-values\.yaml|.*kind.*|.*storageclass.*|.*local-path.*|.*bootstrap.*|.*cluster.*)$' || true
printf '\n## loki values excerpt\n'
cat -n OTel/02-lgtm-stack/loki-values.yaml | sed -n '1,120p'
printf '\n## search for provisioner / storageclass references\n'
rg -n --hidden --glob '!*node_modules*' --glob '!*dist*' --glob '!*build*' --glob '!*vendor*' \
'local-path|StorageClass|storage provisioner|kind cluster|kind:' OTel -S || trueRepository: infracloudio/sre-stack
Length of output: 3335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## file tree around OTel/02-lgtm-stack\n'
fd -t f . OTel/02-lgtm-stack 2>/dev/null || true
printf '\n## find cluster/bootstrap yaml files\n'
fd -t f -e yaml -e yml . | rg 'kind|cluster|bootstrap|storageclass|local-path|lgtm|otel' || true
printf '\n## inspect likely cluster config files\n'
for f in $(fd -t f -e yaml -e yml . | rg 'kind|cluster|bootstrap|storageclass|local-path'); do
echo "### $f"
cat -n "$f" | sed -n '1,220p'
echo
doneRepository: infracloudio/sre-stack
Length of output: 9506
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## repository overview\n'
git ls-files | head -n 200
printf '\n## targeted search for kind/local-path in all tracked files\n'
rg -n --hidden --glob '!*node_modules*' --glob '!*dist*' --glob '!*build*' --glob '!*vendor*' \
'local-path|rancher.io/local-path|storageclass\.kubernetes\.io/is-default-class|kind create cluster|kind:' . || trueRepository: infracloudio/sre-stack
Length of output: 16295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## OTel/01-cluster/up.sh\n'
cat -n OTel/01-cluster/up.sh | sed -n '1,200p'
printf '\n## OTel/02-lgtm-stack/install.sh\n'
cat -n OTel/02-lgtm-stack/install.sh | sed -n '1,240p'
printf '\n## OTel/README.md excerpts\n'
cat -n OTel/README.md | sed -n '1,220p'Repository: infracloudio/sre-stack
Length of output: 5755
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## search for gp2-storageclass usage\n'
rg -n 'gp2-storageclass|gp2-storageclass.yaml|local-path provisioner|rancher.io/local-path' OTel infra monitoring app -S || true
printf '\n## search for StorageClass application in OTel scripts\n'
rg -n 'kubectl apply|kustomize|helm|storageclass|gp2' OTel/01-cluster OTel/02-lgtm-stack -S || trueRepository: infracloudio/sre-stack
Length of output: 1443
Provision a StorageClass before enabling this PVC. OTel/01-cluster/up.sh only creates a plain kind cluster, and OTel/02-lgtm-stack/install.sh does not install a default StorageClass/provisioner. On a fresh lab cluster this claim will stay Pending and Loki will not start.
🤖 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 `@OTel/02-lgtm-stack/loki-values.yaml` around lines 32 - 37, The Loki PVC in
the values config is being enabled without ensuring a backing StorageClass
exists, so the claim can remain Pending and block startup. Update the setup flow
referenced by the cluster bootstrap and install scripts so a default
StorageClass/provisioner is created before Helm applies the Loki release, and
keep the PVC settings in loki-values aligned with that provisioner. Use the
existing Loki persistence block and the cluster/install entrypoints as the main
places to add the StorageClass setup and readiness check.
Comment on lines
+81
to
+83
| # Promote useful resource attributes to Prometheus labels | ||
| resource_to_telemetry_conversion: | ||
| enabled: true |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid converting the full resource set into Prometheus labels.
On Line 82, resource_to_telemetry_conversion.enabled: true will turn every resource attribute into series labels, and this config also extracts all pod labels on both collector tiers. That will push volatile attributes like k8s.pod.name and arbitrary pod labels into Prometheus, which is a fast path to cardinality and storage blow-ups.
Suggested fix
prometheusremotewrite/prom:
endpoint: http://kps-prometheus.observability.svc:9090/api/v1/write
tls:
insecure: true
- # Promote useful resource attributes to Prometheus labels
- resource_to_telemetry_conversion:
- enabled: true
+ # Keep this off until a transform/attributes step explicitly whitelists
+ # the small set of stable labels you want on Prometheus series.
+ resource_to_telemetry_conversion:
+ enabled: false📝 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.
Suggested change
| # Promote useful resource attributes to Prometheus labels | |
| resource_to_telemetry_conversion: | |
| enabled: true | |
| # Keep this off until a transform/attributes step explicitly whitelists | |
| # the small set of stable labels you want on Prometheus series. | |
| resource_to_telemetry_conversion: | |
| enabled: false |
🤖 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 `@OTel/05-metrics-migration/gateway-values.yaml` around lines 81 - 83, The
Prometheus relabeling config in gateway-values.yaml is too broad because
resource_to_telemetry_conversion.enabled under the gateway/collector settings
turns all resource attributes into labels. Update this block so only the
specific useful attributes are promoted, and avoid enabling blanket conversion
in the gateway pipeline; keep the change localized around the
resource_to_telemetry_conversion section and any pod-label extraction settings
used by the collector tiers.
Comment on lines
+1
to
+3
| # Three short-lived workloads, all using service.name="demo-app", emitting one | ||
| # signal each through the agent's local OTLP endpoint (the DaemonSet exposes | ||
| # 4317 on every node's pod IP — we resolve via the agent's headless Service). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
This demo skips the agent and can't support the trace→logs validation.
Both Jobs send OTLP directly to otel-gateway.observability.svc:4317, so step 06 bypasses the agent despite the file header saying it exercises the local agent endpoint. On top of that, the only log producer is a separate demo-logger Deployment emitting synthetic trace_id values that are unrelated to the spans from demo-traces, so the downstream “Logs for this span” check cannot work as written. Route the generators through the agent and emit correlated logs/traces from the same workload before advertising cross-signal validation.
Also applies to: 25-30, 48-52, 54-85
🤖 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 `@OTel/06-validation/demo.yaml` around lines 1 - 3, The demo is bypassing the
agent and the trace/log correlation is not actually supported as written. Update
the workloads in demo.yaml so the Jobs and logger route OTLP through the agent’s
local endpoint instead of otel-gateway.observability.svc:4317, and make the log
producer emit logs correlated with the same spans from demo-traces rather than
synthetic trace_id values. Also align the file header/comments with the actual
data path so the 06-validation flow accurately describes agent-based trace→logs
validation.
Comment on lines
+5
to
+6
| kubectl apply -f demo.yaml | ||
| kubectl -n demo wait --for=condition=available --timeout=60s deploy/demo-logger || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't report success before the demo emitters are ready.
kubectl wait ... || true masks a failed demo-logger rollout, and the script never waits for demo-traces / demo-metrics to complete. That makes the “Demo deployed” banner and the Grafana checks race-prone.
Suggested fix
kubectl apply -f demo.yaml
-kubectl -n demo wait --for=condition=available --timeout=60s deploy/demo-logger || true
+kubectl -n demo wait --for=condition=available --timeout=60s deploy/demo-logger
+kubectl -n demo wait --for=condition=complete --timeout=120s job/demo-traces
+kubectl -n demo wait --for=condition=complete --timeout=120s job/demo-metrics📝 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.
Suggested change
| kubectl apply -f demo.yaml | |
| kubectl -n demo wait --for=condition=available --timeout=60s deploy/demo-logger || true | |
| kubectl apply -f demo.yaml | |
| kubectl -n demo wait --for=condition=available --timeout=60s deploy/demo-logger | |
| kubectl -n demo wait --for=condition=complete --timeout=120s job/demo-traces | |
| kubectl -n demo wait --for=condition=complete --timeout=120s job/demo-metrics |
🤖 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 `@OTel/06-validation/deploy-demo.sh` around lines 5 - 6, The deployment script
currently treats the demo as ready after only waiting on `demo-logger`, and the
`|| true` masks failures, so the success banner can appear too early. Update
`deploy-demo.sh` to wait for all demo emitters to become available, including
`demo-logger`, `demo-traces`, and `demo-metrics`, and remove the failure
suppression so rollout problems are surfaced. Use the existing `kubectl wait`
flow in the script as the place to add the additional readiness checks before
printing any “Demo deployed” success message.
Comment on lines
+6
to
+9
| if ! kubectl -n "${NS}" get secret otel-aws >/dev/null 2>&1; then | ||
| echo "Create the otel-aws Secret first (see overlay file header)." >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the required secret keys, not just secret existence.
This guard only proves that otel-aws exists. The overlay also depends on AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and AWS_REGION; if any are missing, the upgrade succeeds but awscloudwatchreceiver still comes up misconfigured.
Suggested fix
-if ! kubectl -n "${NS}" get secret otel-aws >/dev/null 2>&1; then
- echo "Create the otel-aws Secret first (see overlay file header)." >&2
- exit 1
-fi
+if ! kubectl -n "${NS}" get secret otel-aws >/dev/null 2>&1; then
+ echo "Create the otel-aws Secret first (see overlay file header)." >&2
+ exit 1
+fi
+
+for key in AWS_ACCESS_KEY_ID AWS_SECRET_ACCESS_KEY AWS_REGION; do
+ if ! kubectl -n "${NS}" get secret otel-aws -o "jsonpath={.data.${key}}" | grep -q .; then
+ echo "Secret ${NS}/otel-aws is missing ${key}." >&2
+ exit 1
+ fi
+done📝 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.
Suggested change
| if ! kubectl -n "${NS}" get secret otel-aws >/dev/null 2>&1; then | |
| echo "Create the otel-aws Secret first (see overlay file header)." >&2 | |
| exit 1 | |
| fi | |
| if ! kubectl -n "${NS}" get secret otel-aws >/dev/null 2>&1; then | |
| echo "Create the otel-aws Secret first (see overlay file header)." >&2 | |
| exit 1 | |
| fi | |
| for key in AWS_ACCESS_KEY_ID AWS_SECRET_ACCESS_KEY AWS_REGION; do | |
| if ! kubectl -n "${NS}" get secret otel-aws -o "jsonpath={.data.${key}}" | grep -q .; then | |
| echo "Secret ${NS}/otel-aws is missing ${key}." >&2 | |
| exit 1 | |
| fi | |
| done |
🤖 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 `@OTel/08-yace-decision/option-b-cloudwatch-receiver/apply.sh` around lines 6 -
9, The current guard in apply.sh only checks that the otel-aws secret exists,
but it should also verify the required keys are present before proceeding.
Update the kubectl validation block to inspect the otel-aws secret for
AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and AWS_REGION, and fail fast with a
clear error if any are missing. Use the existing apply.sh secret check as the
entry point so the script stops before the awscloudwatchreceiver is deployed
with an incomplete configuration.
Comment on lines
+1
to
+2
| # Overlay onto 05-metrics-migration/gateway-values.yaml (or 07). | ||
| # Adds awscloudwatchreceiver to the gateway's metrics pipeline. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files 'OTel/08-yace-decision/option-b-cloudwatch-receiver/gateway-overlay-values.yaml' \
'OTel/07-prometheus-scrape-migration/gateway-values.yaml' \
'OTel/05-metrics-migration/gateway-values.yaml' \
'OTel/08-yace-decision/option-b-cloudwatch-receiver/*' \
| sed 's#^`#-` #'
printf '\nOutline 08 overlay:\n'
ast-grep outline OTel/08-yace-decision/option-b-cloudwatch-receiver/gateway-overlay-values.yaml --view expanded || true
printf '\nOutline 07 gateway values:\n'
ast-grep outline OTel/07-prometheus-scrape-migration/gateway-values.yaml --view expanded || true
printf '\nRelevant snippets:\n'
for f in \
OTel/08-yace-decision/option-b-cloudwatch-receiver/gateway-overlay-values.yaml \
OTel/07-prometheus-scrape-migration/gateway-values.yaml \
OTel/05-metrics-migration/gateway-values.yaml
do
echo "== $f =="
wc -l "$f"
sed -n '1,120p' "$f"
echo
doneRepository: infracloudio/sre-stack
Length of output: 8440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os, yaml, json, pathlib, sys
paths = [
"OTel/08-yace-decision/option-b-cloudwatch-receiver/gateway-overlay-values.yaml",
"OTel/07-prometheus-scrape-migration/gateway-values.yaml",
"OTel/05-metrics-migration/gateway-values.yaml",
]
for p in paths:
print(f"\n### {p}")
with open(p, "r", encoding="utf-8") as f:
data = yaml.safe_load(f)
# Print the relevant tree if present
service = data.get("service", {})
pipelines = service.get("pipelines", {})
metrics = pipelines.get("metrics", {})
print(json.dumps({
"service.pipelines.metrics": metrics,
"full_keys": list(data.keys()),
}, indent=2, sort_keys=True))
PYRepository: infracloudio/sre-stack
Length of output: 877
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import yaml, copy, json
base = yaml.safe_load(Path("OTel/07-prometheus-scrape-migration/gateway-values.yaml").read_text())
overlay = yaml.safe_load(Path("OTel/08-yace-decision/option-b-cloudwatch-receiver/gateway-overlay-values.yaml").read_text())
def deep_merge(a, b):
# approximate Helm values merge: maps merge recursively, lists/scalars replace
if isinstance(a, dict) and isinstance(b, dict):
out = dict(a)
for k, v in b.items():
out[k] = deep_merge(out[k], v) if k in out else copy.deepcopy(v)
return out
return copy.deepcopy(b)
merged = deep_merge(base, overlay)
print("base metrics receivers:", base["service"]["pipelines"]["metrics"].get("receivers"))
print("overlay metrics receivers:", overlay["service"]["pipelines"]["metrics"].get("receivers"))
print("merged metrics receivers:", merged["service"]["pipelines"]["metrics"].get("receivers"))
print("base metrics exporters:", base["service"]["pipelines"]["metrics"].get("exporters"))
print("merged metrics exporters:", merged["service"]["pipelines"]["metrics"].get("exporters"))
PYRepository: infracloudio/sre-stack
Length of output: 256
Keep prometheus in the step 07 overlay
config.service.pipelines.metrics.receivers: [otlp, awscloudwatch] replaces the step 07 list, so layering this on OTel/07-prometheus-scrape-migration/gateway-values.yaml drops prometheus and stops scrape ingestion. Split the step 05 and step 07 overlays, or preserve prometheus in the step 07 path.
🤖 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
`@OTel/08-yace-decision/option-b-cloudwatch-receiver/gateway-overlay-values.yaml`
around lines 1 - 2, The metrics pipeline overlay is replacing the step 07
receiver list and unintentionally dropping prometheus from
`config.service.pipelines.metrics.receivers`, which breaks scrape ingestion.
Update `gateway-overlay-values.yaml` so the step 07 path preserves the existing
`prometheus` receiver when adding `awscloudwatch` (or split the step 05 and step
07 overlays so the cloudwatch receiver is layered separately). Make the change
in the overlay that feeds `gateway-values.yaml` and keep the receiver list
additive rather than overwriting the step 07 configuration.
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
2 participants
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.
full OTel migration: two-tier collector, Loki 3.x, demo scripts
Summary by CodeRabbit
New Features
Documentation