windows_mssql metrics use 'instance' label like Pormetheus

[ymasson]

Hi,

The MSSQL metrics use the label 'instance'.
But, this label is automatically attached when Prometheus scrape a target.
https://prometheus.io/docs/concepts/jobs_instances/#jobs-and-instances

In Grafana, if I do query_result({job="myjob"}), and filter on label 'instance', I have targets names and MSSQL instance in the list.
It's complicated to extract the targets list.

is using 'instance' for MSSQL a good idea ?

[breed808]

Can you paste a sample of the mssql metrics you're seeing? I think the label will need to be named something more specific, such as mssql_instance.

[ymasson]

# HELP windows_mssql_accessmethods_au_batch_cleanup_failures (AccessMethods.FailedAUcleanupbatches)
# TYPE windows_mssql_accessmethods_au_batch_cleanup_failures counter
windows_mssql_accessmethods_au_batch_cleanup_failures{instance="TESTDB"} 0
# HELP windows_mssql_accessmethods_au_batch_cleanups (AccessMethods.AUcleanupbatches)
# TYPE windows_mssql_accessmethods_au_batch_cleanups counter
windows_mssql_accessmethods_au_batch_cleanups{instance="TESTDB"} 0
# HELP windows_mssql_accessmethods_au_cleanups (AccessMethods.AUcleanups)
# TYPE windows_mssql_accessmethods_au_cleanups counter
windows_mssql_accessmethods_au_cleanups{instance="TESTDB"} 0

[breed808]

That's definitely not good: the instance label will conflict. I'll change this to mssql_instance.

[ymasson]

Thanks @breed808

[bryanklewis]

@breed808, @ymasson I wanted to comment on this as i think there could be a view either way.

I would argue it is correct as is and should not be modified. All metrics generated by the mssql collector have a scope or context of the database instance. The physical host has nothing to do with the identity or uniqueness of the metric. The normal value of instance as filled by address by default does not apply. If the physical host name was still wanted, one could always write a relabel rule to add address as host.

Now on the other hand, the full unique name of an mssql instance is technically "hostname\instance" but that would look sloppy as a single tag. So then instance stays as essentially host and the you add instance as proposed mssql_instance.

I can see it either way so im torn.

[bryanklewis]

without making any change you could accomplish the above with:
- job_name: mssql
honor_labels: true
relabel_configs:
- source_labels: [__address__]
regex: .*
target_label: host
replacement: $1
action: replace

[breed808]

Thanks for the input @bryanklewis!
I can understand where you're coming from regarding the identity of the metric, though I think setting the host as the instance flag by default would be less confusing and would keep the metrics in line with other collectors from this exporter.

My thinking is that using the database instance as the instance flag would cause confusion, as it wouldn't be consistent with other collectors & metrics from this exporter. It also causes difficulty in troubleshooting (E.G. which host are these metrics being scraped from?).

I'm not overly familiar with the mssql exporter myself, so perhaps further discussion is required on this before a change is made.

[bryanklewis]

I mostly wanted to validate this wasn't overriding an intended feature. I think your evaluation is fair, it comes down to preference for consistency among collectors or metric naming accuracy. Sounds like consistency with the other collectors in the project is preferred. For what its worth im good with that and rather not delay the change. bias for action over analysis. ;-) thank you for taking time to consider.