-
Notifications
You must be signed in to change notification settings - Fork 897
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
Replace remove duplicate timestamp by sql version for chargeback #17538
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,15 +14,18 @@ def self.for_report(cb_class, options, region) | |
|
||
next unless options.include_metrics? | ||
|
||
records = base_rollup.where(:timestamp => query_start_time...query_end_time, :capture_interval_name => 'hourly') | ||
records = MetricRollup.where(:timestamp => query_start_time...query_end_time, :capture_interval_name => 'hourly') | ||
records = cb_class.where_clause(records, options, region) | ||
records = Metric::Helper.remove_duplicate_timestamps(records) | ||
records = uniq_timestamp_record_map(records, options.group_by_tenant?) | ||
|
||
next if records.empty? | ||
_log.info("Found #{records.length} records for time range #{[query_start_time, query_end_time].inspect}") | ||
|
||
# we are building hash with grouped calculated values | ||
# values are grouped by resource_id and timestamp (query_start_time...query_end_time) | ||
options.group_with(records).each_value do |metric_rollup_records| | ||
|
||
records.each_value do |rollup_record_ids| | ||
metric_rollup_records = base_rollup.where(:id => rollup_record_ids).to_a | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you're introducing an N+1 here. Is this really what you want? |
||
consumption = ConsumptionWithRollups.new(metric_rollup_records, query_start_time, query_end_time) | ||
yield(consumption) unless consumption.consumed_hours_in_interval.zero? | ||
end | ||
|
@@ -43,5 +46,29 @@ def self.base_rollup_scope | |
end | ||
|
||
private_class_method :base_rollup_scope | ||
|
||
def self.uniq_timestamp_record_map(report_scope, group_by_tenant = false) | ||
main_select = MetricRollup.select(:id, :resource_id).arel.ast.to_sql | ||
.gsub("SELECT", "DISTINCT ON (resource_type, resource_id, timestamp)") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. have you tried I'd almost prefer raw sql over sql generation and munging. |
||
.gsub(/ FROM.*$/, '') | ||
|
||
query = report_scope.select(main_select) | ||
.order(:resource_type, :resource_id, :timestamp) | ||
.order("created_on DESC") | ||
|
||
rows = ActiveRecord::Base.connection.select_rows(query.to_sql) | ||
|
||
if group_by_tenant | ||
vms = Hash[Vm.where(:id => rows.map(&:second)).pluck(:id, :tenant_id)] | ||
end | ||
|
||
rows.each_with_object({}) do |(id, resource_id), result| | ||
resource_id = vms[resource_id] if group_by_tenant | ||
result[resource_id] ||= [] | ||
result[resource_id] << id | ||
end | ||
end | ||
|
||
private_class_method :uniq_timestamp_record_map | ||
end | ||
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.
It seems like this will result in a change in behavior, maybe it's ok but I want to acknowledge it here - the original code in
remove_duplicate_timestamps
merges all metric rows it finds with the same timestamp - https://github.com/manageiq/manageiq/blob/b6bd309650ee46fedcdf6d9e00af4f06685bd7f8/app/models/metric/helper.rb#L139. This code will keep one of them and discard the other.