NVIDIA / dcgm-exporter

NVIDIA GPU metrics exporter for Prometheus leveraging DCGM
Apache License 2.0
833 stars 150 forks source link

Seeking community feedback on potential new feature: Standardize labels for next major release #356

Open glowkey opened 2 months ago

glowkey commented 2 months ago

Is this a new feature, an improvement, or a change to existing functionality?

New Feature

Please provide a clear description of the problem this feature solves

From this comment there was a request to standardize the labels coming from DCGM-Exporter to make them prometheus compliant. This would be a breaking change, which could be done in the next major release (4.0) coming this fall. We are seeking feedback on how the community views this type of breaking change that helps standardize and cleanup the product. Please let us know your opinion.

(Note this is not a commitment to make any changes, simply a request for feedback on potential changes)

Feature Description

Standardize all labels to follow prometheus best practices

Describe your ideal solution

All labels are prometheus compliant.

Additional context

No response

frittentheke commented 1 month ago

Thanks @glowkey for picking up on my question in this way! I really appreciate it.

Apart from the different casing style for the labels, there are some collisions with labels also coming from Prometheus' kubernetes_sd, e.g.:

So labels from the exporter will end up being prefixed with exported_ depending on honor_labels:

# honor_labels controls how Prometheus handles conflicts between labels that are
# already present in scraped data and labels that Prometheus would attach
# server-side ("job" and "instance" labels, manually configured target
# labels, and labels generated by service discovery implementations).
#
# If honor_labels is set to "true", label conflicts are resolved by keeping label
# values from the scraped data and ignoring the conflicting server-side labels.
#
# If honor_labels is set to "false", label conflicts are resolved by renaming
# conflicting labels in the scraped data to "exported_<original-label>" (for
# example "exported_instance", "exported_job") and then attaching server-side
# labels.
#
# Setting honor_labels to "true" is useful for use cases such as federation and
# scraping the Pushgateway, where all labels specified in the target should be
# preserved.

(https://prometheus.io/docs/prometheus/latest/configuration/configuration/#scrape_config)

There are two implications and approaches to this matter.

  1. If you set honor_labels the metrics are nicely mapped to the actual pod they are about and joining on those labels with other pod metrics is straightforward (see https://github.com/prometheus/prometheus/issues/2204 for the hoops currently required to join metrics on different labels).

  2. But you lose the strict pattern of these labels coming only from kubernetes_sd and indicating which pod they were scraped from (the exporter).

If you go with approach 1 (which is somewhat more sensible, considering the metrics are about other pods), please make the documentation more explicit and even consider changing the default for HonorLabels in the Helm Chart of the gpu-operator (https://github.com/NVIDIA/gpu-operator/blob/d4316a415bbd684ce8416a88042305fc1a093aa4/deployments/gpu-operator/values.yaml#L326C1-L326C23) of the serviceMonitor (https://github.com/prometheus-operator/prometheus-operator/blob/main/Documentation/api.md#scrapeconfigspec)

nvvfedorov commented 1 month ago

@frittentheke , The best place to suggest changes in the gpu-operator is to raise the issue here: https://github.com/NVIDIA/gpu-operator/issues.

nvvfedorov commented 1 month ago

@frittentheke , Regarding labels that are coming from the DCGM-Exporter, such as namespace, pod, and container, how would you like to see such metrics? Maybe we need to add a prefix, such as "gpu_", for example: "gpu_namespace", "gpu_pod", etc. What do you think?

frittentheke commented 1 month ago

@frittentheke , Regarding labels that are coming from the DCGM-Exporter, such as namespace, pod, and container, how would you like to see such metrics?

I have no strong opinion on using either of the two options - but I do tend to like option 1 (keep using namespace, pod and container, but then also switch to honor_labels=true) a little more

Just go one path and document this clearly:

1) If you keep the label names the same as how they come from the kubernetes_sd metadata, it just makes sense to use honor_labels=true by default - because that is what makes the most sense.

If you are at it, I'd also change the Hostname label to be node as this is the term Kubernetes uses. Again, this also aligns nicely with other metrics in case one wants to joint metrics.

Maybe we need to add a prefix, such as "gpu_", for example: "gpu_namespace", "gpu_pod", etc. What do you think?

2) If you rather NOT override the labels namespace, pod and container having a static prefix for all labels does make sense. This follows the call to standardize the labels which this issue is about, so this effort should then cover ALL gpu related labels.

tooji commented 1 month ago

something that is annoying about this exporter is that each nvlink metric is reported as an individual metric as opposed to the same metric with individual labels (nvlink=....) -- can we fix that at some point?

nvvfedorov commented 1 month ago

something that is annoying about this exporter is that each nvlink metric is reported as an individual metric as opposed to the same metric with individual labels (nvlink=....) -- can we fix that at some point? Can you provide an example?