-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add time-based muting to routing tree #2393
Conversation
7a16adb
to
1323d17
Compare
4bcd768
to
98f97cb
Compare
Just added some additional tests for the config |
@simonpasquier Can you please let me know if there's anything else I can do for this one? Thank you! |
This would help our team greatly. Hope you get time to look at this sooner rather than later! @simonpasquier |
@simonpasquier There seems to be good interest in this one... in case you are held up by other stuff, maybe someone else wants to take a look? @roidelapluie @beorn7 I am unfortunately totally swamped at the moment. |
} | ||
|
||
// ContainsTime returns true if the TimeInterval contains the given time, otherwise returns false | ||
func (tp TimeInterval) ContainsTime(t time.Time) bool { |
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.
Do we check somewhere for empty time intervals? WIth an empty time interval, this would always return true
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.
While this makes sense due to the semantics of the time interval (where not specifying a restriction means all possible times are included), I think an empty time interval is probably a symptom of an error. I'll add a check for this, thanks.
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've thought more about this, I think we should leave this as is. This makes sense with the semantics of a time interval and modifying it would complicate the function by possibly returning only a single error case.
if err != nil { | ||
return err | ||
} | ||
r.setBegin(val) |
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.
Do we set them individualy somewhere? Can we do with one setter function ? Do we need those extra setters, can't we set EndDate directly?
Signed-off-by: Ben Ridley <benridley29@gmail.com>
Signed-off-by: Ben Ridley <benridley29@gmail.com>
Signed-off-by: Ben Ridley <benridley29@gmail.com>
Signed-off-by: Ben Ridley <benridley29@gmail.com>
…ther error message wording Signed-off-by: Ben Ridley <benridley29@gmail.com>
Signed-off-by: Ben Ridley <benridley29@gmail.com>
Signed-off-by: Ben Ridley <benridley29@gmail.com>
Signed-off-by: Ben Ridley <benridley29@gmail.com>
…r feedback Signed-off-by: Ben Ridley <benridley29@gmail.com>
40254eb
to
df54b4b
Compare
Thanks for validating that @beorn7. I've rebased and the tests are looking much happier. Do we want to add some additional words around group_interval in the docs to indicate this behavior? The current documentation is:
which doesn't give much indication of the issue we've discovered. Adding some words here might stop people from being surprised by delays in notification delivery. |
@benridley I totally agree that the current documentation is incomplete. However, let's first get clarity if the current behavior is actually the intended behavior or a bug (in which case we would document it very differently). I'll file an issue about this topic. In the meantime, we can merge this PR. |
of the form <start_day>:<end_day> and are inclusive on both ends. For example: | ||
`[‘monday:wednesday','saturday', 'sunday']` | ||
|
||
`days_of_month_ramge`: A list of numerical days in the month. Days begin at 1. |
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.
days_of_month_range
?
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.
Do you want me to fix 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.
here #2497
Can someone explain to me how to integrate this functionality? |
Hey @lopesit0, no problem! go build -o alertmanager cmd/alertmanager/main.go which will give you an alertmanager binary with this included. You can find the raw documentation also in the master branch which explains how to configure your time intervals. Just be warned this is new and potentially unstable. If you do decide to run it, please feel free to report back here or open issues against the repository so we can fix then ASAP! EDIT: Alternatively, I've realized there's a Docker image built from master. So the easiest way to try this out might be using the official docker image built from master. |
Thanks for your answer :) |
Is there any way to make use of this in the helm chart at the moment? It seems like the operator does not handle this yet. |
We need a release of Alertmanager. I think @roidelapluie was working on it, but there were flaky tests blocking it. Perhaps those are resolved now? |
See #2557 |
Hi all.
But with that config alertmanager don't want to start.
Can anyone help me? |
the error about line: |
Hi 👋
I'm testing with amtool and when I send the alert I didn't receive it. If I remove that time interval it go off as expected. BTW today is not Monday and I'm testing for more than 1 minute. |
Just found the issue. I should use
|
sorry team, is there any chance to see this feature within the docker image? |
It has been released in v0.22. Any Docker image with that or a later release should support it. |
Addresses #876
This adds the ability to define time intervals as per this design document.
Time intervals are defined in their own configuration section like so:
Then they can be referenced in the routing tree like so:
Time zones are not supported at the moment because it would break the ability to run Alertmanager on Windows in a reliable way. Once this issue is addressed we can add support fairly easily.