[monitoring] Improve tenant metrics collection - #1684
Conversation
This patch introduces vmagent support inside tenant-level monitoring stacks, ensuring that all tenant workloads have consistent and reliable metrics scraping. By deploying a vmagent per tenant and wiring it to the configured VictoriaMetrics short- and long-term storage, metrics are now properly collected, labeled, and remote-written. This resolves gaps where tenant environments previously missed metrics due to the absence of dedicated agents. ```release-note [monitoring,platform] Add vmagent integration into tenant monitoring stacks to ensure complete and correct metrics collection for all tenants. ``` Signed-off-by: IvanHunters <[email protected]>
dosubot
Bot
added
the
size/M
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds VMAgent support to the monitoring Helm chart: documentation, values schema and defaults, a conditional Helm template to deploy VMAgent, and ordering for vmagent keys in the monitoring CRD presentation. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (Helm values)
participant Helm as Helm chart renderer
participant K8s as Kubernetes API / Controller
participant VMA as VMAgent Pod
participant VMinsert as vminsert (remoteWrite endpoint)
Dev->>Helm: Add vmagent values (externalLabels, remoteWrite.urls)
Helm->>K8s: Render & apply VMAgent manifest (conditional on namespace)
K8s->>VMA: Start VMAgent with configured args & externalLabels
VMA->>VMinsert: Remote write metrics to configured URLs (looped)
VMA-->>K8s: Select PodScrapes/ServiceScrapes in release namespace
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
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 |
Summary of ChangesHello IvanHunters, 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 significantly enhances the monitoring capabilities for tenant environments by integrating Highlights
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 (@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
|
dosubot
Bot
added
the
kind/feature
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces vmagent support for tenant-level monitoring, which is a great improvement for consistent metrics collection. The implementation looks solid, with correct use of selectors for multi-tenancy.
My main feedback is regarding resource management for the new VMAgent component. I've suggested adding configurable resource requests and limits to ensure stability in production environments. I've also made a minor suggestion to improve the robustness of the Helm template by quoting a value.
Overall, this is a valuable addition to the monitoring stack.
| labels: | ||
| policy.cozystack.io/allow-to-apiserver: "true" | ||
| shardCount: 1 | ||
| externalLabels: |
There was a problem hiding this comment.
The VMAgent is being created without any resource requests or limits. This is not recommended for production environments as it can lead to performance issues or the pod being evicted. Please add a resources section to the VMAgent spec.
You should also add a corresponding resources field to vmagent in packages/extra/monitoring/values.yaml and packages/extra/monitoring/values.schema.json to make it configurable. A VPA configuration in packages/extra/monitoring/templates/vpa.yaml would also be consistent with other components.
{{- with .Values.vmagent.resources }}
resources:
{{- toYaml . | nindent 4 }}
{{- end }}
externalLabels:| policy.cozystack.io/allow-to-apiserver: "true" | ||
| shardCount: 1 | ||
| externalLabels: | ||
| cluster: {{ .Values.vmagent.externalLabels.cluster }} |
There was a problem hiding this comment.
The cluster name from .Values.vmagent.externalLabels.cluster is used without quotes. While cluster names are usually simple strings, it's a good practice to quote them to prevent potential YAML parsing issues if the value contains special characters.
cluster: {{ .Values.vmagent.externalLabels.cluster | quote }}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/extra/monitoring/templates/vm/vmagent.yaml (1)
10-23: Consider making vmagent configuration parameters customizable.Several settings are currently hardcoded without corresponding configuration options in values:
shardCount: 1(line 10)scrapeInterval: 30s(line 22)promscrape.maxScrapeSize: "32MB"(line 15)promscrape.streamParse: "true"(line 16)selectAllByDefault: false(line 23)While these defaults may be appropriate for tenant-level monitoring, consider whether operators should be able to override these settings per tenant or globally. This would improve flexibility for future use cases.
Would you like me to generate an updated values.yaml and template that exposes these as configurable parameters with sensible defaults?
packages/extra/monitoring/values.yaml (1)
161-162: Add field documentation to VmagentRemoteWrite typedef.The
VmagentRemoteWritetypedef on line 161 is declared as a struct but lacks documentation for theurlsfield, which is present in the actual values configuration (line 175-178) and referenced in the template (line 18).Add the
@fieldannotation to document theurlsfield:## @typedef {struct} VmagentRemoteWrite - Remote write configuration. +## @field {VmagentRemoteWriteURLs} urls - List of remote write endpoint URLs. ## @typedef {stringSlice} VmagentRemoteWriteURLs - List of remoteWrite endpoint URLs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/extra/monitoring/README.md(1 hunks)packages/extra/monitoring/templates/vm/vmagent.yaml(1 hunks)packages/extra/monitoring/values.schema.json(1 hunks)packages/extra/monitoring/values.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-26T18:26:01.447Z
Learnt from: NickVolynkin
Repo: cozystack/cozystack PR: 1216
File: packages/extra/monitoring/README.md:30-31
Timestamp: 2025-07-26T18:26:01.447Z
Learning: The cozyvalues-gen tool has a known issue (https://github.com/cozystack/cozyvalues-gen/issues/10) where it incorrectly places Grafana configuration entries under the wrong documentation sections, specifically placing grafana.resources.requests.* entries in the Metrics storage configuration table instead of the Grafana configuration table.
Applied to files:
packages/extra/monitoring/values.yaml
🪛 YAMLlint (1.37.1)
packages/extra/monitoring/templates/vm/vmagent.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
packages/extra/monitoring/templates/vm/vmagent.yaml (2)
1-1: YAMLlint error is a false positive; YAML validators don't recognize Helm template syntax.The static analysis tool reported a syntax error on this line, but this is expected behavior—YAML validators cannot parse Helm's
{{- if }}syntax. This is not a blocker.
7-9: Thepolicy.cozystack.io/allow-to-apiserverlabel is recognized and enforced by the platform's policy system. A CiliumNetworkPolicy inpackages/apps/tenant/templates/networkpolicy.yamlexplicitly matches this label viaendpointSelector.matchLabelsand grants egress access to the Kubernetes API server (port 6443). The label is consistently applied across 30+ resources throughout the codebase as a standard pattern for components requiring API server access. The vmagent pod configuration is correct.
| podScrapeNamespaceSelector: | ||
| matchLabels: | ||
| namespace.cozystack.io/monitoring: {{ .Release.Namespace }} | ||
| serviceScrapeNamespaceSelector: | ||
| matchLabels: | ||
| namespace.cozystack.io/monitoring: {{ .Release.Namespace }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for usage of the namespace.cozystack.io/monitoring label in the codebase
rg -n "namespace\.cozystack\.io/monitoring" packages/Repository: cozystack/cozystack
Length of output: 739
🏁 Script executed:
#!/bin/bash
# Check the context around each occurrence
echo "=== vmagent.yaml context ==="
sed -n '20,32p' packages/extra/monitoring/templates/vm/vmagent.yaml
echo -e "\n=== apps.yaml context (line 26) ==="
sed -n '20,35p' packages/core/platform/templates/apps.yaml
echo -e "\n=== vertical-pod-autoscaler.yaml context ==="
head -n 15 packages/apps/kubernetes/templates/helmreleases/vertical-pod-autoscaler.yaml
echo -e "\n=== monitoring-agents.yaml context ==="
head -n 15 packages/apps/kubernetes/templates/helmreleases/monitoring-agents.yamlRepository: cozystack/cozystack
Length of output: 2112
Fix namespace selector mismatch: labels vs. annotations.
The podScrapeNamespaceSelector and serviceScrapeNamespaceSelector use matchLabels to find namespaces by namespace.cozystack.io/monitoring, but the codebase sets this value only as a namespace annotation (see packages/core/platform/templates/apps.yaml:26), not as a label. The selectors will not match any namespaces. Either add the label to namespace creation, or change the selectors to use matchExpressions with metadata.annotations.
🤖 Prompt for AI Agents
In packages/extra/monitoring/templates/vm/vmagent.yaml around lines 24-29 the
namespace selectors use matchLabels for namespace.cozystack.io/monitoring but
namespaces are created with that value as an annotation, so selectors never
match; fix by either updating the namespace creation
(packages/core/platform/templates/apps.yaml) to add the same key as a label on
the namespace, or change podScrapeNamespaceSelector and
serviceScrapeNamespaceSelector to use matchExpressions that target the namespace
annotation (i.e., match the metadata annotation key
namespace.cozystack.io/monitoring with the release namespace value) so the
selectors align with how the namespace is annotated.
| "externalLabels": { | ||
| "description": "External labels applied to all metrics.", | ||
| "default": { | ||
| "cluster": "cozystack" | ||
| } | ||
| }, | ||
| "remoteWrite": { | ||
| "description": "Remote write configuration.", | ||
| "default": { | ||
| "urls": [ | ||
| "http://vminsert-shortterm:8480/insert/0/prometheus", | ||
| "http://vminsert-longterm:8480/insert/0/prometheus" | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add explicit type definitions to vmagent schema properties.
The externalLabels and remoteWrite properties are missing explicit type declarations. While defaults are provided, JSON Schema best practices and validation clarity require explicit types.
Apply this diff to add proper type definitions:
"externalLabels": {
"description": "External labels applied to all metrics.",
+ "type": "object",
"default": {
"cluster": "cozystack"
}
},
"remoteWrite": {
"description": "Remote write configuration.",
+ "type": "object",
"default": {
"urls": [
"http://vminsert-shortterm:8480/insert/0/prometheus",
"http://vminsert-longterm:8480/insert/0/prometheus"
]
}
}Additionally, consider defining the nested urls property explicitly as an array of strings:
"remoteWrite": {
"description": "Remote write configuration.",
"type": "object",
+ "properties": {
+ "urls": {
+ "description": "List of remote write endpoint URLs.",
+ "type": "array",
+ "items": {
+ "type": "string"
+ }
+ }
+ }
"default": { ... }
}📝 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.
| "externalLabels": { | |
| "description": "External labels applied to all metrics.", | |
| "default": { | |
| "cluster": "cozystack" | |
| } | |
| }, | |
| "remoteWrite": { | |
| "description": "Remote write configuration.", | |
| "default": { | |
| "urls": [ | |
| "http://vminsert-shortterm:8480/insert/0/prometheus", | |
| "http://vminsert-longterm:8480/insert/0/prometheus" | |
| ] | |
| } | |
| } | |
| "externalLabels": { | |
| "description": "External labels applied to all metrics.", | |
| "type": "object", | |
| "default": { | |
| "cluster": "cozystack" | |
| } | |
| }, | |
| "remoteWrite": { | |
| "description": "Remote write configuration.", | |
| "type": "object", | |
| "properties": { | |
| "urls": { | |
| "description": "List of remote write endpoint URLs.", | |
| "type": "array", | |
| "items": { | |
| "type": "string" | |
| } | |
| } | |
| }, | |
| "default": { | |
| "urls": [ | |
| "http://vminsert-shortterm:8480/insert/0/prometheus", | |
| "http://vminsert-longterm:8480/insert/0/prometheus" | |
| ] | |
| } | |
| } |
Signed-off-by: IvanHunters <[email protected]>
3 participants
This patch introduces vmagent support inside tenant-level monitoring stacks, ensuring that all tenant workloads have consistent and reliable metrics scraping. By deploying a vmagent per tenant and wiring it to the configured VictoriaMetrics short- and long-term storage, metrics are now properly collected, labeled, and remote-written. This resolves gaps where tenant environments previously missed metrics due to the absence of dedicated agents.
What this PR does
Release note
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.