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.

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

Add CLI args for snapshot intervals #2849

Merged
merged 3 commits into from
Mar 18, 2022
Merged

Add CLI args for snapshot intervals #2849

merged 3 commits into from
Mar 18, 2022

Conversation

sed-i
Copy link
Contributor

@sed-i sed-i commented Mar 15, 2022

This PR adds two new CLI args to support custom snapshot intervals for nflogs and silences.

Fixes #2660.

@sed-i sed-i force-pushed the feature/snapshot_interval_cli_args branch from e7e6031 to fb96525 Compare March 15, 2022 20:32
simonpasquier
simonpasquier reviewed Mar 17, 2022
View reviewed changes
cmd/alertmanager/main.go Outdated Show resolved Hide resolved
cmd/alertmanager/main.go Outdated Show resolved Hide resolved
@gotjosh
Copy link
Member

gotjosh commented Mar 17, 2022

Overall, this looks good. Do you have a use-case for separating (and having to add two new flags, albeit optional), or can we stick with just one for both? I can definitely see the use case for changing the interval but not why you would have separate ones.

@sed-i
Copy link
Contributor Author

sed-i commented Mar 17, 2022

Overall, this looks good. Do you have a use-case for separating (and having to add two new flags, albeit optional), or can we stick with just one for both? I can definitely see the use case for changing the interval but not why you would have separate ones.

For my use-case we can stick with one flag only. It makes sense to me. I wasn't sure the two always go hand-in-hand.

Some ideas for a name for a single flag:

  1. snapshot.interval
  2. maintenance.interval
  3. maintenance.gc-interval

@simonpasquier
Copy link
Member

That's a good point @gotjosh :)
I've don't have a strong feeling but since --data.retention already applies to silences and nflogs, it would make sense to have --data.maintenance-interval. What do you think?

@gotjosh
Copy link
Member

gotjosh commented Mar 18, 2022

--data.maintenance-interval

👍 on the idea of this name.

@sed-i sed-i force-pushed the feature/snapshot_interval_cli_args branch from ce1fd7b to 99f48e3 Compare March 18, 2022 13:47
@sed-i
Copy link
Contributor Author

sed-i commented Mar 18, 2022

Changed to data.maintenance-interval.
@gotjosh would you be able to rerun the failed test? Seems like an unrelated timeout error.

@sed-i sed-i marked this pull request as ready for review March 18, 2022 14:02
simonpasquier
simonpasquier approved these changes Mar 18, 2022
View reviewed changes
Copy link
Member

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

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

lgtm

gotjosh
gotjosh approved these changes Mar 18, 2022
View reviewed changes
Copy link
Member

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

Sadly, I can't kick off the CI but @simonpasquier was here to do so.

LGTM.

@simonpasquier simonpasquier merged commit dd63d04 into prometheus:main Mar 18, 2022
@simonpasquier
Copy link
Member

thanks!

@sed-i sed-i deleted the feature/snapshot_interval_cli_args branch March 18, 2022 16:14
Maissacrement pushed a commit to Maissacrement/alertmanager that referenced this pull request Mar 20, 2022
@sed-i @simonpasquier @Maissacrement
Add CLI args for snapshot intervals (prometheus#2849)
5a0feba 
* Add CLI args for snapshot intervals

Signed-off-by: sed-i <[email protected]>

* Apply suggestions from code review

Co-authored-by: Simon Pasquier <[email protected]>
Signed-off-by: sed-i <[email protected]>

* use same flag for silences and nflogs intervals

Signed-off-by: sed-i <[email protected]>

Co-authored-by: Simon Pasquier <[email protected]>
Signed-off-by: delar <[email protected]>
@gotjosh gotjosh mentioned this pull request Jan 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonpasquier simonpasquier simonpasquier approved these changes

@gotjosh gotjosh gotjosh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add command line argument for silences snapshot period
3 participants
@sed-i @gotjosh @simonpasquier