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

Sleep after over-long iteration #365

Merged
merged 1 commit into from
Jul 7, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/main/java/org/datadog/jmxfetch/App.java
Original file line number Diff line number Diff line change
@@ -472,14 +472,15 @@ void start() {
try {
long loopPeriod = appConfig.getCheckPeriod();
long sleepPeriod = loopPeriod - duration;
if (sleepPeriod <= 0) {
if (sleepPeriod < loopPeriod / 2) {
log.debug(
"The collection cycle took longer that the configured check period,"
+ " the next cycle will start immediatly");
+ " the next cycle will be delayed");
sleepPeriod = loopPeriod / 2;
} else {
log.debug("Sleeping for " + sleepPeriod + " ms.");
Thread.sleep(sleepPeriod);
}
Thread.sleep(sleepPeriod);
Copy link
Member

Choose a reason for hiding this comment

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

I think Thread.sleep() for a negative value will throw a java.lang.IllegalArgumentException. Please bear in mind that some instances might take longer to collect than the actual loopPeriod. That would result in a negative sleepPeriod that could trigger that exception.

The goal of this part of the code was to ensure we didn't skip iterations (and sleep) when the duration of the collection exceeded the check period, I think this change could change that expected behavior.

Maybe I'm missing something, feel free to ping me (or drop it in here) if you want to provide more context or discuss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@truthbk

I think Thread.sleep() for a negative value will throw a java.lang.IllegalArgumentException. Please bear in mind that some instances might take longer to collect than the actual loopPeriod. That would result in a negative sleepPeriod that could trigger that exception.

sleepPeriod will not be negative, because it is assigned a new value a few lines above.

The goal of this part of the code was to ensure we didn't skip iterations (and sleep) when the duration of the collection exceeded the check period, I think this change could change that expected behavior.

True, but when the slowness is caused by the bean update process, two metric collections happen within a short time, sending two sets of metrics within one period. This is useless for gauges, skews histograms a bit and can cause confusing spikes in reported counter rates (small change over tiny time interval yields very high rate). Given that this behavior is actually relatively new, we might take a step back until a better solution is worked out. WDYT?

} catch (InterruptedException e) {
log.warn(e.getMessage(), e);
}