nsqio
Metrics Prom scrape annotation
The Service port is not used for scraping. The actual Endpoints resources part of Service are at 9117 (targetPort/containerPort). The best practice for scraping metrics is to do that against Pods and/or Endpoints, not Services ports directly. It is known to cause issues if more than one endpoint is behind the service, as metric queries will be load-balanced as well leading to inaccurate histograms.
The best practice for scraping metrics is to do that against Pods and/or Endpoints, not Services ports directly. It is known to cause issues if more than one endpoint is behind the service, as metric queries will be load-balanced as well leading to inaccurate histograms.
That's true, if you're scraping counters from something, you want to hit the same instance of that thing every time, and you want to hit every instance individually.
But in this case ... well this change doesn't make any difference, it just changes the port, not the IP or host, and it's the same port value by default. This port actually should stay the same, prometheus will be trying to connect to the service on this port, and this is the service's configured port, and the service is then configured to connect to the nsq_exporter containerPort on the nsqd pods. So, this PR doesn't fix anything, it just mixes up the ports (and happens to work because it's the same ports).
I've never used this chart, we mostly make it available by popular demand. We've all absolutely made extensive use of nsqd metrics, but I'm pretty sure no maintainers here have ever used the metrics part of this chart, we've just accepted the contribution. You're right, this isn't working right, prometheus should be collecting metrics from each nsqd, not a random one each time. This will need a much more significant refactor to fix, and it probably won't be done by the maintainers here.
But in this case ... well this change doesn't make any difference, it just changes the port, not the IP or host, and it's the same port value by default.
The difference with this proposed change is that it allows for parameterization and customization for anyone using this chart in cases where the Service port differs from the ports in Pods/Endpoints. There is no strict requirement that the ports of Pods/Endpoints created by a Kubernetes Deployment (or StatefulSet in this case) must match the Service ports; they might be the same, but they can also differ. When the ports are different, scraping fails because the Helm chart configures the annotations—used by Prometheus for scraping operations—to utilize the same port exposed in the Service resource. However, the actual source of truth for this information should be the Service Endpoints, which are the StatefulSet Pods.
Although the nsqd StatefulSet creates pods that handle traffic on port 9117 by default, there may be a need to create a metrics Service that exposes port 8700 or another port. In such cases, the annotation should be set to 9117—the .metrics.containerPort—instead of 8700 (the .metrics.service.ports.metrics from the Service).
This port actually should stay the same, prometheus will be trying to connect to the service on this port, and this is the service's configured port, and the service is then configured to connect to the
nsq_exportercontainerPort on the nsqd pods.
Prometheus always tries to connect to the Service Endpoints (see the screenshot in the PR body) rather than the Service port itself. If it were to scrape the Service port directly, it would connect in a random manner through the Service load balancer to various backend Endpoints (different nsqd Pods), instead of consistently connecting to the same resource, which would result in inaccurate histograms.
So, this PR doesn't fix anything, it just mixes up the ports (and happens to work because it's the same ports).
In the use case mentioned above, where the Service is exposed on port 8700, the annotation would mistakenly be set to 8700 instead of 9117 (the actual nsq_exporter containerPort). This misconfiguration leads Prometheus to attempt scraping metrics on port 8700, which doesn't match the intended containerPort (9117). As a result, Prometheus would fail to collect the correct metrics because it would not be connecting to the appropriate port where the nsq_exporter is actually serving metrics. Therefore, it’s crucial to ensure that the annotation points to the correct containerPort (9117) to avoid any discrepancies in monitoring and data collection.
I've never used this chart
I've been using this chart for a while, and the screenshot in the PR body shows Prometheus attempting to connect to the Service Endpoints but failing because the Service is exposed on a different port than the Service Endpoints are handling. I have already addressed this issue in my own monitoring setup, but I believe the purpose of a Helm chart is to allow for maximum customization with minimal assumptions. However, I am happy to close this PR if such customization isn't desired.
You're right, this isn't working right, prometheus should be collecting metrics from each nsqd, not a random one each time. This will need a much more significant refactor to fix, and it probably won't be done by the maintainers here.
Prometheus is doing its job correctly, as you mentioned, by scraping metrics from each nsqd (each Service Endpoint resource). However, the default Helm chart setup isn't correctly configured to point Prometheus to the right port for the use case I mentioned. What if my metrics Service is exposed on a different port than 9117? Shouldn't I be able to expose the Service on a different port while still allowing Prometheus to correctly scrape the Endpoints? In my opinion, if someone wants to expose the Service on a port other than 9117, the Helm chart should support this while ensuring that Prometheus monitoring annotations are correctly set up on the source-of-truth port, which is the containerPort of the nsq_exporter container in the nsqd Pods.
So it seems prometheus is working much differently than I assumed, from reading the chart. But then it seems like there's still much to cleanup in the chart? The existence of this metrics "Service" is confusing, if it's not for prometheus to connect to? The port that the Service listens on is irrelevant?
Should the nsqd pods get the prometheus annotations, and this Service just be removed?
The existence of this metrics "Service" is confusing, if it's not for prometheus to connect to? The port that the Service listens on is irrelevant?
Agreed, the Service port itself is not relevant for monitoring from Prometheus and its existance might be confusing; only the annotations which is the source of truth which port it should scrape.
The alternative ways to implement monitoring are either using nsqd pods annotations as you said; or through a ServiceMonitor resource which will use the Service Endpoints and you can define the port (that is how I fixed this case without using the changeset here in this PR).
So... do we want to land this or @wreis are you proposing/signing up for a larger/proper fix?
Are you all still planning to merge this?! Otherwise, I am gonna close it for now.