helm: enable prometheus autodiscovery

[klihub]

helm: allow autodiscovery by prometheus.

Allow autodiscovery of prometheus metrics by annotation. IOW, annotate our plugins' pods according to the Helm config.instrumentation.{prometheusExport,httpEndpoint} values. Since this PR adds annotations to our plugins' pods, add support for additional injection of other custom annotations using the Helm plugin.annotations value.

Also, since it's now uneccessary to expose our custom metrics port on the host side, remove the required and dedicated hostPort Helm value. Add instead an optional generic ports array to allow exposing arbitrary ports to the host. Remove hostPort from the nriplugindeployments CRD as well.

[fmuyassarov]

I'm not sure if I'm missing some context, but could you clarify why we need to introduce these annotations when we already have Helm flags that control the instrumentation of the balloons policy CR? The only reason I can think of for having two methods to enable scraping on the pod would be to allow overriding the default port and scraping settings. Is that the case, or is there something else at play here?

Also, the nri.plugin.annotations setting initially gave an impression that users could pass any annotation they wanted, suggesting it wasn't exclusively linked to Prometheus. However, in the daemonSet spec, it seems that whatever is passed through 'nri.plugin.annotations' ultimately maps to prometheus.io/port, which is a bit odd.

[klihub]

I'm not sure if I'm missing some context, but could you clarify why we need to introduce these annotations when we already have Helm flags that control the instrumentation of the balloons policy CR? The only reason I can think of for having two methods to enable scraping on the pod would be to allow overriding the default port and scraping settings. Is that the case, or is there something else at play here?

There is no existing helm flag to control instrumentation, only a CR configuration setting. There is a Helm value to expose the metrics port to a host port which should not be necessary at all and is removed by this PR, or actually converted to generic port expose mechanism.

All in all, the purpose of this PR is to

• allow prometheus to automatically discover and start scraping our metrics
• remove the metric port exposure to the host as it now should be unnecessary

[klihub]

Also, the nri.plugin.annotations setting initially gave an impression that users could pass any annotation they wanted, suggesting it wasn't exclusively linked to Prometheus. However, in the daemonSet spec, it seems that whatever is passed through 1nri.plugin.annotations1 ultimately maps to prometheus.io/port, which is a bit odd.

If that is the case then I screwed up the latest rebase. The ide is exactly to provide a way to inject arbitrary annotations. But in addition to any custom annotations by Helm config we always annotate for prometheus auto-discovery.

[fmuyassarov]

prometheus auto-discovery

btw, what is prometheus auto-discovery, do you have some reference documentation ?

[fmuyassarov]

prometheus auto-discovery

btw, what is prometheus auto-discovery, do you have some reference documentation ?

I guess you mean that you want the pod to advertise itself as it it can be scraped by prometheus right? like sending ARP

[klihub]

prometheus auto-discovery

btw, what is prometheus auto-discovery, do you have some reference documentation ?

I guess you mean that you want the pod to advertise itself as it it can be scraped by prometheus right? like sending ARP

Yes.

[klihub]

prometheus auto-discovery

btw, what is prometheus auto-discovery, do you have some reference documentation ?

I just googled for autodiscovery then found a few articles describing it. For instance,

• https://signoz.io/guides/how-to-monitor-custom-kubernetes-pod-metrics-using-prometheus/
• https://learn.microsoft.com/en-us/azure/azure-monitor/containers/prometheus-metrics-scrape-configuration?tabs=CRDConfig%2CCRDScrapeConfig%2CConfigFileScrapeConfigBasicAuth%2CConfigFileScrapeConfigTLSAuth#enable-pod-annotation-based-scraping
• https://developer.ibm.com/tutorials/monitoring-kubernetes-prometheus/
• https://acagroup.be/en/blog/auto-discovery-of-kubernetes-endpoint-services-prometheus/

[fmuyassarov]

There is no existing helm flag to control instrumentation, only a CR configuration setting.

There is:

nri-plugins/deployment/helm/balloons/values.yaml

Line 43 in a0d1476

instrumentation:

[fmuyassarov]

Don't we need prometheus.io/path: '/metrics' annotation alongside other too?

[fmuyassarov]

Also, I would merge b437b94 and cdce5b6 into a single commit.

[klihub]

Don't we need prometheus.io/path: '/metrics' annotation alongside other too?

That would only be necessary if we exposed our metrics on a non-standard path, something else than the default /metrics.

[klihub]

There is no existing helm flag to control instrumentation, only a CR configuration setting.

There is:

nri-plugins/deployment/helm/balloons/values.yaml

Line 43 in a0d1476

instrumentation:

That is not a genuine Helm-level setting. It is part of the Helm-level opaque config YAML blob we inject into the default policy CRD. If you look at values.schema.json, it (intentionally) says nothing about config.instrumentation.prometheusExport or even config.instrumentation.

[fmuyassarov]

LGTM

[askervin]

I've got only one question on this.

Could we enable installation so that only the port name is defined but the host is not? That is, we would not ever specify hostPort unless user very very much wants it there.

[klihub]

I've got only one question on this.

Could we enable installation so that only the port name is defined but the host is not? That is, we would not ever specify hostPort unless user very very much wants it there.

Updated, making ports.host/hostPort optional.

[askervin]

LGTM