Add Guidance wrt Labelling to Naming and Rules Best Practices

[conallob]

Add Guidance wrt Labelling to Naming and Rules Best Practices to docs/practices/naming.md and docs/practices/rules.md, specifically:

• The primary purposes of job and instance
• Include WARNINGS about accidentally stripping the job label, especially in multi-tenant systems

This Fixes #2690

[conallob]

Friendly ping @SuperQ @beorn7 ?

[conallob]

Obligatory post-it note reminder: https://photos.app.goo.gl/Bkfir4wRiLtNVG4W8

[beorn7]

With my current patchy availability, there is little chance I get to this anytime soon. Maybe @juliusv has a qualified opinion here?

[conallob]

Friendly ping @SuperQ @juliusv

[andrechalella]

Hey @conallob, congrats for the nice PR!

Just one thing: maybe you could fix the eaach typo in

• The job label is a primary key to differentiate metrics from eaach other.

There is a small confusion I would love to see fixed, in a paragraph just below one of your edits. It is:

To keep the operations clean, _sum is omitted if there are other operations,
as sum().

I don't understand the as sum() part. Like, "x is omitted if there are other operations such as x"? It doesn't make sense to me, in a very basic way. I know it's out of the scope of this PR, but maybe you could touch it to clarify.

[conallob]

Hey @conallob, congrats for the nice PR!

Just one thing: maybe you could fix the eaach typo in

• The job label is a primary key to differentiate metrics from eaach other.

There is a small confusion I would love to see fixed, in a paragraph just below one of your edits. It is:

Fixed the typo.

To keep the operations clean, _sum is omitted if there are other operations,
as sum().

I don't understand the as sum() part. Like, "x is omitted if there are other operations such as x"? It doesn't make sense to me, in a very basic way. I know it's out of the scope of this PR, but maybe you could touch it to clarify.

I'm afraid that best practice is unrelated.

It also makes sense as written, once you've written enough rules. It's weighing up the trade-off between tracking the chain of operations across a pipeline of rules vs the rule name growing unwieldy. Many of these best practices trace back to specific philosophies from Prometheus' predecessor.

If you still think it needs a polish, please a separate doc bug.

[conallob]

Ping @juliusv , since @SuperQ is currently unavailable for life reasons

[conallob]

PTAL

[conallob]

Friendly ping?

[conallob]

Friendly, you're not at SRECon EMEA this week, ping?

[SuperQ]

I'm not sure this is really a useful note here, as this applies to all label matching.

Suggested change

* If not specified in PromQL expressions, they will match unrelated metrics with the same name. This is especially true in a multi system or multi tenant installation

[conallob]

It applies to all labels. But job and instance are two uniform labels found on every metric, including ubiquitous synthetic metrics such as up

[SuperQ]

Yes, but it's not related to job, but related to "target labels" and discovery. That is a different thing and related to querying, not creating labels.

[conallob]

For perspective, one of the motivations behind this PR is the anti-patterm of writing alert expressions intended for a single tenant system, which has evolved into a multi-tenant system.

e.g up{} for 5m without defining a job label works for one job.

Once you start adding additional jobs that match on the same labels (e.g Daemonsets, fleet-wide node_exporter, etc), teams start getting paged for systems they don't own or care about

[jan--f]

Hello from the bug scrub!
@conallob Looks like there some feedback to address still, are you still working on this?

[conallob]

Coming back to this long neglected PR, afaik I can tell, the main issue here is whether to put label best practices in the variable naming best practices or not. I only did that since there is no label best practices section currently.

Would creating a new section for label best practices help to unstick this?

[conallob]

Reviving this PR, PTAL @SuperQ

• I've split the label sections of naming.md out into a new labels.md, as discussed in OOB chat with @SuperQ
• I've also reworded the line mentioning "primary key" to be about defining scope in PromQL expressions, which is the ultimate goal.
• It looks like I need a maintainer to approve the Netlify CI checks though

[conallob]

@SuperQ Friendly ping?

[krajorama]

Thanks for working on this. I feel these additions are currently too specific for your use cases to be included in the generic documentation.

[krajorama]

It might be the intention of the user to not distinguish between unrelated metrics, for example when aggregating across jobs. So I'd turn this into some positive statement instead (use "specify" instead of "not specified"). Multi system seems vague and I don't think multi tenancy has anything to do with this.

Suggested change

	- If not specified in PromQL expressions, they will match unrelated metrics with the same name. This is especially true in a multi system or multi tenant installation
	- If specified in PromQL expressions, they will match metrics scraped by the same job.

[conallob]

It might be the intention of the user to not distinguish between unrelated metrics, for example when aggregating across jobs. So I'd turn this into some positive statement instead (use "specify" instead of "not specified").

I agree that the user may want to aggregate across all, or a subset of jobs. But to do so, they should be explicit with the job label values. But I expect unintended label stripping to vastly outnumber cross-job aggregation use cases.

Multi system seems vague and I don't think multi tenancy has anything to do with this.

"Multi-tenent systems" may not be the best term, but I'm referring to a Prometheus, run as a platform for multiple teams (e.g by a DevEx or Platform Engineering team), to prevent every team running their own siloed Prometheus stack. In such a setup, all PromQL expressions should be scoped with a job label, to ensure the metrics are from the the expected exporters.

Or framed another way, in such a centralised stack, always write up{job=bla}, never up{}

[krajorama]

It might be intentional , so I think this needs to be conditional.

[krajorama]

Don't you need a similar warning for "instance" , depending on usage?

[conallob]

While instance is one of the standard scrape time labels, like job, stripping it doesn't have the same blast radius. Stripping instance will make metrics hard to debug, but should will still work.

For certain use cases that require using multiple layers of rules (e.g in a multi region, multi layered tree of Prometheus), you may want to strip out instance at the higher aggregation layers to manage label cardinality (e.g instance labels make sense to the per region aggregation, but can be problematic if aggregated at the global level)

I've added a warning that stripping instance can make it harder to debug scrape time issues with a metric though.

[krajorama]

I think the need to specify job is very circumstantial , so again I think it needs to be conditional on what you want to achieve. Also specify job is very vague in itself.

[conallob]

Can you elaborate on why job is circumstantial?

Afaik, job will always be set on metrics unless it is explicitly stripped away.

[krajorama]

I don't think multi-tenant has anything to do with job and instance labels.

[conallob]

As above, "Multi-tenent systems" may not be the best term, but I'm referring to a Prometheus, run as a platform for multiple teams (e.g by a DevEx or Platform Engineering team), to prevent every team running their own siloed Prometheus stack. In such a setup, all PromQL expressions should be scoped with a job label, to ensure the metrics are from the the expected exporters.

Or framed another way, in such a centralised stack, always write up{job=bla}, never up{}