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

Configurable sliding window #122

Merged
merged 5 commits into from
Aug 7, 2020

Conversation

whoawhoawhoa
Copy link
Contributor

Resolve issue with configurable sliding window for summaries.

@whoawhoawhoa
Copy link
Contributor Author

Tests went OK on my local machine. I assume it is kind of floating bug.

@coveralls
Copy link

coveralls commented Aug 6, 2020

Coverage Status

Coverage decreased (-0.1%) to 65.27% when pulling 8ee5c6e on whoawhoawhoa:configurable-sliding-window into e898962 on fstab:master.

@@ -211,6 +213,10 @@ func applyImportDefaults(metricConfig *MetricConfig, defaults DefaultConfig) {
if metricConfig.Type == "summary" && len(metricConfig.Quantiles) == 0 {
metricConfig.Quantiles = defaults.Quantiles
}
if metricConfig.Type == "summary" && metricConfig.MaxAge == 0 {
fmt.Print("set max age default")
Copy link
Owner

Choose a reason for hiding this comment

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

please remove :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry, didn't notice that one.
Removed now.

@fstab
Copy link
Owner

fstab commented Aug 6, 2020

Looking good, thanks a lot. There's a fmt.Print() message left over, please remove it before I merge.

BTW the Windows test on AppVeyor failed because in one of the file tailer tests a goroutine did not shut down properly. This has been flaky for a long time and has nothing to do with your PR. I'd love to know how to fix this though.

@fstab fstab merged commit 568cfe5 into fstab:master Aug 7, 2020
@fstab
Copy link
Owner

fstab commented Aug 7, 2020

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Summary percentile sliding window
3 participants