diff --git a/app/services/reporting_events/metric_registry.rb b/app/services/reporting_events/metric_registry.rb index 10b0a589a..7e67efc90 100644 --- a/app/services/reporting_events/metric_registry.rb +++ b/app/services/reporting_events/metric_registry.rb @@ -1,6 +1,12 @@ -# Raw reporting events and rollup rows do not share a single metric namespace. -# This registry keeps the mapping aligned across write and read paths. +# Raw reporting events and rollup rows do not share a single metric namespace; this registry keeps write and read paths aligned. module ReportingEvents::MetricRegistry + SUMMARY_METRICS = { + resolutions_count: :resolved_conversations_count, + avg_resolution_time: :avg_resolution_time, + avg_first_response_time: :avg_first_response_time, + reply_time: :avg_reply_time + }.freeze + EVENT_METRICS = { 'conversation_resolved' => lambda do |values| { @@ -18,36 +24,13 @@ module ReportingEvents::MetricRegistry conversations_count: { aggregate: :count }.freeze, incoming_messages_count: { aggregate: :count }.freeze, outgoing_messages_count: { aggregate: :count }.freeze, - avg_first_response_time: { - raw_event_name: :first_response, - rollup_metric: :first_response, - aggregate: :average - }.freeze, - avg_resolution_time: { - raw_event_name: :conversation_resolved, - rollup_metric: :resolution_time, - aggregate: :average - }.freeze, - reply_time: { - raw_event_name: :reply_time, - rollup_metric: :reply_time, - aggregate: :average - }.freeze, - resolutions_count: { - raw_event_name: :conversation_resolved, - rollup_metric: :resolutions_count, - aggregate: :count - }.freeze, - bot_resolutions_count: { - raw_event_name: :conversation_bot_resolved, - rollup_metric: :bot_resolutions_count, - aggregate: :count - }.freeze, + avg_first_response_time: { raw_event_name: :first_response, rollup_metric: :first_response, aggregate: :average }.freeze, + avg_resolution_time: { raw_event_name: :conversation_resolved, rollup_metric: :resolution_time, aggregate: :average }.freeze, + reply_time: { raw_event_name: :reply_time, rollup_metric: :reply_time, aggregate: :average }.freeze, + resolutions_count: { raw_event_name: :conversation_resolved, rollup_metric: :resolutions_count, aggregate: :count }.freeze, + bot_resolutions_count: { raw_event_name: :conversation_bot_resolved, rollup_metric: :bot_resolutions_count, aggregate: :count }.freeze, bot_handoffs_count: { - raw_event_name: :conversation_bot_handoff, - rollup_metric: :bot_handoffs_count, - aggregate: :count, - raw_count_strategy: :distinct_conversation + raw_event_name: :conversation_bot_handoff, rollup_metric: :bot_handoffs_count, aggregate: :count, raw_count_strategy: :distinct_conversation }.freeze }.freeze @@ -101,17 +84,21 @@ module ReportingEvents::MetricRegistry report_metric(metric)&.dig(:raw_event_name) end - def count_metric(count) + def summary_metrics + SUMMARY_METRICS.map do |metric_name, summary_key| + report_metric(metric_name).merge(metric_name: metric_name, summary_key: summary_key) + end + end + + private_class_method def count_metric(count) { count: count, sum_value: 0, sum_value_business_hours: 0 } end - private_class_method :count_metric - def duration_metric(values) + private_class_method def duration_metric(values) { count: values[:count], sum_value: values[:sum_value], sum_value_business_hours: values[:sum_value_business_hours] } end - private_class_method :duration_metric end diff --git a/app/services/reports/data_source.rb b/app/services/reports/data_source.rb index d93bed875..61b4c6101 100644 --- a/app/services/reports/data_source.rb +++ b/app/services/reports/data_source.rb @@ -90,6 +90,10 @@ class Reports::DataSource report_metric&.dig(:raw_count_strategy) end + def summary_metrics + @summary_metrics ||= ReportingEvents::MetricRegistry.summary_metrics + end + def use_business_hours? ActiveModel::Type::Boolean.new.cast(business_hours) end diff --git a/app/services/reports/raw_data_source.rb b/app/services/reports/raw_data_source.rb index 23ccf0764..5abcd7176 100644 --- a/app/services/reports/raw_data_source.rb +++ b/app/services/reports/raw_data_source.rb @@ -105,37 +105,27 @@ class Reports::RawDataSource < Reports::DataSource def merge_summary_results(metric_results, conversation_counts) (metric_results.keys | conversation_counts.keys).each_with_object({}) do |dimension_id, results| record = metric_results[dimension_id] - - results[dimension_id] = { - conversations_count: conversation_counts[dimension_id].to_i, - resolved_conversations_count: record&.resolved_count.to_i, - avg_resolution_time: record&.avg_resolution_time, - avg_first_response_time: record&.avg_first_response_time, - avg_reply_time: record&.avg_reply_time - } + results[dimension_id] = summary_attributes_for(record, conversation_counts[dimension_id]) end end def summary_select_fields - [ - "#{summary_group_by_key} as #{summary_index_key}", - count_select(:resolutions_count, :resolved_count), - average_select(:avg_resolution_time, :avg_resolution_time), - average_select(:avg_first_response_time, :avg_first_response_time), - average_select(:reply_time, :avg_reply_time) - ] + ["#{summary_group_by_key} as #{summary_index_key}"] + summary_metrics.map { |definition| summary_select_field(definition) } end - def count_select(metric_name, alias_name) - "COUNT(CASE WHEN name = '#{raw_metric_event_name(metric_name)}' THEN 1 END) as #{alias_name}" + def summary_select_field(definition) + if definition[:aggregate] == :count + "COUNT(CASE WHEN name = '#{definition[:raw_event_name]}' THEN 1 END) as #{definition[:summary_key]}" + else + "AVG(CASE WHEN name = '#{definition[:raw_event_name]}' THEN #{average_value_key} END) as #{definition[:summary_key]}" + end end - def average_select(metric_name, alias_name) - "AVG(CASE WHEN name = '#{raw_metric_event_name(metric_name)}' THEN #{average_value_key} END) as #{alias_name}" - end - - def raw_metric_event_name(metric_name) - ReportingEvents::MetricRegistry.raw_event_name_for(metric_name) + def summary_attributes_for(record, conversations_count = 0) + summary_metrics.each_with_object({ conversations_count: conversations_count.to_i }) do |definition, attributes| + value = record&.public_send(definition[:summary_key]) + attributes[definition[:summary_key]] = definition[:aggregate] == :count ? value.to_i : value + end end def summary_group_by_key diff --git a/app/services/reports/rollup_data_source.rb b/app/services/reports/rollup_data_source.rb index 4cd75e729..765afba8a 100644 --- a/app/services/reports/rollup_data_source.rb +++ b/app/services/reports/rollup_data_source.rb @@ -90,38 +90,44 @@ class Reports::RollupDataSource < Reports::DataSource end def summary_select_fields + ['dimension_id'] + summary_metrics.flat_map { |definition| summary_select_fields_for_metric(definition) } + end + + def summary_select_fields_for_metric(definition) + return [sum_count_select(definition[:rollup_metric], definition[:summary_key])] if definition[:aggregate] == :count + [ - 'dimension_id', - sum_count_select(:resolutions_count, :resolved_count), - sum_count_select(:avg_resolution_time, :resolution_count), - sum_value_select(:avg_resolution_time, :resolution_sum_value), - sum_count_select(:avg_first_response_time, :first_response_count), - sum_value_select(:avg_first_response_time, :first_response_sum_value), - sum_count_select(:reply_time, :reply_count), - sum_value_select(:reply_time, :reply_sum_value) + sum_count_select(definition[:rollup_metric], summary_count_alias(definition)), + sum_value_select(definition[:rollup_metric], summary_sum_alias(definition)) ] end - def sum_count_select(metric_name, alias_name) - "SUM(CASE WHEN metric = '#{rollup_metric_name(metric_name)}' THEN count ELSE 0 END) as #{alias_name}" + def sum_count_select(rollup_metric_name, alias_name) + "SUM(CASE WHEN metric = '#{rollup_metric_name}' THEN count ELSE 0 END) as #{alias_name}" end - def sum_value_select(metric_name, alias_name) - "SUM(CASE WHEN metric = '#{rollup_metric_name(metric_name)}' THEN #{rollup_value_column} ELSE 0 END) as #{alias_name}" - end - - def rollup_metric_name(metric_name) - ReportingEvents::MetricRegistry.rollup_metric_for(metric_name) + def sum_value_select(rollup_metric_name, alias_name) + "SUM(CASE WHEN metric = '#{rollup_metric_name}' THEN #{rollup_value_column} ELSE 0 END) as #{alias_name}" end def summary_attributes_for(row, conversations_count = 0) - { - conversations_count: conversations_count.to_i, - resolved_conversations_count: row&.resolved_count.to_i, - avg_resolution_time: average_from(row&.resolution_sum_value, row&.resolution_count), - avg_first_response_time: average_from(row&.first_response_sum_value, row&.first_response_count), - avg_reply_time: average_from(row&.reply_sum_value, row&.reply_count) - } + summary_metrics.each_with_object({ conversations_count: conversations_count.to_i }) do |definition, attributes| + attributes[definition[:summary_key]] = summary_value_for(row, definition) + end + end + + def summary_value_for(row, definition) + return row&.public_send(definition[:summary_key]).to_i if definition[:aggregate] == :count + + average_from(row&.public_send(summary_sum_alias(definition)), row&.public_send(summary_count_alias(definition))) + end + + def summary_count_alias(definition) + "#{definition[:summary_key]}_count" + end + + def summary_sum_alias(definition) + "#{definition[:summary_key]}_sum_value" end def dimension_id_for_rollup diff --git a/spec/services/reporting_events/metric_registry_spec.rb b/spec/services/reporting_events/metric_registry_spec.rb index 22357330c..3c73a4cb8 100644 --- a/spec/services/reporting_events/metric_registry_spec.rb +++ b/spec/services/reporting_events/metric_registry_spec.rb @@ -175,4 +175,41 @@ RSpec.describe ReportingEvents::MetricRegistry do expect(described_class.raw_event_name_for(:bot_resolutions_count)).to eq(:conversation_bot_resolved) end end + + describe '.summary_metrics' do + it 'returns the registry-backed summary metric definitions' do + expect(described_class.summary_metrics).to eq( + [ + { + metric_name: :resolutions_count, + summary_key: :resolved_conversations_count, + aggregate: :count, + raw_event_name: :conversation_resolved, + rollup_metric: :resolutions_count + }, + { + metric_name: :avg_resolution_time, + summary_key: :avg_resolution_time, + aggregate: :average, + raw_event_name: :conversation_resolved, + rollup_metric: :resolution_time + }, + { + metric_name: :avg_first_response_time, + summary_key: :avg_first_response_time, + aggregate: :average, + raw_event_name: :first_response, + rollup_metric: :first_response + }, + { + metric_name: :reply_time, + summary_key: :avg_reply_time, + aggregate: :average, + raw_event_name: :reply_time, + rollup_metric: :reply_time + } + ] + ) + end + end end diff --git a/spec/services/reports/data_source_spec.rb b/spec/services/reports/data_source_spec.rb index 3be3c0629..3f6e0a545 100644 --- a/spec/services/reports/data_source_spec.rb +++ b/spec/services/reports/data_source_spec.rb @@ -60,6 +60,55 @@ RSpec.describe Reports::DataSource do end end + describe 'summary select fields' do + let(:account) { create(:account, reporting_timezone: 'Etc/UTC') } + let(:context) do + { + account: account, + metric: nil, + dimension_type: 'inbox', + dimension_id: nil, + scope: nil, + range: 1.day.ago.beginning_of_day...Time.current.end_of_day, + group_by: 'day', + timezone: 'UTC', + timezone_offset: '0', + business_hours: false + } + end + + it 'derives raw summary selects from registry summary metrics' do + source = Reports::RawDataSource.new(**context) + + expect(source.send(:summary_select_fields)).to eq( + [ + 'inbox_id as inbox_id', + "COUNT(CASE WHEN name = 'conversation_resolved' THEN 1 END) as resolved_conversations_count", + "AVG(CASE WHEN name = 'conversation_resolved' THEN value END) as avg_resolution_time", + "AVG(CASE WHEN name = 'first_response' THEN value END) as avg_first_response_time", + "AVG(CASE WHEN name = 'reply_time' THEN value END) as avg_reply_time" + ] + ) + end + + it 'derives rollup summary selects from registry summary metrics' do + source = Reports::RollupDataSource.new(**context) + + expect(source.send(:summary_select_fields)).to eq( + [ + 'dimension_id', + "SUM(CASE WHEN metric = 'resolutions_count' THEN count ELSE 0 END) as resolved_conversations_count", + "SUM(CASE WHEN metric = 'resolution_time' THEN count ELSE 0 END) as avg_resolution_time_count", + "SUM(CASE WHEN metric = 'resolution_time' THEN sum_value ELSE 0 END) as avg_resolution_time_sum_value", + "SUM(CASE WHEN metric = 'first_response' THEN count ELSE 0 END) as avg_first_response_time_count", + "SUM(CASE WHEN metric = 'first_response' THEN sum_value ELSE 0 END) as avg_first_response_time_sum_value", + "SUM(CASE WHEN metric = 'reply_time' THEN count ELSE 0 END) as avg_reply_time_count", + "SUM(CASE WHEN metric = 'reply_time' THEN sum_value ELSE 0 END) as avg_reply_time_sum_value" + ] + ) + end + end + describe 'adapter contract' do let(:account) { create(:account, reporting_timezone: 'Etc/UTC') } let(:user1) { create(:user) }