docs: Add best practices for metrics

[mrueg]

What this PR does / why we need it:
This PR introduces a doc for best practices around metrics.
Hoping to get some comments and an agreement on these ideas.
@rexagod @dgrisonnet @dashpole @logicalhan @CatherineF-dev

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
None
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

[CatherineF-dev]

LGTM for Stability

[SuperQ]

Looking really nice so far. A couple of comments.

[SuperQ]

Hmm, I'm not sure about this one.

Prometheus metric families should have consistent labels.

For example, this is considered invalid.

my_metric{a="foo",b="bar"} 1
my_metric{b="baz"}1

In the above case, you want to expose an empty string label so the metric family is consistent.

my_metric{a="foo",b="bar"} 1
my_metric{a="",b="baz"}1

[mrueg]

That's good to know! I'll fix it (and will add some fixes to the next release of ksm).
Is there any upstream documentation on this that suggests labels to be empty instead of leaving them out so I can reference them in here?

[SuperQ]

I thought we had an exmaple in the OpenMetrics exposition spec, but I can't find it right now. I'll see if I can find the docs on this.

[rexagod]

The technicality here can be split into two considerations:

• The lint enforcement mentioned above comes about as a result of the registry machinery.
• KSM does not employ the same for its metrics since none of these are structured (except KSM's own meta-metrics, but piped-out the in the same non-proto manner).

KSM bypasses this trait by design (a promtool check metrics will succeed for the invalid example above).

That being said, it's always good to be consistent with the ecosystem, given we can incorporate a solution that ensures this in the CI.

[mrueg]

I'm also trying to suggest we move most labels into a single metric

metric_with_lots_of_labels{uid=identifier, a1=a1,a2=a2,...an=an} 1

instead of

metric_for_a1{uid=identifier,a1=a1}
metric_for_a2{uid=identifier,a2=a2}
....
metric_for_an{uid=identifier,an=an}

unless there are good reasons why the latter one is better for a TSDB/Querying etc

[rexagod]

I believe field-specific metrics may help (a) limit cardinality (ref)? In the original issue, there was talk about including only tightly bounded labelsets in the _info metrics, however, that may lead to separate metrics that may fall into the "gray area" and (b) be ambiguous to the user if they are in an _info metric or not, as there isn't a feasible way to guess their inclusion or exclusion from the same without knowing a fields possible values beforehand. Not to mention, (c) static values may become dynamic in the future and cause breaking changes when moved to their own metrics (referring to the metric stability spec for BETA metrics).

[mrueg]

I believe field-specific metrics may help (a) limit cardinality (ref)? In the original issue, there was talk about including only tightly bounded labelsets in the _info metrics, however, that may lead to separate metrics that may fall into the "gray area" and (b) be ambiguous to the user if they are in an _info metric or not, as there isn't a feasible way to guess their inclusion or exclusion from the same without knowing a fields possible values beforehand.
Not to mention, (c) static values may become dynamic in the future and cause breaking changes when moved to their own metrics (referring to the metric stability spec for BETA metrics).

This is why you need to look at the lifecycle of the object. If it's not lifetime scoped, it's dynamic. I'd rather have a few metric series with more labels than a bunch of metrics to avoid bad UX during querying where you need to do a few group lefts and joins to get to the result you want to see.

[SuperQ]

LGTM

[richabanker]

/triage accepted
/assign @rexagod

[brancz]

lgtm!

[brancz]

empty string and not having the label is the same thing in Prometheus, so why do this?

[mrueg]

This might be treated differently by other monitoring systems, so rather stay explicit here and ensure we do it the same way everywhere.

[mrueg]

Feel free to chime in on #2528 (comment)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, mrueg, SuperQ

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [mrueg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mrueg]

@dgrisonnet @CatherineF-dev @rexagod I think this is in a good state now. :)

[rexagod]

/assign @dgrisonnet

For visibility.