-
Notifications
You must be signed in to change notification settings - Fork 816
Update Prometheus vendoring, adapt Cortex #668
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
Conversation
7f3c4a8
to
a1d4089
Compare
(and some test updates still needed) |
The most noticeable change is around the lifecycle manager of the Alertmanager notifier and its associated service discovery manager. This has now been moved from the notifier package into main.go in Prometheus and thus we also need to duplicate that lifecycle management ourselves.
ccf4ddb
to
dda18ca
Compare
Ok, tests updated and additionally verified that it still sends notifications in a real setup. @jml Should be ready now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm quite rusty post-vacation, but I think this is all good.
pkg/ruler/ruler.go
Outdated
notifiers map[string]*rulerNotifier | ||
} | ||
|
||
type rulerNotifier struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a comment for this explaining what it is and why we want it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pkg/ruler/ruler.go
Outdated
} | ||
delay := 0 * time.Second // Unused, so 0 value is fine. | ||
return rules.NewGroup("default", "none", delay, rs, opts), nil | ||
} | ||
|
||
// sendAlerts implements a the rules.NotifyFunc for a Notifier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: s/a the/a/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
This is preparatory work for #622
The most noticeable change is around the lifecycle manager of the Alertmanager
notifier and its associated service discovery manager. This has now been moved
from the notifier package into main.go in Prometheus and thus we also need to
duplicate that lifecycle management ourselves.