-
Notifications
You must be signed in to change notification settings - Fork 12
*: add metrics-min-interval to improve the chance of collecting top-heavy metrics #490
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
Conversation
yibin87
left a comment
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.
LGTM
kaaaaaaang
left a comment
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.
rest LGT @mornyx
| subdirRaw = "raw" | ||
| maxQueryRange = 120 * 60 // 120min | ||
| minQueryRange = 5 * 60 // 5min | ||
| minQueryRange = 1 * 60 // 1min |
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 think it is not used anymore
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.
This is the minimum protection value. Because if the value is lower than this, the proportion of metadata will be high. If the segmentation is too small, it would be meaningless.
| cmd.Flags().IntVar(&cOpt.MetricsLimit, "metricslimit", 10000, "metric size limit of single request, specified in series*hour per request") | ||
| cmd.Flags().StringVar(&metricsConf, "metricsconfig", "", "config file of metricsfilter") | ||
| cmd.Flags().StringSliceVar(&labels, "metricslabel", nil, "only collect metrics that match labels") | ||
| cmd.Flags().IntVar(&cOpt.MetricsMinInterval, "metrics-min-interval", 120, "the minimum interval of a single request in seconds") |
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.
is 120s the new default value?
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.
Yes, if you think it's best to keep it the same as before, I can change it to 5 minutes.
|
PTAL @mornyx |
What problem does this PR solve?
This is a small part extracted from PR #123.
What is changed and how it works?
If pulling top-heavy metrics fails, you can consider reducing the time range for each request. The
metrics-low-priorityflag is used to reduce the time range of each request.Optimized connection pool management (use default http.Client)
Check List
Tests
Before this PR
In this case, one file every 5 minutes, and the following three tests only get 5 minutes of data for each

After this PR
with

--metrics-min-interval "300"In this case, one file every 5 minutes, and the following three tests get 10 minutes of data for each
with default

metrics-min-interval(120)In this case, one file every 2 minutes, and we have 3 runs, the first with 12 minutes of data (6 files), and the next two tests with 20 minutes of data (10 files)
Code changes
Side effects
Related changes