Skip to content
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

Add style guide for alert naming, labels, annotations #10

Merged
merged 1 commit into from
Aug 5, 2020

Conversation

beorn7
Copy link
Contributor

@beorn7 beorn7 commented Aug 5, 2020

This is obviously very opinionated as I just put down what I'm evangelizing myself. But I wanted to get started with a suggestion. Let's take it from here. @brancz @paulfantom

README.md Outdated
- `runbook_url` (optional): A link to a runbook for the alert. (TODO: Flesh out
details. Also see [kubernetes-mixin issue
#312](https://github.com/kubernetes-monitoring/kubernetes-mixin/issues/312).)
- `dashboard_url` (optional): A link to a meaningful Grafana dashboard. (TODO:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in favor of this annotation field, but it greatly complicates a generic usage of mixins. This is the only field that always needs to be configured by end-user, which is not the case for any other mixin.

Additionally, it does not show up in any of the currently used ones. I would prefer to do a spike on at least one mixin before making this into a general guideline.

Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy with moving forward with this modulo runbook and dashboard url

README.md Outdated
$labels.instance }} has only {{ printf "%.2f" $value }}% available inodes
left.`, `Alert notification queue of Prometheus %(prometheusName)s is running
full.`.
- `runbook_url` (optional): A link to a runbook for the alert. (TODO: Flesh out
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like runbook url and dashboard url, but as you already noted, these haven't really been fleshed out enough, I think we should skip them for now

@brancz
Copy link
Member

brancz commented Aug 5, 2020

cc @metalmatze

Copy link
Member

@paulfantom paulfantom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome doc! Apart from dashboards_url I am in favor of such guidelines.

@beorn7
Copy link
Contributor Author

beorn7 commented Aug 5, 2020

Sure, let's remove dashboard_url and runbook_url. But perhaps add a note that we intend to add a guideline for those once we have fleshed out the details? That would help users that like those annotation and might assume we haven't even thought about those…

@brancz
Copy link
Member

brancz commented Aug 5, 2020

Yeah I'm happy with that.

@brancz brancz merged commit 5a2d7c0 into monitoring-mixins:master Aug 5, 2020
@metalmatze
Copy link

Amazing stuff!
Sorry for lag, still catching up from everything after 2 offline weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants