Fix flawed logic in calculation of top query metrics collection window #44163
+32
−4
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.
Description
Issue area -1
Based on current implementation the EffectiveLookbackTime is calculated as
2 x cfg.ControllerConfig.CollectionIntervalwhich is wrong.Below are the default values,
With the existing logic the EffectiveLookbackTime will be calculated as 20 seconds (2 x 10 =20), and this will not cover the previous 1 minute collection window of top_query.
Issue area -2
And the logic to determine the window slot needs to be fixed too.
The collection latency (mostly a few milliseconds) is not taken into account when the next check is done. As a result when the top_query is configured to collect at 60 seconds, practically the collection is done only by next cycle at 80 seconds.
Fix Proposal
The logic should have been
2 X TopQueryCollection's CollectionIntervalto effectively cover the collection period.And to determine the window,
current check:
if s.lastExecutionTimestamp.Add(s.config.TopQueryCollection.CollectionInterval).After(time.Now()) {...}proposed fix:
if int(math.Ceil(time.Since(s.lastExecutionTimestamp).Seconds())) < int(s.config.TopQueryCollection.CollectionInterval.Seconds()) {...}Link to tracking issue
Fixes #44162
Testing
Documentation