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

Add Timeout config option for metricseter to exit #1175

Merged
merged 1 commit into from Mar 21, 2016

Conversation

Projects
None yet
2 participants
@ruflin
Collaborator

ruflin commented Mar 17, 2016

The fetch on each metricseter is called based on the predefined ticker without taking delays of fetching into account. This can lead to the issue, that one metricset adds more requests before the last one was ended. The timeout variable was introduced. This allows to configure when the metricseter should exist. By default it is set to period, but it can also be selected larger or smaller then period based on the specific needs.

  • Add timeout config option which is by default set to period
  • Implement timeout for redis and mysql
@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Mar 17, 2016

Collaborator

@tsg @andrewkroh After starting the implementation for a Cancel interface function I decided to go with the timeout, as the Cancel function introduced even more movable parts like tracking which request belongs to which Fetch call etc.

Collaborator

ruflin commented Mar 17, 2016

@tsg @andrewkroh After starting the implementation for a Cancel interface function I decided to go with the timeout, as the Cancel function introduced even more movable parts like tracking which request belongs to which Fetch call etc.

Show outdated Hide outdated metricbeat/helper/interfacer.go Outdated
Show outdated Hide outdated metricbeat/module/redis/info/info.go Outdated
Show outdated Hide outdated metricbeat/helper/module.go Outdated
@@ -144,26 +152,18 @@ func (m *Module) Run(period time.Duration, b *beat.Beat) {
go m.publishing(b)
// Start fetching metrics
// The frequency is always based on the ticker interval. No delays are taken into account.
// Each Metricset must ensure to exit after timeout.

This comment has been minimized.

@tsg

tsg Mar 21, 2016

Collaborator

Like @andrewkroh was suggesting, could we keep a counter with the number of goroutines started but not exit? We can then stop creating new ones if the previous 60 or so didn't exit yet, and log an error instead. This is to protect against "too many open files" and excessive memory usage. Good to have this in a follow up PR if you prefer.

@tsg

tsg Mar 21, 2016

Collaborator

Like @andrewkroh was suggesting, could we keep a counter with the number of goroutines started but not exit? We can then stop creating new ones if the previous 60 or so didn't exit yet, and log an error instead. This is to protect against "too many open files" and excessive memory usage. Good to have this in a follow up PR if you prefer.

This comment has been minimized.

@ruflin

ruflin Mar 21, 2016

Collaborator

Good idea. We should have this limit probably per metricset and not module so in case on metricset does not behave well, it does not affect others. Should we make this configurable and have a "reasonable" default value?

I prefer to do this in a second PR. There we should also introduce some metrics on this for httpprof.

@ruflin

ruflin Mar 21, 2016

Collaborator

Good idea. We should have this limit probably per metricset and not module so in case on metricset does not behave well, it does not affect others. Should we make this configurable and have a "reasonable" default value?

I prefer to do this in a second PR. There we should also introduce some metrics on this for httpprof.

This comment has been minimized.

@tsg

tsg Mar 21, 2016

Collaborator

Sounds good!

@tsg

tsg Mar 21, 2016

Collaborator

Sounds good!

This comment has been minimized.

@tsg

tsg Mar 21, 2016

Collaborator

IMHO a high hard coded value is enough to start with. +1 to have it per metricset and to have expvars for it.

@tsg

tsg Mar 21, 2016

Collaborator

IMHO a high hard coded value is enough to start with. +1 to have it per metricset and to have expvars for it.

This comment has been minimized.

@ruflin

ruflin Mar 21, 2016

Collaborator
@ruflin

ruflin Mar 21, 2016

Collaborator
Add Timeout config option for metricseter to exit
The fetch on each metricseter is called based on the predefined ticker without taking delays of fetching into account. This can lead to the issue, that one metricset adds more requests before the last one was ended. The timeout variable was introduced. This allows to configure when the metricseter should exist. By default it is set to period, but it can also be selected larger or smaller then period based on the specific needs.

* Add timeout config option which is by default set to period
* Implement timeout for redis and mysql

tsg added a commit that referenced this pull request Mar 21, 2016

Merge pull request #1175 from ruflin/mb-timeout
Add Timeout config option for metricseter to exit

@tsg tsg merged commit 739e5cf into elastic:master Mar 21, 2016

3 checks passed

CLA Commit author has signed the CLA
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ruflin ruflin deleted the ruflin:mb-timeout branch Mar 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment