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

dispatch: fix missing receivers in Groups() #1964

Merged

Conversation

simonpasquier
Copy link
Member

@simonpasquier simonpasquier commented Jul 16, 2019

Closes #1959

@simonpasquier simonpasquier force-pushed the fix-inconsistent-alertgroup-v2 branch 2 times, most recently from c6595bd to 613a5ad Compare July 16, 2019 16:12
sort.Sort(groups)
for i := range groups {
sort.Sort(groups[i].Alerts)
Copy link
Contributor

Choose a reason for hiding this comment

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

great idea

Copy link
Member Author

Choose a reason for hiding this comment

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

It was very much needed for the tests to pass :)

@simonpasquier simonpasquier requested a review from mxinden July 18, 2019 14:06
@simonpasquier
Copy link
Member Author

@prymitive can you tell me if it fixes the issue for you?

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@simonpasquier simonpasquier force-pushed the fix-inconsistent-alertgroup-v2 branch from 613a5ad to 110dd75 Compare July 23, 2019 08:23
@prymitive
Copy link
Contributor

I can do some testing after work and update here

@prymitive
Copy link
Contributor

Test using master branch:

$ ./alertmanager --version
alertmanager, version 0.18.0 (branch: HEAD, revision: f450720213de12237437552fa8e86b715e0baafa)
  build user:       lukasz@kwakBook
  build date:       20190723-21:56:49
  go version:       go1.12.7
kwakBook:alertmanager lukasz$ curl -s localhost:9093/api/v2/alerts/groups | jq -c '.[] | .receiver' | sort | uniq -c
   3 {"name":"by-cluster"}
  15 {"name":"by-cluster-service"}
  24 {"name":"by-name"}
kwakBook:alertmanager lukasz$ curl -s localhost:9093/api/v2/alerts/groups | jq -c '.[] | .receiver' | sort | uniq -c
   3 {"name":"by-cluster"}
  15 {"name":"by-cluster-service"}
  24 {"name":"by-name"}
kwakBook:alertmanager lukasz$ curl -s localhost:9093/api/v2/alerts/groups | jq -c '.[] | .receiver' | sort | uniq -c
   3 {"name":"by-cluster"}
  15 {"name":"by-cluster-service"}
  24 {"name":"default"}

There's either default or by-name receiver, but never both, so we hit this issue.

Test using this PR as HEAD:

$ ./alertmanager --version
alertmanager, version 0.18.0 (branch: pr/1964, revision: 110dd7555c254a3409515d1d7289e6833b7acd1a)
  build user:       lukasz@kwakBook
  build date:       20190723-21:45:37
  go version:       go1.12.7
kwakBook:alertmanager lukasz$ curl -s localhost:9093/api/v2/alerts/groups | jq -c '.[] | .receiver' | sort | uniq -c
   3 {"name":"by-cluster"}
  16 {"name":"by-cluster-service"}
  13 {"name":"by-name"}
  13 {"name":"default"}
kwakBook:alertmanager lukasz$ curl -s localhost:9093/api/v2/alerts/groups | jq -c '.[] | .receiver' | sort | uniq -c
   3 {"name":"by-cluster"}
  15 {"name":"by-cluster-service"}
  12 {"name":"by-name"}
  12 {"name":"default"}
kwakBook:alertmanager lukasz$ curl -s localhost:9093/api/v2/alerts/groups | jq -c '.[] | .receiver' | sort | uniq -c
   3 {"name":"by-cluster"}
  16 {"name":"by-cluster-service"}
  13 {"name":"by-name"}
  13 {"name":"default"}

Both receivers are always present as expected, so the issue is gone.

@simonpasquier simonpasquier merged commit ab537b5 into prometheus:master Jul 24, 2019
@simonpasquier simonpasquier deleted the fix-inconsistent-alertgroup-v2 branch July 24, 2019 15:12
DuskEagle pushed a commit to DuskEagle/alertmanager that referenced this pull request Aug 1, 2019
Signed-off-by: Simon Pasquier <spasquie@redhat.com>
@fcastello
Copy link

fcastello commented Sep 19, 2019

Can it be that this issue was not fully fixed?
I am still seeing duplicates when I add multiple receivers

    - match:
        destination: webhook
      receiver: webhook
      continue: true
    - match:
        destination: webhook
      receiver: slack

That is the part of my config that generates the duplicate alerts in the UI, Also they are inheriting the group_by from the main config.
I am running the image with master tag from dockerhub which states that was built 2 days ago so this fix should be in there

@simonpasquier
Copy link
Member Author

@fcastello can you share a screenshot and the full AlertManager configuration? I tried to reproduce but couldn't spot the issue.

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.

Not all receivers present in /api/v2/alerts/groups responses
4 participants