Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

monitoring: implement owner routing for alerts#12491

Merged
bobheadxi merged 8 commits into
masterfrom
monitoring/alert-routing
Jul 28, 2020
Merged

monitoring: implement owner routing for alerts#12491
bobheadxi merged 8 commits into
masterfrom
monitoring/alert-routing

Conversation

@bobheadxi

@bobheadxi bobheadxi commented Jul 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Adds a new owners field to observability.alerts as proposed in #12010 for RFC-189. Only alerts that match the specified owners will be received by the associated notifier. Notifiers configured without the owners field will continue to receive all alerts of the specified level, even with notifiers that have owners present.

image

(outdated screenshot, but close enough)

Closes #12010 - the other half of the proposal was implemented in https://github.com/sourcegraph/sourcegraph/pull/12358

Still thinking about owners vs onOwners 🤔

@bobheadxi bobheadxi requested review from a team and emidoots July 27, 2020 09:39
@bobheadxi bobheadxi requested a review from a team as a code owner July 27, 2020 09:39
@bobheadxi bobheadxi requested a review from pecigonzalo July 27, 2020 09:40
@pecigonzalo

pecigonzalo commented Jul 27, 2020

Copy link
Copy Markdown
Contributor

I would use owners similarly to how we use level and not onLevel. Could we add some tests to this to ensure it behaves as expected without doing a full deployment?

@codecov

codecov Bot commented Jul 27, 2020

Copy link
Copy Markdown

Codecov Report

Merging #12491 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #12491      +/-   ##
==========================================
- Coverage   50.58%   50.58%   -0.01%     
==========================================
  Files        1417     1417              
  Lines       79285    79283       -2     
  Branches     6552     6634      +82     
==========================================
- Hits        40110    40108       -2     
+ Misses      35726    35725       -1     
- Partials     3449     3450       +1
Flag Coverage Δ
#go 51.98% <ø> (-0.01%) ⬇️
#integration 24.25% <ø> (ø)
#storybook 12.63% <ø> (ø)
#typescript 46.85% <ø> (+<0.01%) ⬆️
#unit 47.20% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../internal/codeintel/resolvers/graphql/locations.go 85.45% <0.00%> (-1.82%) ⬇️
web/src/search/input/SearchPage.tsx 66.66% <0.00%> (+6.66%) ⬆️

@bobheadxi

bobheadxi commented Jul 27, 2020

Copy link
Copy Markdown
Member Author

I would use owners similarly to how we use level and not onLevel.

🚀

Could we add some tests to this to ensure it behaves as expected without doing a full deployment?

Unfortunately the only really relevant thing to test here is how alertmanager actually behaves when sending out alerts, but I'll add something to check on the rendered routes

@sourcegraph sourcegraph deleted a comment from codecov Bot Jul 27, 2020
@sourcegraph sourcegraph deleted a comment from codecov Bot Jul 27, 2020
@pecigonzalo

pecigonzalo commented Jul 27, 2020

Copy link
Copy Markdown
Contributor

I'll add something to check on the rendered routes

Exactly, we should not test Alertmanager itself or that it routes properly, only that we render the expected config given X input.

@codecov

codecov Bot commented Jul 27, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #12491 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #12491   +/-   ##
=======================================
  Coverage   50.40%   50.40%           
=======================================
  Files        1425     1425           
  Lines       79996    79996           
  Branches     6553     6553           
=======================================
  Hits        40318    40318           
  Misses      36211    36211           
  Partials     3467     3467
Flag Coverage Δ
#go 51.71% <0.00%> (ø)
#integration 24.24% <0.00%> (ø)
#storybook 12.63% <0.00%> (ø)
#typescript 46.85% <0.00%> (ø)
#unit 47.04% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

emidoots
emidoots reviewed Jul 28, 2020
View reviewed changes
Comment thread docker-images/prometheus/cmd/prom-wrapper/receivers.go Outdated
emidoots
emidoots reviewed Jul 28, 2020
View reviewed changes
Comment thread docker-images/prometheus/cmd/prom-wrapper/receivers.go Outdated
MatchRE: amconfig.MatchRegexps{
"owner": *ownerRegexp,
},
Continue: true, // match siblings, so that general-case level receivers still get the alert

@emidoots emidoots Jul 28, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does this comment mean? I'm not sure I follow - could you detail it more?

@bobheadxi bobheadxi Jul 28, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I clarified it here! 5ed2c89

emidoots
emidoots reviewed Jul 28, 2020
View reviewed changes
Comment thread schema/site.schema.json Outdated
@bobheadxi bobheadxi merged commit 5d73f94 into master Jul 28, 2020
@bobheadxi bobheadxi deleted the monitoring/alert-routing branch July 28, 2020 12:59
@bobheadxi bobheadxi mentioned this pull request Aug 1, 2020
Closed
55 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@pecigonzalo pecigonzalo Awaiting requested review from pecigonzalo

1 more reviewer

@emidoots emidoots emidoots approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Approved: Proposal: RFC-189: Support per-team alerts and on-call rotations

3 participants

@bobheadxi @pecigonzalo @emidoots