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

Show alert annotations #833

Merged
merged 3 commits into from
May 30, 2017
Merged

Show alert annotations #833

merged 3 commits into from
May 30, 2017

Conversation

w0rm
Copy link
Member

@w0rm w0rm commented May 29, 2017

Closes #821

annotations

@w0rm w0rm force-pushed the alert-annotations branch from cc64477 to 6d43b98 Compare May 30, 2017 08:38
@w0rm
Copy link
Member Author

w0rm commented May 30, 2017

@mxinden @stuartnelson3 this bindata stuff is annoying. It produces different file locally, even when I pull the latest golang docker and do docker run --rm -t -v "$(pwd):/usr/src/app" -w /usr/src/app golang make assets

@w0rm w0rm force-pushed the alert-annotations branch from 6d43b98 to 4362973 Compare May 30, 2017 08:56
Copy link
Contributor

@stuartnelson3 stuartnelson3 left a comment

Choose a reason for hiding this comment

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

Looks great, but there's a bug -- it seems that the maybeActiveIndex ends up showing the annotations for all alerts at index idx, even if they're separated into different groups.

So if we separate groups by alertname and dev, if I click index == 0 in the first group, it shows annotation data at index == 0 in all groups.

I've attached the time traveling debugger things for you to see:
index_per_group.txt

@w0rm
Copy link
Member Author

w0rm commented May 30, 2017

@stuartnelson3 oh I see. it would be nice if alerts had ids so we didn't have to rely on index

@stuartnelson3
Copy link
Contributor

Well, they DO have a unique fingerprint.

@mxinden @fabxc @brancz thoughts on returning the alert's fingerprint from the /alerts endpoint?

@w0rm
Copy link
Member Author

w0rm commented May 30, 2017

@stuartnelson3 if we're going to add this field, will it be named id? I can mock it in the decoder to be index for now, and then we can use the real value.

@mxinden
Copy link
Member

mxinden commented May 30, 2017

Is it possible to finish this PR without adding id to the alert object for now? If so, can we move the AlertID discussion to this issue #681

@w0rm w0rm force-pushed the alert-annotations branch from 4362973 to cb5ae67 Compare May 30, 2017 16:30
@w0rm
Copy link
Member Author

w0rm commented May 30, 2017

@mxinden I think it's fine to enhance alerts with local ids. We will just have to fix the decoder, and it will still work. Please re-review my changes.

Also, I fixed the decoding of silenced status, it actually works now and links to the first silence in the silencedBy array:

screen shot 2017-05-30 at 18 30 55

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Great work. Thanks for the quick adjustments!

@w0rm
Copy link
Member Author

w0rm commented May 30, 2017

@stuartnelson3 @mxinden sorry for the misinformation, it was the same problem of updated elm-test. I fixed the versions in the Dockerfile and it works now.

@w0rm w0rm merged commit eff5341 into master May 30, 2017
@w0rm w0rm deleted the alert-annotations branch May 30, 2017 19:04
hh pushed a commit to ii/alertmanager that referenced this pull request Feb 28, 2018
* Use go 1.10
* Use latest aktau/github-release
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.

3 participants