Make the history limit a configurable flag #308
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brian-brazil
reviewed
Apr 2, 2018
main.go
Outdated
| @@ -50,6 +50,7 @@ var ( | |||
| listenAddress = kingpin.Flag("web.listen-address", "The address to listen on for HTTP requests.").Default(":9115").String() | |||
| timeoutOffset = kingpin.Flag("timeout-offset", "Offset to subtract from timeout in seconds.").Default("0.5").Float64() | |||
| configCheck = kingpin.Flag("config.check", "If true validate the config file and then exit.").Default().Bool() | |||
| historyLimit = kingpin.Flag("historyLimit", "The maximum amount of items to keep in the history.").Default("100").Int() | |||
There was a problem hiding this comment.
Why not uint? This also needs a gofmt.
There was a problem hiding this comment.
I went with int because I thought it looked cleaner to avoid the having a cast on history.go:54 and negative values will be set to 100 by history.go:40. I have no problem changing it though.
Formatting issue was because I tried to use the github merge ui. Will fix with the uint commit.
brian-brazil
reviewed
Apr 3, 2018
history.go
Outdated
| } | ||
|
|
||
| // Add a result to the history. | ||
| func (rh *resultHistory) Add(moduleName, target, debugOutput string, success bool) { | ||
| rh.mu.Lock() | ||
| defer rh.mu.Unlock() | ||
|
|
||
| if rh.maxResults < 1 { |
There was a problem hiding this comment.
You don't need this now.
brian-brazil
reviewed
Apr 3, 2018
main.go
Outdated
| @@ -50,6 +50,7 @@ var ( | |||
| listenAddress = kingpin.Flag("web.listen-address", "The address to listen on for HTTP requests.").Default(":9115").String() | |||
| timeoutOffset = kingpin.Flag("timeout-offset", "Offset to subtract from timeout in seconds.").Default("0.5").Float64() | |||
| configCheck = kingpin.Flag("config.check", "If true validate the config file and then exit.").Default().Bool() | |||
| historyLimit = kingpin.Flag("historyLimit", "The maximum amount of items to keep in the history.").Default("100").Uint() | |||
|
@brian-brazil Any other changes you want to see? |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change replaces the hardcoded limit on the number of elements stored in the log history with a configurable value that can be set as a runtime argument. The the old value of 100 elements will be use by default either when the argument is left off or the argument is out of range (less than 1).
The driving use case for this is when there are over 100 endpoints queried, some of them will be removed almost immediately from the logs.