-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add Guidance wrt Labelling to Naming and Rules Best Practices #2691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Conall O'Brien <[email protected]>
Signed-off-by: Conall O'Brien <[email protected]>
Obligatory post-it note reminder: https://photos.app.goo.gl/Bkfir4wRiLtNVG4W8 |
With my current patchy availability, there is little chance I get to this anytime soon. Maybe @juliusv has a qualified opinion here? |
Hey @conallob, congrats for the nice PR! Just one thing: maybe you could fix the
There is a small confusion I would love to see fixed, in a paragraph just below one of your edits. It is:
I don't understand the |
Signed-off-by: Conall O'Brien <[email protected]>
Fixed the typo.
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. |
Co-authored-by: Ben Kochie <[email protected]> Signed-off-by: Conall O'Brien <[email protected]>
Co-authored-by: Ben Kochie <[email protected]> Signed-off-by: Conall O'Brien <[email protected]>
@@ -19,6 +19,8 @@ This page documents proper naming conventions and aggregation for recording rule | |||
Keeping the metric name unchanged makes it easy to know what a metric is and | |||
easy to find in the codebase. | |||
|
|||
IMPORTANT: `job` label acts as a primary key. It is **strongly** recommended that you use it to scope your PromQL expressions to the system you are monitoring. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is misleading. Prometheus doesn't have the concept of "primary key". Not even metric names are a "primary key".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, especially since folks used to SQL DBs will jump to the conclusion that it's a SQL DB, which it isn't.
Iterated on the language to avoid creating ambiguity
Iterate on the description of the job label, removing "primary key", given it's association with SQL Signed-off-by: Conall O'Brien <[email protected]>
PTAL |
Friendly ping? |
Add Guidance wrt Labelling to Naming and Rules Best Practices to docs/practices/naming.md and docs/practices/rules.md, specifically:
job
andinstance
job
label, especially in multi-tenant systemsThis Fixes #2690