[monitoring] Add slack dashboard url and vmagent env label and dynamictext-panel to Grafna#2210
[monitoring] Add slack dashboard url and vmagent env label and dynamictext-panel to Grafna#2210vnyakas wants to merge 1 commit intocozystack:maincozystack/cozystack:mainfrom alphacruncher:monitoring/add-slack-dashboard-url-and-vmagent-env-labelalphacruncher/cozystack:monitoring/add-slack-dashboard-url-and-vmagent-env-labelCopy head branch name to clipboard
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughAdds a Grafana panel plugin installation; adds two optional Alerta Slack env vars ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
There was a problem hiding this comment.
Code Review
The pull request introduces a new Grafana plugin (marcusolsson-dynamictext-panel), adds configurable DASHBOARD_URL and SLACK_SUMMARY_FMT environment variables for Alerta's Slack integration, and includes an optional environment external label for vmagent. Review feedback suggests improving the robustness of Helm templates by quoting the environment label and Alerta Slack configuration values to prevent YAML parsing issues, and optimizing the Grafana Dockerfile by combining multiple grafana-cli plugins install commands into a single RUN instruction to reduce image layers.
| cluster: {{ .Values.vmagent.externalLabels.cluster }} | ||
| tenant: {{ .Release.Namespace }} | ||
| {{- if .Values.vmagent.externalLabels.environment }} | ||
| environment: {{ .Values.vmagent.externalLabels.environment }} |
There was a problem hiding this comment.
The value for the environment label should be quoted to ensure it's always treated as a string, regardless of its content. Without quotes, values like true, false, 123, or null would be interpreted as booleans, numbers, or nulls by the YAML parser, which is likely not the intended behavior for a label. Please use the quote function.
environment: {{ .Values.vmagent.externalLabels.environment | quote }}| RUN grafana-cli --pluginsDir /var/lib/grafana-plugins plugins install natel-discrete-panel | ||
| RUN grafana-cli --pluginsDir /var/lib/grafana-plugins plugins install grafana-worldmap-panel | ||
| RUN grafana-cli --pluginsDir /var/lib/grafana-plugins plugins install marcusolsson-dynamictext-panel |
There was a problem hiding this comment.
To reduce the number of layers in the Docker image and improve build efficiency, you can combine the grafana-cli plugins install commands into a single RUN instruction. The grafana-cli supports installing multiple plugins at once.
RUN grafana-cli --pluginsDir /var/lib/grafana-plugins plugins install natel-discrete-panel grafana-worldmap-panel marcusolsson-dynamictext-panel
| {{- if .Values.alerta.alerts.slack.dashboardUrl }} | ||
| - name: DASHBOARD_URL | ||
| value: "{{ .Values.alerta.alerts.slack.dashboardUrl }}" | ||
| {{- end }} | ||
| {{- if .Values.alerta.alerts.slack.summaryFmt }} | ||
| - name: SLACK_SUMMARY_FMT | ||
| value: "{{ .Values.alerta.alerts.slack.summaryFmt }}" | ||
| {{- end }} |
There was a problem hiding this comment.
It's a best practice in Helm templates to use the quote function to wrap string values. This prevents issues if the values for dashboardUrl or summaryFmt contain special characters that could break the YAML structure. Please apply the quote function to both values for safety and consistency.
{{- if .Values.alerta.alerts.slack.dashboardUrl }}
- name: DASHBOARD_URL
value: {{ .Values.alerta.alerts.slack.dashboardUrl | quote }}
{{- end }}
{{- if .Values.alerta.alerts.slack.summaryFmt }}
- name: SLACK_SUMMARY_FMT
value: {{ .Values.alerta.alerts.slack.summaryFmt | quote }}
{{- end }}There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/system/monitoring/images/grafana/Dockerfile (1)
16-16: Pin the Grafana plugin to a specific version.Line 16 installs the latest release of
marcusolsson-dynamictext-panelat build time, which makes the image non-reproducible and exposes future rebuilds to upstream incompatibilities. Usegrafana-cli plugins install marcusolsson-dynamictext-panel <version>with an explicit version compatible with Grafana 11.4.0 (e.g., 6.2.0 or later 6.x release).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/monitoring/images/grafana/Dockerfile` at line 16, The Dockerfile currently installs marcusolsson-dynamictext-panel without a version; update the RUN command that invokes grafana-cli (the `grafana-cli --pluginsDir /var/lib/grafana-plugins plugins install marcusolsson-dynamictext-panel` line) to pin an explicit, tested release (for Grafana 11.4.0 use e.g. `6.2.0` or another 6.x tag) so the image is reproducible and resilient to upstream changes; replace the install call with `plugins install marcusolsson-dynamictext-panel <version>` and ensure the chosen version is compatible with Grafana 11.4.0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/system/monitoring/templates/alerta/alerta.yaml`:
- Around line 143-149: Apply Helm's quote filter to both
.Values.alerta.alerts.slack.dashboardUrl and
.Values.alerta.alerts.slack.summaryFmt in the alerta.yaml template so the
environment values are properly escaped (matching the existing use of quote for
disabledSeverity); replace the manual double-quote wrapping with the quote
filter for the DASHBOARD_URL and SLACK_SUMMARY_FMT value fields.
---
Nitpick comments:
In `@packages/system/monitoring/images/grafana/Dockerfile`:
- Line 16: The Dockerfile currently installs marcusolsson-dynamictext-panel
without a version; update the RUN command that invokes grafana-cli (the
`grafana-cli --pluginsDir /var/lib/grafana-plugins plugins install
marcusolsson-dynamictext-panel` line) to pin an explicit, tested release (for
Grafana 11.4.0 use e.g. `6.2.0` or another 6.x tag) so the image is reproducible
and resilient to upstream changes; replace the install call with `plugins
install marcusolsson-dynamictext-panel <version>` and ensure the chosen version
is compatible with Grafana 11.4.0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c94d8c5e-b4f7-4da8-9454-7e7f4704de52
📒 Files selected for processing (4)
packages/system/monitoring/images/grafana/Dockerfilepackages/system/monitoring/templates/alerta/alerta.yamlpackages/system/monitoring/templates/vm/vmagent.yamlpackages/system/monitoring/values.yaml
|
@vnyakas please fix DCO: that should be included on all commits in this PR, please consider squashing them |
48578c0 to
073e171
Compare
|
@kvaps Fixed DCO |
|
Hey! Could you please rebase your branch onto the latest git fetch upstream main
git rebase upstream/mainThe conflict is straightforward — just accept the upstream version (keep the 6-space indentation for URLs and the new - http://vminsert-shortterm:8480/insert/0/prometheus
- http://vminsert-longterm:8480/insert/0/prometheus
## inlineScrapeConfig: |
## - job_name: "custom"
## static_configs:
## - targets: ["my-service:9090"]Then force-push: git push --force-with-lease origin monitoring/add-slack-dashboard-url-and-vmagent-env-labelThanks! |
…nt environment label, and dynamictext Grafana plugin Signed-off-by: Viktor Nyakas <viktor.nyakas@alphacruncher.com>
073e171 to
83c0271
Compare
|
Done |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/system/monitoring/images/grafana/Dockerfile (1)
16-16: Pinmarcusolsson-dynamictext-panelto an explicit version.Unpinned plugin installs make container builds non-deterministic and can introduce unexpected breakage when upstream publishes updates. Use the
grafana-cli plugins install <plugin-id> <version>syntax (space-separated) to specify an explicit version.Proposed change
+ARG DYNAMICTEXT_PANEL_VERSION=1.3.0 -RUN grafana-cli --pluginsDir /var/lib/grafana-plugins plugins install marcusolsson-dynamictext-panel +RUN grafana-cli --pluginsDir /var/lib/grafana-plugins plugins install marcusolsson-dynamictext-panel ${DYNAMICTEXT_PANEL_VERSION}Choose an appropriate version compatible with Grafana 11.4.0 and replace
1.3.0above. Consider also pinningnatel-discrete-panelandgrafana-worldmap-panelon lines 14–15 using the same pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/monitoring/images/grafana/Dockerfile` at line 16, The RUN that installs marcusolsson-dynamictext-panel is unpinned, making builds non-deterministic; change the command to use the grafana-cli plugins install <plugin-id> <version> syntax and specify an explicit version compatible with Grafana 11.4.0 for "marcusolsson-dynamictext-panel" (replace the current unpinned invocation with the same plugin-id plus the chosen version), and apply the same pinning pattern to the other plugin installs "natel-discrete-panel" and "grafana-worldmap-panel" so all three use explicit versions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/system/monitoring/images/grafana/Dockerfile`:
- Line 16: The RUN that installs marcusolsson-dynamictext-panel is unpinned,
making builds non-deterministic; change the command to use the grafana-cli
plugins install <plugin-id> <version> syntax and specify an explicit version
compatible with Grafana 11.4.0 for "marcusolsson-dynamictext-panel" (replace the
current unpinned invocation with the same plugin-id plus the chosen version),
and apply the same pinning pattern to the other plugin installs
"natel-discrete-panel" and "grafana-worldmap-panel" so all three use explicit
versions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d74353f5-6cd7-4fe9-9cca-4f3af2e53cc6
📒 Files selected for processing (4)
packages/system/monitoring/images/grafana/Dockerfilepackages/system/monitoring/templates/alerta/alerta.yamlpackages/system/monitoring/templates/vm/vmagent.yamlpackages/system/monitoring/values.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/system/monitoring/templates/vm/vmagent.yaml
- packages/system/monitoring/templates/alerta/alerta.yaml
- packages/system/monitoring/values.yaml
What this PR does
Extends the
monitoringsystem chart with additional optional configuration for the Alerta Slack plugin, a VMAgent environment label, and adds a Grafana plugin to the custom image build.Alerta — Slack plugin (
alerta.alerts.slack)dashboardUrlDASHBOARD_URLsummaryFmtSLACK_SUMMARY_FMTAll new fields are optional and default to empty, preserving existing behavior when unset.
VMAgent — external labels (
vmagent.externalLabels)environmentlabel that gets attached to all scraped metrics (e.g.Staging,Production)"Production"Grafana — custom image
marcusolsson-dynamictext-panelplugin to the Grafana image buildAdded marcusolsson-dynamictext-panel plugin to the Grafana image build
Release note
Summary by CodeRabbit