feat: add metrics support - #170
Conversation
7b70cc1 to
b2725fd
Compare
There was a problem hiding this comment.
Pull request overview
This PR enables Prometheus metrics for the Shopware operator by adding a metrics collection package, wiring it into the reconciliation flow, and exposing the controller-runtime metrics endpoint via configuration and Helm.
Changes:
- Add
internal/metricswith Prometheus gauges for Store state, deployment state/replicas, HPA settings, and scheduled task (CronJob) status. - Update controllers to publish metrics on status updates and clean up metrics on Store deletion.
- Enable controller-runtime metrics server binding via config (
METRICS_BIND_ADDRESS) and add a Helm Service intended to expose the metrics endpoint.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/metrics/metrics.go | New Prometheus metrics definitions + update/remove helpers for Store and CronJob status. |
| internal/controller/store_status.go | Emit metrics during status reconciliation; fetch CronJob for scheduled-task metrics. |
| internal/controller/store_controller.go | Remove metrics when a Store is being deleted. |
| internal/config/config.go | Default metrics bind address changed to :8080. |
| cmd/main.go | Enable controller-runtime metrics server using cfg.MetricsAddr. |
| helm/templates/metrics-service.yaml | New Service intended to expose the metrics endpoint. |
| helm/values.yaml | Add storeAPI.port value (currently tied to metrics Service). |
| go.mod | Promote prometheus/client_golang to a direct dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b2725fd to
839a559
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
402dc9f to
7dc0b90
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7dc0b90 to
ee719d6
Compare
42df9de to
dfa36ae
Compare
8eb590e to
70b7ace
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (9)
internal/config/config.go:67
- Changing the default metrics bind address to
:8080enables the metrics listener even when the Helm chart hasmetrics.enabled: false, which contradicts the chart's opt-in semantics and can unintentionally expose metrics on the pod network. Consider keeping the default disabled ("0") and enabling it via Helm/env when needed.
MetricsAddr string `env:"METRICS_BIND_ADDRESS, default=:8080"`
helm/values.yaml:118
- Same env-var naming mismatch here: the chart docs mention
K8S_OPERATOR_URL, but the operator injectsSHOPWARE_OPERATOR_URL.
# shopwareOperatorUrl overrides the K8S_OPERATOR_URL injected into store
# containers (admin, storefront, worker). When left empty the URL is
internal/metrics/metrics.go:59
UpdateScheduledTaskMetricssetsshopware_store_scheduled_task_suspendedto 0 when the CronJob is nil/not found, but the help string describes 0 as "active". This makes the metric description inaccurate for the "missing CronJob" case.
storeScheduledTaskSuspended = prometheus.NewGaugeVec(prometheus.GaugeOpts{
Name: "shopware_store_scheduled_task_suspended",
Help: "Whether the scheduled task CronJob is suspended (1) or active (0)",
}, []string{"store", "namespace"})
internal/metrics/metrics.go:165
- The code parses both available and desired from
cond.Ready, but onlyavailableis used (desired comes fromcond.StoreReplicas). Passing a throwaway pointer makes the intent clearer and avoids the misleading localdesiredvariable.
// Parse available/desired from the Ready field (format: "available/desired")
var available, desired int
if cond.Ready != "" {
fmtScan(cond.Ready, &available, &desired)
}
internal/metrics/metrics.go:205
- This new metrics package adds non-trivial behavior (state gauges, deployment replica parsing, and CronJob last-run status logic) but there are no accompanying unit tests. The repo already runs
go test ./...(Makefile:89), so adding coverage here would help prevent regressions.
// UpdateScheduledTaskMetrics sets metrics for the scheduled task CronJob.
func UpdateScheduledTaskMetrics(store *v1.Store, cronJob *batchv1.CronJob) {
name := store.Name
ns := store.Namespace
helm/templates/metrics-service.yaml:27
- The Service uses a configurable
.Values.metrics.port, buttargetPortis hardcoded to 8080. If the port is changed, the Service will route to the wrong container port and metrics scraping will fail.
ports:
- name: http-metrics
port: {{ .Values.metrics.port | default 8080 }}
targetPort: 8080
protocol: TCP
helm/templates/deployment.yaml:90
.Values.metrics.portcontrols the exposed container port and the constructed operator URL, but the deployment does not setMETRICS_BIND_ADDRESS. The manager will still bind to the default from code/config, which breaks custom ports and can desync from the Service.
{{- if .Values.metrics.enabled }}
- name: OPERATOR_SERVICE_URL
value: "{{ .Values.metrics.shopwareOperatorUrl | default (printf "http://shopware-operator.%s.svc.cluster.local:%d" .Release.Namespace (.Values.metrics.port | default 8080 | int)) }}"
{{- end }}
internal/config/config.go:72
- The comment says this URL is exposed to store containers as
K8S_OPERATOR_URL, but the code injectsSHOPWARE_OPERATOR_URL(api/v1/store_env.go). This is misleading and should be aligned with the actual env var name.
// OperatorServiceURL is exposed to store containers as K8S_OPERATOR_URL so
// the Shopware consumer knows how to reach the operator service.
OperatorServiceURL string `env:"OPERATOR_SERVICE_URL"`
helm/values.yaml:114
- This comment says
K8S_OPERATOR_URLis injected into store containers, but the operator currently injectsSHOPWARE_OPERATOR_URL(api/v1/store_env.go). Please align the docs/config naming to avoid confusion for chart users.
This issue also appears on line 117 of the same file.
# When enabled, a Service named 'shopware-operator' is created, the metrics
# endpoint is exposed, and K8S_OPERATOR_URL is injected into every store
# container so the Shopware consumer can reach the operator.
7f9acbf to
81426ba
Compare
81426ba to
6884b34
Compare
No description provided.