feat(monitoring): ship Grafana dashboards as pod-scoped ConfigMaps

[christopherfickess]

Summary

Proof-of-concept (draft) for SRE review — the "monitoring with Grafana dashboards" variant.

Builds on #466 (rules + ServiceMonitor) and adds the Grafana dashboard layer, so the two branches differ only by this feature — letting SRE weigh adopting dashboards separately from rules/ServiceMonitor.

• Adds spec.monitoring.grafanaDashboard (enable + sidecar discovery label/value) and the reconcile path.
• Ships all nine Mattermost dashboards from mattermost-performance-assets, transformed for Kubernetes by hack/transform_dashboards.py:
  • Datasource normalization — collapse the per-export ${DS_*} inputs onto one templated datasource variable and drop __inputs/__requires, so the Grafana sidecar provisions them without an import prompt.
  • instance → pod — the four server dashboards now select a stable pod (scoped by a new namespace variable) instead of the churning instance IP:port (123 instance=~"$server" filters rewritten to pod=~"$server").
• One ConfigMap per dashboard (largest ~115 KB) to stay under the 1 MB ConfigMap limit and match the sidecar's one-file-per-ConfigMap convention.
• Cleanup/prune — each dashboard ConfigMap carries a stable marker label; disabling the flag deletes them all, and reconcile prunes ones no longer in the embedded set (rename/removal), independent of the configurable discovery label.

Validation: unit tests; an envtest reconcile test asserting the ConfigMaps are created and deleted; and a live kind + kube-prometheus-stack run confirming the stock Grafana sidecar imports and provisions all nine dashboards (/api/search).

Dashboard-only delta vs the base branch:

git diff feat/operator-monitoring-poc..feat/operator-monitoring-poc-grafana

Note: the Calls dashboard's rtcd/offloader/node-exporter (:9100) panels intentionally remain instance-based — they depend on those separate exporters. Client-app dashboards (web/mobile/desktop) need clientMetrics enabled to have data.

Ticket Link

Internal SRE monitoring POC — no public ticket. Companion to #466.

Release Note

Add `spec.monitoring.grafanaDashboard` to the Mattermost CRD: ships the Mattermost Grafana dashboards as pod-scoped ConfigMaps (one per dashboard) for pickup by the Grafana dashboard sidecar.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Adds opt-in Mattermost monitoring resources and Grafana dashboard provisioning.

Changes:

• Adds ServiceMonitor, PrometheusRule, client/calls metrics, and Grafana dashboard APIs.
• Embeds and transforms nine dashboards into sidecar-discoverable ConfigMaps.
• Adds reconciliation, cleanup, RBAC, documentation, and envtest coverage.

Reviewed changes

Copilot reviewed 24 out of 30 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
Makefile	Adds envtest target
go.mod	Adds monitoring dependencies
go.sum	Updates dependency checksums
main.go	Registers monitoring types
apis/mattermost/v1beta1/mattermost_types.go	Defines monitoring API
apis/mattermost/v1beta1/zz_generated.deepcopy.go	Adds generated deep copies
config/crd/bases/installation.mattermost.com_mattermosts.yaml	Publishes monitoring schema
config/rbac/role.yaml	Grants monitoring-resource access
controllers/mattermost/mattermost/controller.go	Adds recorder and ConfigMap watch
controllers/mattermost/mattermost/mattermost.go	Invokes monitoring reconciliation
controllers/mattermost/mattermost/monitoring.go	Reconciles monitoring resources
controllers/mattermost/mattermost/monitoring_envtest_test.go	Tests create/delete lifecycle
docs/examples/mattermost_monitoring.yaml	Documents monitoring configuration
hack/transform_dashboards.py	Transforms upstream dashboards
pkg/mattermost/mattermost_v1beta.go	Configures client metrics
pkg/mattermost/monitoring.go	Generates monitoring resources
pkg/mattermost/monitoring_test.go	Tests resource generation
pkg/mattermost/prometheusrules/mattermost.yaml	Defines alerting rules
pkg/resources/create_resources.go	Adds monitoring CRUD helpers
test/envtest/cel_validation_test.go	Tests CEL validation
pkg/mattermost/dashboards/mattermost-calls-performance-monitoring.json	Adds Calls dashboard
pkg/mattermost/dashboards/mattermost-collapsed_reply_threads_performance.json	Adds thread-performance dashboard
pkg/mattermost/dashboards/mattermost-desktop-app-metrics.json	Adds desktop dashboard
pkg/mattermost/dashboards/mattermost-mobile-performance.json	Adds mobile dashboard
pkg/mattermost/dashboards/mattermost-notification-metrics.json	Adds notification dashboard
pkg/mattermost/dashboards/mattermost-performance-kpi-metrics_rev2.json	Adds KPI dashboard
pkg/mattermost/dashboards/mattermost-performance-monitoring-bonus-metrics_rev2.json	Adds bonus metrics dashboard
pkg/mattermost/dashboards/mattermost-performance-monitoring-v2.json	Adds server-performance dashboard
pkg/mattermost/dashboards/mattermost-web-app-metrics.json	Adds web dashboard

Files not reviewed (2)

• apis/mattermost/v1beta1/zz_generated.deepcopy.go: Generated file
• apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file

Suppressed comments (3)

pkg/mattermost/monitoring.go:184

• In the calls-only path, this labels map aliases Spec.ResourceLabels; merging discovery labels into it leaks ServiceMonitor-specific labels onto resources generated later in the reconcile. Clone the base labels before applying the rtcd discovery labels.

	labels := mattermost.MattermostLabels(mattermost.Name)

pkg/mattermost/monitoring.go:258

• This map aliases Spec.ResourceLabels, so adding the Grafana discovery/prune labels mutates the CR object and causes those ConfigMap-only labels to be copied onto the subsequently generated Deployment and pods. Changing dashboard discovery settings can therefore cause an unrelated workload rollout. Clone the base labels for each ConfigMap before modifying them.

		labels := mattermost.MattermostLabels(mattermost.Name)
		// The discovery label is what the Grafana sidecar watches for.
		labels[discoveryLabel] = discoveryValue
		// Stable marker (independent of the discovery label) for pruning.
		labels[dashboardConfigMapLabel] = mattermost.Name

pkg/mattermost/monitoring.go:330

• When only PrometheusRule is enabled, labels aliases Spec.ResourceLabels, and the rule-selector labels merged below leak into the Deployment/pod labels generated later. Clone the base label map before adding resource-specific labels.

	labels := mattermost.MattermostLabels(mattermost.Name)

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[christopherfickess]

Thanks for the review. This PR builds on #466; the shared findings (service-label scoping, MattermostLabels map copy, license-warning scope, openapi regen, gofmt, CEL removal for K8s v1.22, server-side apply for the oversized CRD) are addressed there and included here.

Dashboard-specific:

• Dashboard ConfigMaps carry a stable marker label (mattermost.com/grafana-dashboard) independent of the configurable discovery label, and cleanup/prune lists by that marker — so only operator-owned ConfigMaps are deleted.
• Removed the now-obsolete CEL envtest (the callsMetrics CEL validation was dropped for CI-kind compatibility); the reconcile envtest remains.

Deferred (POC): the up-with-no-endpoints edge case and interval/label pattern validation — noted as follow-ups.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 32 changed files in this pull request and generated 1 comment.

Files not reviewed (2)

• apis/mattermost/v1beta1/zz_generated.deepcopy.go: Generated file
• apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file

Suppressed comments (2)

controllers/mattermost/mattermost/monitoring.go:147

