[prometheus-windows-exporter] Extract the prometheus.io/scrape service annotation

[petewall]

What this PR does / why we need it

This change moves the prometheus.io/scrape service annotation into the values.yaml file where it can be disabled if not desired.

Also add comments in the values.yaml file for the service section.

All of these changes make this match how things look and work in the Node Exporter helm chart.

Which issue this PR fixes

• fixes [prometheus-windows-exporter] Unable to disable prometheus.io/scrape annotation on the service #5129

Special notes for your reviewer

My testing to ensure that things behave correctly:

# Default
helm template test . | yq 'select(.kind == "Service") | .metadata.annotations'
# Result: prometheus.io/scrape: "true"

# Extra annotations
helm template test . --set 'service.annotations.extra=1234' | yq 'select(.kind == "Service") | .metadata.annotations'
# Result: prometheus.io/scrape: "true", extra: "1234"

# Disabling the prometheus.io/scrape annotation
helm template test . --set 'service.annotations.prometheus\.io/scrape=null' | yq 'select(.kind == "Service") | .metadata.annotations'
# Result: null

# ServiceMonitor still removes the prometheus.io/scrape annotation
helm template test . --set 'prometheus.monitor.enabled=true' | yq 'select(.kind == "Service") | .metadata.annotations'
# Result: {}

# PodMonitor still removes the prometheus.io/scrape annotation
helm template test . --set 'prometheus.podMonitor.enabled=true' | yq 'select(.kind == "Service") | .metadata.annotations'
# Result: {}

# PodMonitor with extra works
helm template test . --set 'prometheus.podMonitor.enabled=true' --set 'service.annotations.extra=1234' | yq 'select(.kind == "Service") | .metadata.annotations'
# Result: extra: 1234

Checklist

• DCO signed
• Chart Version bumped
• Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

[jkroepke]

Hi @petewall

I understand this kind of issue and the PR how you try to resolve it. It works in context of this chart, but it wont work, if the chart is embedded as sub chart, like in k8s-monitoring-helm.

There is a known helm issue, that removing the annotation via service.annotations.prometheus\.io/scrape=null does not work. Please keep that in mind and re-evalulate, if the proposed solution is still valid in context of k8s-monitoring-helm

[petewall]

You're correct in that with Helm today, I can't "null" the value to remove it. But I can override it by setting this:

service:
  annotations:
    prometheus.io/scrape: "false"

which will disable the annotation based scraping configurations for prometheus and other scrapers. Many aren't looking for the presence of the annotation, but checking that it is true.

I'm doing that with Node Exporter:
https://github.com/grafana/k8s-monitoring-helm/blob/main/charts/k8s-monitoring/charts/feature-cluster-metrics/values.yaml#L549-L551

Which is setting the value to false:
https://github.com/grafana/k8s-monitoring-helm/blob/main/charts/k8s-monitoring/docs/examples/features/cluster-metrics/default/output.yaml#L2556-L2557

When Helm makes it so we can remove values in subcharts, then i'll go that route, but this is a workable solution for now.

[jkroepke]

Please go ahead with annotations: {} (by default, no prometheus.io/scrape annotation is set) It change might be breaky, but it's a cleaner solution at the end which i favorite. Please increase the minor version in that case.

[petewall]

Please go ahead with annotations: {} (by default, no prometheus.io/scrape annotation is set

As in, the values file remains:

service:
    annotations: {}

And the service definition just applies those annotations? Would you keep the unset in the case of service monitors or pod monitors?

[jkroepke]

Good catch! Yeah, the unset can be cleaned up as well. If the end user explicit set prometheus.io/scrape and enable the monitor, it might be expected.

[jkroepke]

LGTM