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.

Sign up for GitHub

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

Jump to bottom

add runbook urls for alerts #3452

Merged
merged 2 commits into from
Nov 17, 2022
Merged

add runbook urls for alerts #3452

merged 2 commits into from
Nov 17, 2022

Conversation

manohar-koukuntla
Copy link
Contributor

@manohar-koukuntla manohar-koukuntla commented Nov 15, 2022 •
edited by pracucci
Loading

What this PR does

Adds runbook_urls to all the mimir alerts

Which issue(s) this PR fixes or relates to

Fixes #2303

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@manohar-koukuntla manohar-koukuntla requested a review from a team as a code owner November 15, 2022 14:52
@CLAassistant
Copy link

CLAassistant commented Nov 15, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

jhesketh
jhesketh requested changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@jhesketh jhesketh left a comment

Choose a reason for hiding this comment

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

Thanks for this. LGTM, just one small nit on the changelog format.

CHANGELOG.md Outdated Show resolved Hide resolved
jhesketh
jhesketh reviewed Nov 17, 2022
View reviewed changes
Copy link
Contributor

@jhesketh jhesketh left a comment

Choose a reason for hiding this comment

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

Sorry, I meant grouped with the other ENHANCEMENTS. (I might have gotten the line number wrong as you would have deleted the previous line it was on). Hope this makes more sense.

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@manohar-koukuntla manohar-koukuntla force-pushed the add_runbook_urls_for_alerts branch 2 times, most recently from 8047432 to a339454 Compare November 17, 2022 12:24
@manohar-koukuntla
Copy link
Contributor Author

@jhesketh I updated the code with suggested changes, and rebased my branch with latest main.

jhesketh
jhesketh approved these changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@jhesketh jhesketh left a comment

Choose a reason for hiding this comment

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

Thanks! Sorry about the run-around. Looks good to me :-)

pracucci
pracucci reviewed Nov 17, 2022
View reviewed changes
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Great work. I just have a comment to actually reuse the utility function instead of copy-pasting it.

CHANGELOG.md Outdated Show resolved Hide resolved
operations/mimir-mixin/alerts/alerts-utils.libsonnet Show resolved Hide resolved
@pracucci pracucci enabled auto-merge (squash) November 17, 2022 14:23
@pracucci
Copy link
Collaborator

To fix the linter you can just run make build-mixin format-mixin and commit. Thanks!

manohar-koukuntla and others added 2 commits November 17, 2022 16:07
@manohar-koukuntla @jhesketh
add runbook urls for alerts
0312790 
Co-authored-by: Joshua Hesketh <[email protected]>

Update CHANGELOG.md

Co-authored-by: Joshua Hesketh <[email protected]>
@manohar-koukuntla
Update CHANGELOG.md
abcf3dc 
Co-authored-by: Marco Pracucci <[email protected]>

Add scheme to DNS service discovery docs (grafana#3450)

* Add schema to DNS service discovery docs

The missing schema leads to errors like `improperly formatted alertmanager URL "alertmanager.mimir.svc.cluster.local:8080/alertmanager" (maybe the scheme is missing?)`.

* Update docs/sources/operators-guide/configure/about-dns-service-discovery.md

Co-authored-by: Ursula Kallio <[email protected]>

Co-authored-by: Marco Pracucci <[email protected]>
Co-authored-by: Ursula Kallio <[email protected]>

Alertmanager configuration validate-only mode grafana#3437 (grafana#3440)

* Alertmanager configuration validate-only mode grafana#3437

* Documentation update for alertmanager config validation

* Update changelog

* Update docs/sources/operators-guide/architecture/components/alertmanager.md

Co-authored-by: Ursula Kallio <[email protected]>

* Swap a command line flag for a verify subcommand.

* Update pkg/mimirtool/commands/alerts.go

Co-authored-by: Marco Pracucci <[email protected]>

* For loop to reduce repetition

* Use review suggestion.

* Make alertmanager verification docs more consistent.
Bump from enhancement to feature.

* fmt

* Fix the golint issue.

* Fix typo

* Make doc result (as specified by lint process)

Co-authored-by: Ursula Kallio <[email protected]>
Co-authored-by: Marco Pracucci <[email protected]>

lint fix
auto-merge was automatically disabled November 17, 2022 15:07

Head branch was pushed to by a user without write access

pracucci
pracucci approved these changes Nov 17, 2022
View reviewed changes
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks!

@pracucci pracucci enabled auto-merge (squash) November 17, 2022 15:36
@pracucci pracucci merged commit 07c92ff into grafana:main Nov 17, 2022
masonmei pushed a commit to udmire/mimir that referenced this pull request Dec 16, 2022
@manohar-koukuntla @jhesketh
add runbook urls for alerts (grafana#3452)
4de2d25 
* add runbook urls for alerts

Co-authored-by: Joshua Hesketh <[email protected]>

Update CHANGELOG.md

Co-authored-by: Joshua Hesketh <[email protected]>

* Update CHANGELOG.md

Co-authored-by: Marco Pracucci <[email protected]>

Add scheme to DNS service discovery docs (grafana#3450)

* Add schema to DNS service discovery docs

The missing schema leads to errors like `improperly formatted alertmanager URL "alertmanager.mimir.svc.cluster.local:8080/alertmanager" (maybe the scheme is missing?)`.

* Update docs/sources/operators-guide/configure/about-dns-service-discovery.md

Co-authored-by: Ursula Kallio <[email protected]>

Co-authored-by: Marco Pracucci <[email protected]>
Co-authored-by: Ursula Kallio <[email protected]>

Alertmanager configuration validate-only mode grafana#3437 (grafana#3440)

* Alertmanager configuration validate-only mode grafana#3437

* Documentation update for alertmanager config validation

* Update changelog

* Update docs/sources/operators-guide/architecture/components/alertmanager.md

Co-authored-by: Ursula Kallio <[email protected]>

* Swap a command line flag for a verify subcommand.

* Update pkg/mimirtool/commands/alerts.go

Co-authored-by: Marco Pracucci <[email protected]>

* For loop to reduce repetition

* Use review suggestion.

* Make alertmanager verification docs more consistent.
Bump from enhancement to feature.

* fmt

* Fix the golint issue.

* Fix typo

* Make doc result (as specified by lint process)

Co-authored-by: Ursula Kallio <[email protected]>
Co-authored-by: Marco Pracucci <[email protected]>

lint fix

Co-authored-by: Joshua Hesketh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhesketh jhesketh jhesketh approved these changes

@pracucci pracucci pracucci approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Link mimir mixin alerts with their runbook
4 participants
@manohar-koukuntla @CLAassistant @pracucci @jhesketh