Skip to content

Modify Alertmanager config metrics #3289

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
Merged
merged 7 commits into from
Oct 14, 2020
Merged

Modify Alertmanager config metrics #3289

merged 7 commits into from
Oct 14, 2020

Conversation

jpdstan
Copy link
Contributor

@jpdstan jpdstan commented Oct 6, 2020 •
edited
Loading

Signed-off-by: Stan Kwong stanley.kwong@robinhood.com

What this PR does:

  • Changes cortex_alertmanager_config_invalid to cortex_alertmanager_config_last_reload_successful, such that the nomenclature matches that of the ruler and the upstream alertmanager
  • Adds the alertmanager_config_last_reload_successful_seconds metric

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]
pracucci
pracucci approved these changes Oct 9, 2020
View reviewed changes
Copy link
Contributor

@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.

The change LGTM, but I would like to have @gotjosh and/or @gouthamve checking it since they recently worked on that (in particular, I would like to know if we're OK with this change).
@jpdstan jpdstan force-pushed the add_am_metrics branch 2 times, most recently from 3c52172 to 3c880db Compare October 12, 2020 21:34
@jpdstan
Copy link
Contributor Author

jpdstan commented Oct 13, 2020

Any idea why my integration tests keep timing out?
@pracucci
Copy link
Contributor

Any idea why my integration tests keep timing out?

I will look into it. Do not worry too much about it. We'll take care of that.
gotjosh
gotjosh suggested changes Oct 13, 2020
View reviewed changes
@pracucci pracucci self-requested a review October 13, 2020 10:03
@pracucci
Copy link
Contributor

I've just merged the PR #3335 to increase integration tests timeout. Could you rebase this PR, please?
@jpdstan
Modify Alertmanager config metrics
8a54908 
Signed-off-by: Stan Kwong <stanley.kwong@robinhood.com>
@jpdstan
Fix gauge
3312883 
Signed-off-by: Stan Kwong <stanley.kwong@robinhood.com>
@jpdstan
Fix tests
67405fa 
Signed-off-by: Stan Kwong <stanley.kwong@robinhood.com>
@jpdstan
docs: add to CHANGELOG
0fb014f 
Signed-off-by: Stan Kwong <stanley.kwong@robinhood.com>
@jpdstan
fix: record timestamp on success
b3b7df9 
Signed-off-by: Stan Kwong <stanley.kwong@robinhood.com>
pracucci
pracucci approved these changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thanks!
@jpdstan
Copy link
Contributor Author

jpdstan commented Oct 13, 2020

@pracucci still timing out :(
@pstibrany
Copy link
Contributor

There is an alertmanager_test.go in the integration tests which is still using the old metric name.
@jpdstan
test: fix integration tests
9bbc5c9 
Signed-off-by: Stan Kwong <stanley.kwong@robinhood.com>
@jpdstan
Copy link
Contributor Author

jpdstan commented Oct 14, 2020 •
edited
Loading

9bbc5c9 @pstibrany good catch!! too bad those timeouts don't trigger a test failure. took the opportunity to add my timestamp metric in those tests as well. thanks for your help!
pracucci
pracucci reviewed Oct 14, 2020
View reviewed changes
@pracucci
Update integration/alertmanager_test.go
3d62519 
Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci merged commit b135907 into cortexproject:master Oct 14, 2020
@jpdstan jpdstan deleted the add_am_metrics branch October 14, 2020 18:25
@gotjosh gotjosh mentioned this pull request Nov 18, 2020
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M
5 participants
@jpdstan @pracucci @pstibrany @gotjosh @gouthamve