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

amtool: Fix behavior of adding silence with duration option #2741

Merged

Conversation

nekketsuuu
Copy link
Contributor

Closes #2490, also closes #2740.

amtool treats --duration option differently between amtool silence add and amtool silence update; amtool silence add calculates endsAt as time.Now() + duration, while amtool silence update calculates endsAt as startsAt + duration.

According to #1298 (comment), it seems that startsAt + duration is preferable. That also resolves the confusion such as #2490 and #2740.

So I changed behavior of --duration option of amtool silence add. Note that this is a breaking change.

Local test:

$ # This raises an error "silence cannot start after it ends" using amtool 0.23.0.
$ ./amtool silence add alertname=testing --start=2022-01-01T00:00:00Z --duration=30m --comment=testing --alertmanager.url=http://localhost:9093
aa6c19aa-3a14-4760-9075-d726c8bd9cb9
$ ./amtool silence --alertmanager.url=http://localhost:9093
ID                                    Matchers             Ends At                  Created By  Comment  
aa6c19aa-3a14-4760-9075-d726c8bd9cb9  alertname="testing"  2022-01-01 00:30:00 UTC  nek         testing  

It may be better to add tests for these CLI options. Adding tests, however, will be a little large change to cli package because almost all functions of cli package are not exported. So I skipped adding tests in this patch.

Signed-off-by: nekketsuuu <[email protected]>

endsAt should be calculated from startsAt, not from the current time

Signed-off-by: nekketsuuu <[email protected]>
Copy link
Contributor

@jan--f jan--f left a comment

Choose a reason for hiding this comment

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

Thanks, I think all this makes sense and the changes look good.

Two questions:
Should we add the startsAt default in the help text?
Should we mention this in the release notes? This does change user visible behavior after all.

cc @simonpasquier @roidelapluie

@roidelapluie
Copy link
Member

Yes but the release notes are only crafted during the releases, not in individual PR's.

@roidelapluie roidelapluie merged commit 5ddf9e2 into prometheus:main Oct 18, 2021
@roidelapluie
Copy link
Member

Thank you!

@roidelapluie
Copy link
Member

Note that tests are welcome! Functions do not need to be exported to be tested.

@roidelapluie
Copy link
Member

Two questions: Should we add the startsAt default in the help text?

I don't think it is really needed.

@nekketsuuu nekketsuuu deleted the nekketsuuu-silence-add-duration branch October 18, 2021 13:53
markoposavec pushed a commit to markoposavec/alertmanager that referenced this pull request Nov 4, 2021
…us#2741)

endsAt should be calculated from startsAt, not from the current time

Signed-off-by: nekketsuuu <[email protected]>
markoposavec pushed a commit to markoposavec/alertmanager that referenced this pull request Nov 4, 2021
…us#2741)

endsAt should be calculated from startsAt, not from the current time

Signed-off-by: nekketsuuu <[email protected]>
Signed-off-by: Marko Posavec <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants