fix monitoring dashboards and logging - #172
Conversation
📝 WalkthroughWalkthroughPromtail now runs as root, gains Docker and Kubernetes log discovery, and emits deployment diagnostics. Grafana provisioning replaces the sample dashboard with GenAI and Spring Services dashboards under a renamed provider. ChangesPromtail monitoring
Grafana dashboard provisioning
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
Preview deploymentCommit:
Monitoring
|
GitHub Actions
Failure
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
infra/ansible/roles/app/templates/docker-compose.prod.yml.j2 (1)
128-130: 🔒 Security & Privacy | 🔵 TrivialConsider native Docker logging instead of running Promtail as root.
Promtail is configured to run as
rootand mount the Docker socket to discover and tail container logs. While the socket mount is read-only (mitigating some risks), running as root grants Promtail the ability to read all container environment variables, secrets, and host log directories.If your primary goal is to ship Docker Compose logs to Loki, consider using the native Docker Loki logging driver instead. This architecture pushes logs directly from the Docker daemon to Loki, entirely eliminating the need for Promtail, root privileges, and socket mounts in your Compose stack.
🤖 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 `@infra/ansible/roles/app/templates/docker-compose.prod.yml.j2` around lines 128 - 130, Replace the root-running promtail service with Docker’s native Loki logging driver for Compose services that need log shipping. Configure the driver and its Loki connection options at the appropriate Compose scope, then remove the promtail service and its Docker socket, log-directory, and related mounts while preserving Loki delivery behavior.
🤖 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 `@infra/ansible/roles/app/tasks/main.yml`:
- Around line 207-208: Update the diagnostic command in the task’s container log
directory section to run the /var/lib/docker/containers listing through sudo,
preserving the existing head -20 output limit and diagnostic message.
In `@infra/helm/aidan-monitoring/promtail.yml`:
- Around line 12-16: Update the docker-containers scrape job’s relabel_configs
to construct the Docker container log __path__ from the discovered container ID,
enabling Promtail to read the files. Also add the docker pipeline stage to parse
JSON-wrapped Docker logs correctly, while preserving the existing Docker
discovery configuration.
- Around line 35-40: Update the __path__ relabel rule to use the single capture
group produced by its default regex, removing the invalid $2 reference and
ensuring the replacement glob correctly incorporates the combined pod
UID/container name value.
---
Nitpick comments:
In `@infra/ansible/roles/app/templates/docker-compose.prod.yml.j2`:
- Around line 128-130: Replace the root-running promtail service with Docker’s
native Loki logging driver for Compose services that need log shipping.
Configure the driver and its Loki connection options at the appropriate Compose
scope, then remove the promtail service and its Docker socket, log-directory,
and related mounts while preserving Loki delivery behavior.
🪄 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: 5c05a957-46d4-47f1-96fb-dc783c8328d7
📒 Files selected for processing (5)
infra/ansible/roles/app/tasks/main.ymlinfra/ansible/roles/app/templates/docker-compose.prod.yml.j2infra/helm/aidan-monitoring/grafana/dashboards/dashboards.ymlinfra/helm/aidan-monitoring/promtail.ymlinfra/helm/aidan-monitoring/templates/configmaps.yaml
| echo "=== container log dir on host ===" | ||
| ls -la /var/lib/docker/containers | head -20 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use sudo to access the protected Docker log directory.
The /var/lib/docker/containers directory is typically restricted to root access on the host. Because this task executes as azureuser (become_user: azureuser), this command will fail with a Permission denied error and the intended diagnostic output will be lost.
🛠️ Proposed fix
echo "=== container log dir on host ==="
- ls -la /var/lib/docker/containers | head -20
+ sudo ls -la /var/lib/docker/containers | head -20📝 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.
| echo "=== container log dir on host ===" | |
| ls -la /var/lib/docker/containers | head -20 | |
| echo "=== container log dir on host ===" | |
| sudo ls -la /var/lib/docker/containers | head -20 |
🤖 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 `@infra/ansible/roles/app/tasks/main.yml` around lines 207 - 208, Update the
diagnostic command in the task’s container log directory section to run the
/var/lib/docker/containers listing through sudo, preserving the existing head
-20 output limit and diagnostic message.
| - job_name: docker-containers | ||
| docker_sd_configs: | ||
| - host: unix:///var/run/docker.sock | ||
| refresh_interval: 5s | ||
| relabel_configs: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Set the __path__ label to enable log ingestion for Docker containers.
The docker-containers scrape job discovers container metadata via the Docker socket, but it never sets the __path__ label. Unlike Prometheus, Promtail strictly requires __path__ to know which files to read on the filesystem. Without it, this job will discover targets but fail to ingest any logs.
You must add a relabel rule to construct the log path from the container ID. Additionally, including the docker pipeline stage ensures the JSON-wrapped Docker logs are parsed correctly.
🛠️ Proposed fix
- job_name: docker-containers
+ pipeline_stages:
+ - docker: {}
docker_sd_configs:
- host: unix:///var/run/docker.sock
refresh_interval: 5s
relabel_configs:
+ - source_labels: ['__meta_docker_container_id']
+ target_label: '__path__'
+ replacement: '/var/lib/docker/containers/$1/*-json.log'📝 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.
| - job_name: docker-containers | |
| docker_sd_configs: | |
| - host: unix:///var/run/docker.sock | |
| refresh_interval: 5s | |
| relabel_configs: | |
| - job_name: docker-containers | |
| pipeline_stages: | |
| - docker: {} | |
| docker_sd_configs: | |
| - host: unix:///var/run/docker.sock | |
| refresh_interval: 5s | |
| relabel_configs: | |
| - source_labels: ['__meta_docker_container_id'] | |
| target_label: '__path__' | |
| replacement: '/var/lib/docker/containers/$1/*-json.log' |
🤖 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 `@infra/helm/aidan-monitoring/promtail.yml` around lines 12 - 16, Update the
docker-containers scrape job’s relabel_configs to construct the Docker container
log __path__ from the discovered container ID, enabling Promtail to read the
files. Also add the docker pipeline stage to parse JSON-wrapped Docker logs
correctly, while preserving the existing Docker discovery configuration.
| # 3. Automatically map the K8s API log path (Promtail handles the path details behind the scenes) | ||
| - action: replace | ||
| source_labels: ['__meta_kubernetes_pod_uid', '__meta_kubernetes_pod_container_name'] | ||
| separator: '/' | ||
| target_label: '__path__' | ||
| replacement: '/var/log/pods/*$1*/*$2*/*.log' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix the logically broken __path__ replacement syntax.
The replacement string '/var/log/pods/*$1*/*$2*/*.log' is broken because no regex is provided. By default, the regex is (.*), which captures the entire concatenated string (<uid>/<container_name>) into $1 and leaves $2 empty.
The resulting __path__ evaluates to /var/log/pods/*<uid>/<container_name>*//*/*.log, which is an invalid glob pattern that will fail to match any K8s pod logs. You can fix this by simplifying the replacement to utilize the single captured group.
🛠️ Proposed fix
# 3. Automatically map the K8s API log path (Promtail handles the path details behind the scenes)
- action: replace
source_labels: ['__meta_kubernetes_pod_uid', '__meta_kubernetes_pod_container_name']
separator: '/'
target_label: '__path__'
- replacement: '/var/log/pods/*$1*/*$2*/*.log'
+ replacement: '/var/log/pods/*$1/*.log'📝 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.
| # 3. Automatically map the K8s API log path (Promtail handles the path details behind the scenes) | |
| - action: replace | |
| source_labels: ['__meta_kubernetes_pod_uid', '__meta_kubernetes_pod_container_name'] | |
| separator: '/' | |
| target_label: '__path__' | |
| replacement: '/var/log/pods/*$1*/*$2*/*.log' | |
| # 3. Automatically map the K8s API log path (Promtail handles the path details behind the scenes) | |
| - action: replace | |
| source_labels: ['__meta_kubernetes_pod_uid', '__meta_kubernetes_pod_container_name'] | |
| separator: '/' | |
| target_label: '__path__' | |
| replacement: '/var/log/pods/*$1/*.log' |
🤖 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 `@infra/helm/aidan-monitoring/promtail.yml` around lines 35 - 40, Update the
__path__ relabel rule to use the single capture group produced by its default
regex, removing the invalid $2 reference and ensuring the replacement glob
correctly incorporates the combined pod UID/container name value.
This branch was previously deployed
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores