Add created_for_pv label to unused_disk_size_bytes metric

[paulajulve]

Adding this label to make my own life easier for a join I'm working on. I don't think it'll have much of an impact, since this metric already has the disk ID label, and the relationship between PV and disk should be 1 to 1. If I understand correctly, it should not affect current cardinality.

I've tested it out locally, but I don't really know how to add automated tests for the metrics in this file. Open to pointers if you have them, I think it'll be a nice improvement.

[inkel]

I've tested it out locally, but I don't really know how to add automated tests for the metrics in this file. Open to pointers if you have them, I think it'll be a nice improvement.

This is tricky, I believe. In my point of view, you would be testing that Prometheus works on adding a label on a metric to the value that you're assigning, in other words, you would be testing Prometheus and not unused.

In any case, what would be needed instead is a test that proves that Meta.CreatedForPV returns what we expect, which if I'm not mistaken, is missing.

[inkel]

LGTM but I'd like a few more reviewers to chime in before merging this.

[Pokom]

This is tricky, I believe. In my point of view, you would be testing that Prometheus works on adding a label on a metric to the value that you're assigning, in other words, you would be testing Prometheus and not unused.

I don't think you're necessarily testing Prometheus. We went down down this path in cloudcost-exporter to ensure the metrics we export have the correct set of labels and values given a set of responses from cloud providers. My philosophy around it is that once people start using the exporter, we have a contract on the metrics and want to ensure we don't have regressions. I'd say it's more of an integration/end to end test? FWIW, I found this pattern from a few prometheus-community maintained exporters:

• https://github.com/prometheus-community/postgres_exporter/blob/master/collector/pg_long_running_transactions_test.go
• https://github.com/prometheus/blackbox_exporter/blob/1b5a642f10628953d71115a8aa622154b114a833/prober/dns_test.go#L614-L657
• https://github.com/prometheus/mysqld_exporter/blob/main/collector/engine_innodb_test.go

With that though, the tests can be brittle and we occasionally have flaky tests due to race conditions. I don't think in this case it's worth it to add tests validating the exported metrics align with expectations.

[paulajulve]

There were a few tests missing for meta.go, so I've added them, even if not exactly within the scope of this PR. And cleaned up a function that wasn't being used, nor could be used across all three providers. My guess is it made sense in the first iteration of this project, but not now that it's mostly expected to work for all three major CSPs.

[paulajulve]

@Pokom agreed, I don't think this case warrants risking having to maintain brittle tests.