Built-in collectors fail promtool linting

[mpalmer]

With a stock install of prometheus.ex exporting the default Erlang metrics, the following linting failures are reported:

$ wget -O - -q http://localhost:9548/metrics | promtool check metrics
erlang_vm_statistics_context_switches counter metrics should have "_total" suffix
erlang_vm_statistics_garbage_collection_bytes_reclaimed counter metrics should have "_total" suffix
erlang_vm_memory_atom_bytes_total non-counter metrics should not have "_total" suffix
erlang_vm_statistics_garbage_collection_number_of_gcs counter metrics should have "_total" suffix
erlang_vm_statistics_garbage_collection_words_reclaimed counter metrics should have "_total" suffix
erlang_vm_statistics_runtime_milliseconds counter metrics should have "_total" suffix
erlang_vm_statistics_wallclock_time_milliseconds counter metrics should have "_total" suffix
erlang_vm_memory_bytes_total non-counter metrics should not have "_total" suffix
erlang_vm_memory_processes_bytes_total non-counter metrics should not have "_total" suffix
erlang_vm_memory_system_bytes_total non-counter metrics should not have "_total" suffix
erlang_vm_statistics_run_queues_length_total non-counter metrics should not have "_total" suffix
erlang_vm_atom_count non-histogram and non-summary metrics should not have "_count" suffix
erlang_vm_port_count non-histogram and non-summary metrics should not have "_count" suffix
erlang_vm_process_count non-histogram and non-summary metrics should not have "_count" suffix
erlang_vm_statistics_runtime_milliseconds use base unit "seconds" instead of "milliseconds"
erlang_vm_statistics_wallclock_time_milliseconds use base unit "seconds" instead of "milliseconds"

Allowing people to adjust their base units if they like is nice, but IMAO the out-of-the-box defaults should be promtool compliant.

[NelsonVides]

I really wonder, how critical this would be. I want to release a new major version, with breaking changes, so if we're to change metric names this would be the perfect chance, but on the other hand, it'd break a lot of existing dashboards with metrics that don't exist anymore. Looking for opinions @mkuratczyk @onno-vos-dev @mikpe @deadtrickster (tagging a few random members).

[onno-vos-dev]

I'm a bit torn on this sort of change for the simple purpose of making a linter happy. The current metric names are descriptive and simply changing them and breaking all existing dashboards for this purpose seems a bit excessive in my opinion would be my initial $0,02.

I'd agree that out-of-the-box defaults should be promtool compliant but how important is this really? Is a happy linter more important than existing dashboards?
Does promtool check metrics not have a way to disable the linter? It doesn't look like it based on the documentation at least?

I'm a bit torn on this...

[mikpe]

I agree that existing dashboards are more important than satisfying that linter. OTOH I don't want to stand in the way of DTRT.

If there was a toogle to select new or old behaviour then I think a breaking update with that toogle prominently documented could be Ok.

[paulo-ferraz-oliveira]

A major is a major, right? As long as there's a guide that explains clearly, what it'll break nobody's forcing consumers. I also want to DTRT, and am also torn on this one; maybe one good thing we could do would be to integrate promtool in this repo's CIs to at least prevent further non-compliance (?)