• rtcdServiceSelector is documented as required when calls metrics are enabled, but the CRD accepts an empty selector and this branch reports a successful reconcile without creating the requested ServiceMonitor. This also contradicts the stated runtime enforcement after CEL validation was removed; return a configuration error here (or add compatible API validation) instead of silently skipping it.

	desired := mattermostApp.GenerateRtcdServiceMonitorV1Beta(mattermost)
	if desired == nil {
		reqLogger.Info("callsMetrics is enabled but rtcdServiceSelector is empty; skipping rtcd ServiceMonitor")
		return nil

controllers/mattermost/mattermost/controller.go:75

• Omitting these watches means deletion or external modification of an owned ServiceMonitor or PrometheusRule never enqueues its Mattermost owner, so after the core resources become stable either object can remain missing/stale indefinitely. Keep startup safe by conditionally adding both watches when their REST mappings exist (and document that installing the CRDs later requires a controller restart).

		// NOTE: ServiceMonitor is intentionally NOT added to Owns(). A watch on a
		// type whose CRD is absent would fail the manager at startup; the Prometheus
		// Operator CRDs are an optional dependency. GC still works via owner references.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 32 changed files in this pull request and generated 5 comments.

Files not reviewed (2)

• apis/mattermost/v1beta1/zz_generated.deepcopy.go: Generated file
• apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file

Suppressed comments (3)

pkg/resources/create_resources.go:222

• Deletion is based only on the generated rule name, so a Mattermost using another monitoring capability can delete an unrelated pre-existing <name>-rules PrometheusRule while this flag is off. Require the Mattermost owner and verify metav1.IsControlledBy before deleting.
  controllers/mattermost/mattermost/monitoring.go:200
• A pre-existing <name>-metrics Service is overwritten here without checking whether this Mattermost controls it. Reject the name collision before updating, as the ConfigMap path already does.

	current := &corev1.Service{}
	if err := r.Client.Get(context.TODO(), types.NamespacedName{Name: desired.Name, Namespace: desired.Namespace}, current); err != nil {
		return errors.Wrap(err, "failed to fetch current metrics service")
	}

controllers/mattermost/mattermost/monitoring.go:202

• Kubernetes populates spec.clusterIPs (including ["None"] for this headless Service) after creation, but newly generated desired objects leave it empty. Any later reconcile that needs to update labels or other fields sends an object that clears this immutable field and fails; preserve the API-assigned value first, as the existing Service reconcile does.

	return r.Resources.Update(current, desired, reqLogger)

[Copilot]

Pull request overview

Copilot reviewed 26 out of 32 changed files in this pull request and generated 1 comment.

Files not reviewed (2)

• apis/mattermost/v1beta1/zz_generated.deepcopy.go: Generated file
• apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file

Suppressed comments (3)

controllers/mattermost/mattermost/monitoring.go:160

• This successfully reconciles an invalid callsMetrics configuration even though the API declares the selector required. More importantly, if a selector is removed after a ServiceMonitor was created, this path leaves the old ServiceMonitor and its stale selector active indefinitely. Return a configuration error here (or explicitly delete the old resource) instead of silently skipping it.

	if desired == nil {
		reqLogger.Info("callsMetrics is enabled but rtcdServiceSelector is empty; skipping rtcd ServiceMonitor")
		return nil

Makefile:130

• This target no longer exercises CEL validation—the CEL rule and its envtest were removed—so the comment now overstates what test-envtest validates. Describe only the monitoring reconcile lifecycle covered by monitoring_envtest_test.go.

# Docker). Covers CRD CEL validation and the monitoring reconcile create/delete

controllers/mattermost/mattermost/monitoring.go:318

• anyMonitoringEnabled omits two capabilities that depend on the Enterprise-gated Mattermost metrics: an enabled Grafana dashboard and an explicitly enabled clientMetrics. Consequently, those configurations alone never emit the promised license warning. Include both while continuing to exclude the separately deployed rtcd metrics.

func anyMonitoringEnabled(mattermost *mmv1beta.Mattermost) bool {
	return serviceMonitorEnabled(mattermost) ||
		prometheusRuleEnabled(mattermost)
}

[Copilot]

Pull request overview

Copilot reviewed 26 out of 32 changed files in this pull request and generated no new comments.

Files not reviewed (2)

• apis/mattermost/v1beta1/zz_generated.deepcopy.go: Generated file
• apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file

Suppressed comments (3)

pkg/mattermost/monitoring.go:253

• This exported generator dereferences Spec.Monitoring without the nil guard used by the other monitoring generators. Calling it for a CR with no monitoring block panics; the shared nil-monitoring test also omits this generator. Guard Spec.Monitoring before reading GrafanaDashboard.

	if gd := mattermost.Spec.Monitoring.GrafanaDashboard; gd != nil {
		if gd.DiscoveryLabel != "" {
			discoveryLabel = gd.DiscoveryLabel
		}
		if gd.DiscoveryLabelValue != "" {
			discoveryValue = gd.DiscoveryLabelValue
		}
	}

pkg/mattermost/monitoring.go:377

• The CR name is inserted into a PromQL regex without escaping. Kubernetes object names may contain ., which is a regex wildcard, so a Mattermost named mm.prod can make these resource alerts match pods such as mmXprod-* in the same namespace. Quote the name with regexp.QuoteMeta before appending the pod suffix pattern.

		rulePlaceholderPodSelector: mattermost.Name + "-[^-]+-[^-]+",

controllers/mattermost/mattermost/monitoring.go:326

• This predicate claims to cover every Enterprise-gated monitoring capability, but it omits Grafana dashboards and explicitly enabled client metrics. With either capability enabled alone, the operator emits no license warning even though the shipped dashboards/client metrics depend on the licensed Mattermost /metrics endpoint. Include grafanaDashboardEnabled and an explicit-true client-metrics check.

func anyMonitoringEnabled(mattermost *mmv1beta.Mattermost) bool {
	return serviceMonitorEnabled(mattermost) ||
		prometheusRuleEnabled(mattermost)
}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bf6e1cb4-c873-4742-84f2-ec17358c64ab

📥 Commits

Reviewing files that changed from the base of the PR and between 0368c43 and ee91616.

📒 Files selected for processing (1)

• docs/mattermost-operator/mattermost-operator.yaml

---

📝 Walkthrough

Walkthrough

Changes

Mattermost monitoring

Layer / File(s)	Summary
Monitoring API and schema apis/mattermost/v1beta1/*, config/crd/bases/..., docs/mattermost-operator/...	Adds optional monitoring settings and generated schema support for ServiceMonitors, PrometheusRules, Grafana dashboards, client metrics, and Calls metrics.
Monitoring resource generation pkg/mattermost/monitoring.go, pkg/resources/create_resources.go, pkg/mattermost/prometheusrules/*	Generates metrics Services, ServiceMonitors, dashboard ConfigMaps, and PrometheusRules. Handles ownership, stale resources, missing Prometheus Operator CRDs, and resource errors.
Dashboard transformation and packaging hack/transform_dashboards.py, pkg/mattermost/dashboards/*, pkg/mattermost/mattermost_v1beta.go	Transforms dashboards, packages Grafana dashboards, and maps explicit client metrics settings to Mattermost environment variables.
Controller reconciliation and validation controllers/mattermost/mattermost/*, config/rbac/role.yaml, main.go, pkg/mattermost/*_test.go	Reconciles monitoring resources, records license warnings, watches owned ConfigMaps, registers Prometheus types, and tests creation, idempotency, and cleanup.
Environment and deployment workflow Makefile, go.mod, docs/examples/mattermost_monitoring.yaml	Adds envtest setup, changes installation and deployment to non-forced server-side apply, updates dependencies, and documents the monitoring configuration.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant MattermostReconciler
  participant MonitoringGenerators
  participant KubernetesAPI
  participant PrometheusOperator
  MattermostReconciler->>MonitoringGenerators: Generate enabled monitoring resources
  MonitoringGenerators->>KubernetesAPI: Reconcile metrics Service and dashboard ConfigMaps
  MonitoringGenerators->>PrometheusOperator: Reconcile ServiceMonitor and PrometheusRule
  MattermostReconciler->>KubernetesAPI: Remove disabled or stale owned resources

Loading

Possibly related PRs

• mattermost/mattermost-operator#466: Adds related monitoring configuration and reconciliation changes.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: shipping Grafana dashboards as pod-scoped ConfigMaps.
Description check	✅ Passed	The description directly explains the Grafana dashboard configuration, transformation, reconciliation, cleanup, and validation changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/operator-monitoring-poc-grafana

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 6

🧹 Nitpick comments (11)

hack/transform_dashboards.py (3)

33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Silence the false positive from Ruff.

Ruff reports S105 for EXPR_DS_TOKEN because the name contains TOKEN. The value is a Grafana datasource placeholder, not a secret. Add a suppression comment so the lint run stays clean.

🔧 Proposed suppression

-EXPR_DS_TOKEN = "${DS_EXPRESSION}"
+EXPR_DS_TOKEN = "${DS_EXPRESSION}"  # noqa: S105 - Grafana datasource placeholder, not a secret

🤖 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 `@hack/transform_dashboards.py` at line 33, Add an inline Ruff S105 suppression
to the EXPR_DS_TOKEN constant declaration, documenting that this Grafana
datasource placeholder is not a secret while leaving its name and value
unchanged.

Source: Linters/SAST tools

---

54-62: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Panel queries stay unscoped by namespace.

podify_server_var scopes the server variable options by $namespace. replace_instance_filter rewrites panel filters to pod=~"$server" only. The panel queries therefore select by pod name across all namespaces. If two namespaces expose Mattermost pods with the same name, the panels mix series from both.

Consider rewriting the filter to include the namespace selector.

♻️ Proposed change to scope panel queries

 def replace_instance_filter(node):
-    """Recursively rewrite instance=~"$server" -> pod=~"$server" in query strings."""
+    """Recursively rewrite instance=~"$server" -> namespace/pod scoped filter."""
     if isinstance(node, dict):
         return {k: replace_instance_filter(v) for k, v in node.items()}
     if isinstance(node, list):
         return [replace_instance_filter(v) for v in node]
     if isinstance(node, str):
-        return node.replace('instance=~"$server"', 'pod=~"$server"')
+        return node.replace(
+            'instance=~"$server"',
+            'namespace="$namespace",pod=~"$server"',
+        )
     return node

The packaged dashboard JSON files must be regenerated after this change.

Also applies to: 102-116

🤖 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 `@hack/transform_dashboards.py` around lines 54 - 62, Update
replace_instance_filter so rewritten panel filters include the $namespace
selector alongside pod=~"$server", ensuring queries remain namespace-scoped.
Preserve the recursive handling of dictionaries, lists, strings, and other
values, and regenerate the packaged dashboard JSON files afterward.

---

119-143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

transform is not idempotent.

Each call prepends a new datasource variable. transform checks for an existing namespace variable but not for an existing datasource variable. If the script runs on an already-transformed directory, the output gains a duplicate variable.

♻️ Proposed guard

-    leading = [datasource_variable()]
+    leading = []
+    if not any(v.get("name") == DATASOURCE_VAR_NAME for v in templating):
+        leading.append(datasource_variable())
     if is_server_dashboard and not any(v.get("name") == "namespace" for v in templating):
         leading.append(namespace_variable())

🤖 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 `@hack/transform_dashboards.py` around lines 119 - 143, The transform function
should avoid duplicating the datasource variable when processing an
already-transformed dashboard. Before prepending the result of
datasource_variable(), check templating for an existing variable named
"datasource" and only add it when absent, while preserving the existing
namespace guard and ordering behavior.

pkg/mattermost/mattermost_v1beta_test.go (1)

1363-1369: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add the remaining nil cases.

GenerateDeploymentV1Beta guards three conditions: Monitoring != nil, ClientMetrics != nil, and ClientMetrics.Enabled != nil. This subtest covers only the third condition. Add cases for a nil Monitoring and a nil ClientMetrics so a future refactor cannot drop a nil check without a test failure.

💚 Proposed additional subtests

+	t.Run("nil Monitoring emits neither variable", func(t *testing.T) {
+		mm := &mmv1beta.Mattermost{Spec: mmv1beta.MattermostSpec{}}
+		_, hasClient := mattermostContainerEnv(t, mm, "MM_METRICSSETTINGS_ENABLECLIENTMETRICS")
+		_, hasNotif := mattermostContainerEnv(t, mm, "MM_METRICSSETTINGS_ENABLENOTIFICATIONMETRICS")
+		assert.False(t, hasClient)
+		assert.False(t, hasNotif)
+	})
+
+	t.Run("nil ClientMetrics emits neither variable", func(t *testing.T) {
+		mm := withClientMetrics(nil)
+		_, hasClient := mattermostContainerEnv(t, mm, "MM_METRICSSETTINGS_ENABLECLIENTMETRICS")
+		_, hasNotif := mattermostContainerEnv(t, mm, "MM_METRICSSETTINGS_ENABLENOTIFICATIONMETRICS")
+		assert.False(t, hasClient)
+		assert.False(t, hasNotif)
+	})
+
 	t.Run("nil Enabled emits neither variable (server defaults preserved)", func(t *testing.T) {

🤖 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 `@pkg/mattermost/mattermost_v1beta_test.go` around lines 1363 - 1369, Add
subtests covering nil Monitoring and nil ClientMetrics in the
GenerateDeploymentV1Beta test suite, alongside the existing nil Enabled case.
Verify each configuration emits neither client nor notification metrics
environment variable, preserving server defaults and guarding both corresponding
nil checks.

pkg/mattermost/dashboards/mattermost-performance-kpi-metrics_rev2.json (1)

630-662: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Legacy dashboard alert blocks are inert.

These alert objects use the schemaVersion 16 dashboard alerting format. Grafana 9 and later removed legacy dashboard alerting, so Grafana ignores these blocks. They also keep the hidden helper targets at lines 720-726 and 858-864 alive for no purpose.

The operator ships these dashboards through the Grafana sidecar, and the PR verifies against kube-prometheus-stack, which runs a recent Grafana. Consider stripping the alert blocks and their helper targets in hack/transform_dashboards.py, then regenerating this file.

Also applies to: 778-810

🤖 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 `@pkg/mattermost/dashboards/mattermost-performance-kpi-metrics_rev2.json`
around lines 630 - 662, Remove the legacy dashboard alert blocks and their
associated hidden helper targets from the dashboard transformation logic in
hack/transform_dashboards.py, including both alert sections represented here.
Regenerate mattermost-performance-kpi-metrics_rev2.json so the resulting
dashboard contains neither obsolete alert objects nor unused helper targets,
while preserving all other dashboard queries and panels.

pkg/mattermost/mattermost_v1beta.go (1)

353-371: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

One field drives two server settings.

ClientMetrics.Enabled sets both MM_METRICSSETTINGS_ENABLECLIENTMETRICS and MM_METRICSSETTINGS_ENABLENOTIFICATIONMETRICS. A user cannot enable client metrics and disable notification metrics.

The nil handling is correct, and mergeEnvVars at line 401 still lets spec.mattermostEnv override both values. Adding a separate field later is an additive API change, so this is safe for the proof of concept. Record the limitation in the field documentation so users understand the coupling.

🤖 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 `@pkg/mattermost/mattermost_v1beta.go` around lines 353 - 371, Document in the
API field documentation for Monitoring.ClientMetrics.Enabled that it controls
both client/RUM metrics and notification performance metrics, so users
understand they cannot configure them independently. Preserve the existing nil
handling and environment-variable generation in the Mattermost deployment logic.

controllers/mattermost/mattermost/monitoring.go (2)

253-263: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

anyMonitoringEnabled omits grafanaDashboard, which contradicts the stated rule.

The doc comment on anyMonitoringEnabled states the function reports whether any capability that depends on the Enterprise-gated /metrics endpoint is on. The Grafana dashboards query the same Mattermost metrics. If a user enables only grafanaDashboard and sets no licenseSecret, the dashboards render empty and no warning event is emitted.

If the omission is deliberate, record the reason in the doc comment. Otherwise include the flag.

♻️ Proposed change

 func anyMonitoringEnabled(mattermost *mmv1beta.Mattermost) bool {
 	return serviceMonitorEnabled(mattermost) ||
-		prometheusRuleEnabled(mattermost)
+		prometheusRuleEnabled(mattermost) ||
+		grafanaDashboardEnabled(mattermost)
 }

Also applies to: 319-326

🤖 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 `@controllers/mattermost/mattermost/monitoring.go` around lines 253 - 263,
Update anyMonitoringEnabled to include the grafanaDashboard setting when
determining whether Enterprise-gated monitoring is enabled, so
warnMonitoringWithoutLicense emits the existing warning and event for
dashboard-only configurations without a license. Keep the current behavior for
all other monitoring flags.

---

128-129: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Centralize monitoring resource names.

Export shared name helpers from pkg/mattermost and use them in generators and cleanup keys. This prevents future suffix changes from causing cleanup no-ops.

🤖 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 `@controllers/mattermost/mattermost/monitoring.go` around lines 128 - 129,
Centralize the Mattermost monitoring resource naming by adding exported name
helpers in pkg/mattermost, then update the monitoring generators and cleanup
keys around metricsSvcKey and smKey to use those helpers instead of constructing
names inline. Ensure all consumers share the same helper-generated names so
future suffix changes remain consistent.

controllers/mattermost/mattermost/monitoring_envtest_test.go (1)

92-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider asserting the exact dashboard count.

GreaterOrEqual(..., 9) passes if the reconcile creates extra ConfigMaps. An exact count also detects duplicate or unpruned dashboards.

♻️ Proposed change

-	assert.GreaterOrEqual(t, len(cms.Items), 9, "expected one ConfigMap per embedded dashboard")
+	assert.Len(t, cms.Items, 9, "expected exactly one ConfigMap per embedded dashboard")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@controllers/mattermost/mattermost/monitoring_envtest_test.go` at line 92,
Update the assertion in the monitoring reconciliation test to require exactly 9
ConfigMaps, replacing the lower-bound check while preserving the existing
failure message and dashboard-count expectation.

Makefile (2)

258-258: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Use an explicit dry-run strategy

The bare --dry-run form remains supported, but it is deprecated. Use --dry-run=client -o yaml for forward compatibility.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` at line 258, Update the kubectl namespace creation command to use
the explicit client-side dry-run mode with YAML output, replacing the deprecated
bare --dry-run form while preserving the existing apply pipeline.

---

136-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pin setup-envtest to the v0.21.0 commit.

The nested module does not publish a v0.21.0 tag. Use v0.0.0-20250520071515-71f7db556ca5, which resolves to the controller-runtime v0.21.0 commit, instead of latest.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` around lines 136 - 137, Update the $(SETUP_ENVTEST) Makefile target
to install setup-envtest at version v0.0.0-20250520071515-71f7db556ca5 instead
of latest, preserving the existing GOBIN and GO_INSTALL invocation.

🤖 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 `@controllers/mattermost/mattermost/mattermost.go`:
- Around line 64-68: Reorder the checkMattermost flow so
checkMattermostDeployment runs before checkMattermostMonitoring. Keep monitoring
errors returned from checkMattermost after the deployment reconciliation,
ensuring monitoring remains reported while failures in the optional add-on do
not block the primary Mattermost workload from converging.

In
`@pkg/mattermost/dashboards/mattermost-collapsed_reply_threads_performance.json`:
- Around line 30-83: Update the dashboard transformation flow in
hack/transform_dashboards.py, specifically replace_ds_strings, to add {"type":
"prometheus", "uid": "${datasource}"} when panels or targets lack a datasource.
Regenerate the dashboard so panels with ids 2 and 4 and their targets honor the
datasource variable consistently with panels 6 and 8.

In `@pkg/mattermost/dashboards/mattermost-mobile-performance.json`:
- Around line 1981-2000: Replace the unresolved ${VAR_RATE} value and text in
the rate constant with a concrete PromQL duration, then regenerate the dashboard
so all $rate targets use that valid duration. Also update the __inputs handling
in transform_dashboards.py to resolve constant defaults before removing the
inputs metadata, preventing future exports from retaining unresolved
placeholders.

In `@pkg/mattermost/monitoring.go`:
- Around line 242-246: Update GenerateGrafanaDashboardConfigMapsV1Beta to check
mattermost.Spec.Monitoring for nil before accessing GrafanaDashboard, returning
nil, nil when absent. Extend TestGeneratorsNilMonitoring to cover this behavior.
- Around line 364-379: The pod selector built in rulePlaceholders must escape
mattermost.Name before embedding it in the PromQL regular expression. Use
regexp.QuoteMeta for the name while preserving the existing suffix pattern, and
add coverage for a dotted Mattermost name to ensure literal dots do not match
arbitrary characters.
- Around line 262-266: Validate the configured DiscoveryLabel before generating
ConfigMaps, rejecting it when it equals the reserved dashboardConfigMapLabel
value "mattermost.com/grafana-dashboard". Add this guard in the monitoring flow
before the label assignments around MattermostLabels, and return the existing
error type or propagation path so ConfigMap generation does not proceed with the
conflicting label.

---

Nitpick comments:
In `@controllers/mattermost/mattermost/monitoring_envtest_test.go`:
- Line 92: Update the assertion in the monitoring reconciliation test to require
exactly 9 ConfigMaps, replacing the lower-bound check while preserving the
existing failure message and dashboard-count expectation.

In `@controllers/mattermost/mattermost/monitoring.go`:
- Around line 253-263: Update anyMonitoringEnabled to include the
grafanaDashboard setting when determining whether Enterprise-gated monitoring is
enabled, so warnMonitoringWithoutLicense emits the existing warning and event
for dashboard-only configurations without a license. Keep the current behavior
for all other monitoring flags.
- Around line 128-129: Centralize the Mattermost monitoring resource naming by
adding exported name helpers in pkg/mattermost, then update the monitoring
generators and cleanup keys around metricsSvcKey and smKey to use those helpers
instead of constructing names inline. Ensure all consumers share the same
helper-generated names so future suffix changes remain consistent.

In `@hack/transform_dashboards.py`:
- Line 33: Add an inline Ruff S105 suppression to the EXPR_DS_TOKEN constant
declaration, documenting that this Grafana datasource placeholder is not a
secret while leaving its name and value unchanged.
- Around line 54-62: Update replace_instance_filter so rewritten panel filters
include the $namespace selector alongside pod=~"$server", ensuring queries
remain namespace-scoped. Preserve the recursive handling of dictionaries, lists,
strings, and other values, and regenerate the packaged dashboard JSON files
afterward.
- Around line 119-143: The transform function should avoid duplicating the
datasource variable when processing an already-transformed dashboard. Before
prepending the result of datasource_variable(), check templating for an existing
variable named "datasource" and only add it when absent, while preserving the
existing namespace guard and ordering behavior.

In `@Makefile`:
- Line 258: Update the kubectl namespace creation command to use the explicit
client-side dry-run mode with YAML output, replacing the deprecated bare
--dry-run form while preserving the existing apply pipeline.
- Around line 136-137: Update the $(SETUP_ENVTEST) Makefile target to install
setup-envtest at version v0.0.0-20250520071515-71f7db556ca5 instead of latest,
preserving the existing GOBIN and GO_INSTALL invocation.

In `@pkg/mattermost/dashboards/mattermost-performance-kpi-metrics_rev2.json`:
- Around line 630-662: Remove the legacy dashboard alert blocks and their
associated hidden helper targets from the dashboard transformation logic in
hack/transform_dashboards.py, including both alert sections represented here.
Regenerate mattermost-performance-kpi-metrics_rev2.json so the resulting
dashboard contains neither obsolete alert objects nor unused helper targets,
while preserving all other dashboard queries and panels.

In `@pkg/mattermost/mattermost_v1beta_test.go`:
- Around line 1363-1369: Add subtests covering nil Monitoring and nil
ClientMetrics in the GenerateDeploymentV1Beta test suite, alongside the existing
nil Enabled case. Verify each configuration emits neither client nor
notification metrics environment variable, preserving server defaults and
guarding both corresponding nil checks.

In `@pkg/mattermost/mattermost_v1beta.go`:
- Around line 353-371: Document in the API field documentation for
Monitoring.ClientMetrics.Enabled that it controls both client/RUM metrics and
notification performance metrics, so users understand they cannot configure them
independently. Preserve the existing nil handling and environment-variable
generation in the Mattermost deployment logic.

🪄 Autofix

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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7fef7de4-252b-4d82-8ba3-5ac38200a417

📥 Commits

Reviewing files that changed from the base of the PR and between 8ef3cfa and 0ec5749.

⛔ Files ignored due to path filters (1)

• go.sum is excluded by !**/*.sum

📒 Files selected for processing (31)

• Makefile
• apis/mattermost/v1beta1/mattermost_types.go
• apis/mattermost/v1beta1/mattermost_utils.go
• apis/mattermost/v1beta1/zz_generated.deepcopy.go
• apis/mattermost/v1beta1/zz_generated.openapi.go
• config/crd/bases/installation.mattermost.com_mattermosts.yaml
• config/rbac/role.yaml
• controllers/mattermost/mattermost/controller.go
• controllers/mattermost/mattermost/mattermost.go
• controllers/mattermost/mattermost/monitoring.go
• controllers/mattermost/mattermost/monitoring_envtest_test.go
• controllers/mattermost/mattermost/monitoring_reconcile_test.go
• docs/examples/mattermost_monitoring.yaml
• go.mod
• hack/transform_dashboards.py
• main.go
• pkg/mattermost/dashboards/mattermost-calls-performance-monitoring.json
• pkg/mattermost/dashboards/mattermost-collapsed_reply_threads_performance.json
• pkg/mattermost/dashboards/mattermost-desktop-app-metrics.json
• pkg/mattermost/dashboards/mattermost-mobile-performance.json
• pkg/mattermost/dashboards/mattermost-notification-metrics.json
• pkg/mattermost/dashboards/mattermost-performance-kpi-metrics_rev2.json
• pkg/mattermost/dashboards/mattermost-performance-monitoring-bonus-metrics_rev2.json
• pkg/mattermost/dashboards/mattermost-performance-monitoring-v2.json
• pkg/mattermost/dashboards/mattermost-web-app-metrics.json
• pkg/mattermost/mattermost_v1beta.go
• pkg/mattermost/mattermost_v1beta_test.go
• pkg/mattermost/monitoring.go
• pkg/mattermost/monitoring_test.go
• pkg/mattermost/prometheusrules/mattermost.yaml
• pkg/resources/create_resources.go

[Copilot]

Pull request overview

Copilot reviewed 27 out of 33 changed files in this pull request and generated no new comments.

Files not reviewed (2)

• apis/mattermost/v1beta1/zz_generated.deepcopy.go: Generated file
• apis/mattermost/v1beta1/zz_generated.openapi.go: Generated file

Suppressed comments (4)

controllers/mattermost/mattermost/monitoring.go:163

• rtcdServiceSelector is documented as required when Calls metrics is enabled, but this path treats the invalid configuration as a successful reconcile and silently creates no ServiceMonitor. That leaves the CR appearing healthy while the requested capability is absent. After cleaning up any previous object, return an error (or reject the configuration at admission) so the requirement is actually enforced.

	desired := mattermostApp.GenerateRtcdServiceMonitorV1Beta(mattermost)
	if desired == nil {
		// callsMetrics is enabled but the selector was cleared: tear down any rtcd
		// ServiceMonitor we previously created rather than leaving it orphaned.
		reqLogger.Info("callsMetrics is enabled but rtcdServiceSelector is empty; removing any owned rtcd ServiceMonitor")
		return r.deleteOwnedResource(mattermost, rtcdKey, &monitoringv1.ServiceMonitor{}, reqLogger)

controllers/mattermost/mattermost/monitoring.go:326

• A dashboard-only configuration is omitted from the Enterprise-license warning even though every shipped Mattermost dashboard depends on the license-gated /metrics endpoint. With grafanaDashboard.enabled: true and no ServiceMonitor or rule, the operator provisions dashboards that have no Mattermost data without emitting the warning promised by this helper.

func anyMonitoringEnabled(mattermost *mmv1beta.Mattermost) bool {
	return serviceMonitorEnabled(mattermost) ||
		prometheusRuleEnabled(mattermost)

Makefile:130

• This target no longer runs any CEL validation: the CEL rule/test was removed, and the command now filters only the controller monitoring tests. Keeping this claim makes the target's validation scope misleading.

# Fast local validation with envtest (a real apiserver+etcd from binaries — no
# Docker). Covers CRD CEL validation and the monitoring reconcile create/delete
# lifecycle. Downloads the apiserver binaries on first run.

hack/transform_dashboards.py:272

• The transform only overwrites dashboards present in src_dir; it never removes JSON files that disappeared or were renamed upstream. Re-running it into the embedded dashboard directory therefore leaves stale dashboards in the binary, so the reconcile-time prune cannot observe the removal. Make the destination a mirror of the source set before writing outputs (while preserving safe behavior when source and destination are the same directory).

    src, dst = Path(sys.argv[1]), Path(sys.argv[2])
    dst.mkdir(parents=True, exist_ok=True)

    for path in sorted(src.glob("*.json")):