-
Notifications
You must be signed in to change notification settings - Fork 896
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
Find peaks/troughs of memory usage during rollup #16224
Conversation
@miq-bot add_labels bug, providers/metrics |
@gtanzillo @Ladas please help review this. thanks |
Checked commit jameswnl@683c1e8 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 |
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.
Hey @jameswnl can you elaborate a bit on what cased the issue and how this fixes it? The metric rollup code is pretty opaque to me so its hard to tell what the problem was.
BURST_COLS.each do |col| | ||
value = rt.send(col) | ||
rollup_burst(col, new_perf[:min_max], rt.timestamp, value) | ||
end |
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.
What did moving this section out of the loop above help with?
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.
It was embedded in the loop of Metric::Capture.capture_cols.each do |col|
which wouldn't cover any derived*
columns.
This logic is doing burst-rollup and should be targeting BURST_COL
. It was ok before because BURST_COL
was a subset of Metric::Capture.capture_cols
, but now it is not.
.maximum(:derived_memory_used) | ||
end | ||
perfs = VimPerformanceAnalysis.find_perf_for_time_period(self, "daily", :end_date => Time.now.utc, :days => Metric::LongTermAverages::AVG_DAYS) | ||
perfs.collect(&:abs_max_derived_memory_used_value).compact.max |
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.
What about the old code was causing it to not pick the maximum 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.
The old code was getting the max of daily averages, not the realtime peaks/troughs which were lost during rollup. (The burst-rollup part change is now determining and storing these during rollup time)
Awesome thanks @jameswnl ! |
Part of the fix for https://bugzilla.redhat.com/show_bug.cgi?id=1468634.
This PR covers the memory usage.
(#16195 covers CPU)