Commit 71d4bf72 authored by Timothy Andrew's avatar Timothy Andrew

Implement (some) comments from @DouweM's review.

- Move things common to `Issue` and `MergeRequest` into `Issuable`
- Move more database-specific functions into `Gitlab::Database`
- Indentation changes and other minor refactorings.
parent 6f194e28
...@@ -33,6 +33,8 @@ module Issuable ...@@ -33,6 +33,8 @@ module Issuable
has_many :labels, through: :label_links has_many :labels, through: :label_links
has_many :todos, as: :target, dependent: :destroy has_many :todos, as: :target, dependent: :destroy
has_one :metrics
validates :author, presence: true validates :author, presence: true
validates :title, presence: true, length: { within: 0..255 } validates :title, presence: true, length: { within: 0..255 }
...@@ -82,6 +84,7 @@ module Issuable ...@@ -82,6 +84,7 @@ module Issuable
acts_as_paranoid acts_as_paranoid
after_save :update_assignee_cache_counts, if: :assignee_id_changed? after_save :update_assignee_cache_counts, if: :assignee_id_changed?
after_save :record_metrics
def update_assignee_cache_counts def update_assignee_cache_counts
# make sure we flush the cache for both the old *and* new assignee # make sure we flush the cache for both the old *and* new assignee
...@@ -287,4 +290,9 @@ module Issuable ...@@ -287,4 +290,9 @@ module Issuable
def can_move?(*) def can_move?(*)
false false
end end
def record_metrics
metrics = self.metrics || create_metrics
metrics.record!
end
end end
class CycleAnalytics class CycleAnalytics
include Gitlab::Database::Median include Gitlab::Database::Median
include Gitlab::Database::DateTime
def initialize(project, from:) def initialize(project, from:)
@project = project @project = project
...@@ -12,58 +13,64 @@ class CycleAnalytics ...@@ -12,58 +13,64 @@ class CycleAnalytics
end end
def issue def issue
calculate_metric!(:issue, calculate_metric(:issue,
TableReferences.issues[:created_at], TableReferences.issues[:created_at],
[TableReferences.issue_metrics[:first_associated_with_milestone_at], [TableReferences.issue_metrics[:first_associated_with_milestone_at],
TableReferences.issue_metrics[:first_added_to_board_at]]) TableReferences.issue_metrics[:first_added_to_board_at]])
end end
def plan def plan
calculate_metric!(:plan, calculate_metric(:plan,
[TableReferences.issue_metrics[:first_associated_with_milestone_at], [TableReferences.issue_metrics[:first_associated_with_milestone_at],
TableReferences.issue_metrics[:first_added_to_board_at]], TableReferences.issue_metrics[:first_added_to_board_at]],
TableReferences.issue_metrics[:first_mentioned_in_commit_at]) TableReferences.issue_metrics[:first_mentioned_in_commit_at])
end end
def code def code
calculate_metric!(:code, calculate_metric(:code,
TableReferences.issue_metrics[:first_mentioned_in_commit_at], TableReferences.issue_metrics[:first_mentioned_in_commit_at],
TableReferences.merge_requests[:created_at]) TableReferences.merge_requests[:created_at])
end end
def test def test
calculate_metric!(:test, calculate_metric(:test,
TableReferences.merge_request_metrics[:latest_build_started_at], TableReferences.merge_request_metrics[:latest_build_started_at],
TableReferences.merge_request_metrics[:latest_build_finished_at]) TableReferences.merge_request_metrics[:latest_build_finished_at])
end end
def review def review
calculate_metric!(:review, calculate_metric(:review,
TableReferences.merge_requests[:created_at], TableReferences.merge_requests[:created_at],
TableReferences.merge_request_metrics[:merged_at]) TableReferences.merge_request_metrics[:merged_at])
end end
def staging def staging
calculate_metric!(:staging, calculate_metric(:staging,
TableReferences.merge_request_metrics[:merged_at], TableReferences.merge_request_metrics[:merged_at],
TableReferences.merge_request_metrics[:first_deployed_to_production_at]) TableReferences.merge_request_metrics[:first_deployed_to_production_at])
end end
def production def production
calculate_metric!(:production, calculate_metric(:production,
TableReferences.issues[:created_at], TableReferences.issues[:created_at],
TableReferences.merge_request_metrics[:first_deployed_to_production_at]) TableReferences.merge_request_metrics[:first_deployed_to_production_at])
end end
private private
def calculate_metric!(name, start_time_attrs, end_time_attrs) def calculate_metric(name, start_time_attrs, end_time_attrs)
cte_table = Arel::Table.new("cte_table_for_#{name}") cte_table = Arel::Table.new("cte_table_for_#{name}")
# Add a `SELECT` for (end_time - start-time), and add an alias for it. # Build a `SELECT` query. We find the first of the `end_time_attrs` that isn't `NULL` (call this end_time).
query = Arel::Nodes::As.new(cte_table, subtract_datetimes(base_query, end_time_attrs, start_time_attrs, name.to_s)) # Next, we find the first of the start_time_attrs that isn't `NULL` (call this start_time).
queries = Array.wrap(median_datetime(cte_table, query, name)) # We compute the (end_time - start_time) interval, and give it an alias based on the current
results = queries.map { |query| run_query(query) } # cycle analytics stage.
interval_query = Arel::Nodes::As.new(
cte_table,
subtract_datetimes(base_query, end_time_attrs, start_time_attrs, name.to_s))
median_queries = Array.wrap(median_datetime(cte_table, interval_query, name))
results = median_queries.map { |query| run_query(query) }
extract_median(results).presence extract_median(results).presence
end end
...@@ -82,51 +89,17 @@ class CycleAnalytics ...@@ -82,51 +89,17 @@ class CycleAnalytics
where(TableReferences.issues[:created_at].gteq(@from)) where(TableReferences.issues[:created_at].gteq(@from))
# Load merge_requests # Load merge_requests
query = query.join(TableReferences.merge_requests, Arel::Nodes::OuterJoin).on(TableReferences.merge_requests[:id].eq(arel_table[:merge_request_id])). query = query.join(TableReferences.merge_requests, Arel::Nodes::OuterJoin).
join(TableReferences.merge_request_metrics).on(TableReferences.merge_requests[:id].eq(TableReferences.merge_request_metrics[:merge_request_id])) on(TableReferences.merge_requests[:id].eq(arel_table[:merge_request_id])).
join(TableReferences.merge_request_metrics).
on(TableReferences.merge_requests[:id].eq(TableReferences.merge_request_metrics[:merge_request_id]))
# Limit to merge requests that have been deployed to production after `@from` # Limit to merge requests that have been deployed to production after `@from`
query.where(TableReferences.merge_request_metrics[:first_deployed_to_production_at].gteq(@from)) query.where(TableReferences.merge_request_metrics[:first_deployed_to_production_at].gteq(@from))
end end
# Note: We use COALESCE to pick up the first non-null column for end_time / start_time.
def subtract_datetimes(query_so_far, end_time_attrs, start_time_attrs, as)
diff_fn = case ActiveRecord::Base.connection.adapter_name
when 'PostgreSQL'
Arel::Nodes::Subtraction.new(
Arel::Nodes::NamedFunction.new("COALESCE", Array.wrap(end_time_attrs)),
Arel::Nodes::NamedFunction.new("COALESCE", Array.wrap(start_time_attrs)))
when 'Mysql2'
Arel::Nodes::NamedFunction.new(
"TIMESTAMPDIFF",
[Arel.sql('second'),
Arel::Nodes::NamedFunction.new("COALESCE", Array.wrap(start_time_attrs)),
Arel::Nodes::NamedFunction.new("COALESCE", Array.wrap(end_time_attrs))])
else
raise NotImplementedError, "Cycle analytics doesn't support your database type."
end
query_so_far.project(diff_fn.as(as))
end
def run_query(query) def run_query(query)
if query.is_a? String query = query.to_sql unless query.is_a?(String)
ActiveRecord::Base.connection.execute query ActiveRecord::Base.connection.execute(query)
else
ActiveRecord::Base.connection.execute query.to_sql
end
end
def extract_median(results)
result = results.compact.first
case ActiveRecord::Base.connection.adapter_name
when 'PostgreSQL'
result = result.first.presence
median = result['median'] if result
median.to_f if median
when 'Mysql2'
result.to_a.flatten.first
end
end end
end end
...@@ -46,22 +46,19 @@ class Deployment < ActiveRecord::Base ...@@ -46,22 +46,19 @@ class Deployment < ActiveRecord::Base
def update_merge_request_metrics def update_merge_request_metrics
if environment.update_merge_request_metrics? if environment.update_merge_request_metrics?
query = project.merge_requests. merge_requests = project.merge_requests.
joins(:metrics). joins(:metrics).
where(target_branch: self.ref, merge_request_metrics: { first_deployed_to_production_at: nil }) where(target_branch: self.ref, merge_request_metrics: { first_deployed_to_production_at: nil }).
where("merge_request_metrics.merged_at <= ?", self.created_at)
merge_requests =
if previous_deployment if previous_deployment
query.where("merge_request_metrics.merged_at <= ? AND merge_request_metrics.merged_at >= ?", merge_requests = merge_requests.where("merge_request_metrics.merged_at >= ?", previous_deployment.created_at)
self.created_at,
previous_deployment.created_at)
else
query.where("merge_request_metrics.merged_at <= ?", self.created_at)
end end
# Need to use `map` instead of `select` because MySQL doesn't allow `SELECT`ing from the same table # Need to use `map` instead of `select` because MySQL doesn't allow `SELECT`ing from the same table
# that we're updating. # that we're updating.
MergeRequest::Metrics.where(merge_request_id: merge_requests.map(&:id), first_deployed_to_production_at: nil). MergeRequest::Metrics.
where(merge_request_id: merge_requests.map(&:id), first_deployed_to_production_at: nil).
update_all(first_deployed_to_production_at: self.created_at) update_all(first_deployed_to_production_at: self.created_at)
end end
end end
......
...@@ -23,8 +23,6 @@ class Issue < ActiveRecord::Base ...@@ -23,8 +23,6 @@ class Issue < ActiveRecord::Base
has_many :events, as: :target, dependent: :destroy has_many :events, as: :target, dependent: :destroy
has_one :metrics
has_many :merge_requests_closing_issues, class_name: 'MergeRequestsClosingIssues' has_many :merge_requests_closing_issues, class_name: 'MergeRequestsClosingIssues'
has_many :closed_by_merge_requests, through: :merge_requests_closing_issues, source: :merge_request has_many :closed_by_merge_requests, through: :merge_requests_closing_issues, source: :merge_request
...@@ -41,8 +39,6 @@ class Issue < ActiveRecord::Base ...@@ -41,8 +39,6 @@ class Issue < ActiveRecord::Base
scope :order_due_date_asc, -> { reorder('issues.due_date IS NULL, issues.due_date ASC') } scope :order_due_date_asc, -> { reorder('issues.due_date IS NULL, issues.due_date ASC') }
scope :order_due_date_desc, -> { reorder('issues.due_date IS NULL, issues.due_date DESC') } scope :order_due_date_desc, -> { reorder('issues.due_date IS NULL, issues.due_date DESC') }
after_save :record_metrics
attr_spammable :title, spam_title: true attr_spammable :title, spam_title: true
attr_spammable :description, spam_description: true attr_spammable :description, spam_description: true
...@@ -277,9 +273,4 @@ class Issue < ActiveRecord::Base ...@@ -277,9 +273,4 @@ class Issue < ActiveRecord::Base
def check_for_spam? def check_for_spam?
project.public? project.public?
end end
def record_metrics
metrics = Metrics.find_or_create_by(issue_id: self.id)
metrics.record!
end
end end
...@@ -10,7 +10,7 @@ class Issue::Metrics < ActiveRecord::Base ...@@ -10,7 +10,7 @@ class Issue::Metrics < ActiveRecord::Base
self.first_added_to_board_at = Time.now self.first_added_to_board_at = Time.now
end end
self.save if self.changed? self.save
end end
private private
......
...@@ -13,7 +13,6 @@ class MergeRequest < ActiveRecord::Base ...@@ -13,7 +13,6 @@ class MergeRequest < ActiveRecord::Base
has_many :merge_request_diffs, dependent: :destroy has_many :merge_request_diffs, dependent: :destroy
has_one :merge_request_diff, has_one :merge_request_diff,
-> { order('merge_request_diffs.id DESC') } -> { order('merge_request_diffs.id DESC') }
has_one :metrics
has_many :events, as: :target, dependent: :destroy has_many :events, as: :target, dependent: :destroy
...@@ -35,8 +34,6 @@ class MergeRequest < ActiveRecord::Base ...@@ -35,8 +34,6 @@ class MergeRequest < ActiveRecord::Base
# when creating new merge request # when creating new merge request
attr_accessor :can_be_created, :compare_commits, :compare attr_accessor :can_be_created, :compare_commits, :compare
after_save :record_metrics
state_machine :state, initial: :opened do state_machine :state, initial: :opened do
event :close do event :close do
transition [:reopened, :opened] => :closed transition [:reopened, :opened] => :closed
...@@ -515,7 +512,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -515,7 +512,7 @@ class MergeRequest < ActiveRecord::Base
transaction do transaction do
self.merge_requests_closing_issues.delete_all self.merge_requests_closing_issues.delete_all
closes_issues(current_user).each do |issue| closes_issues(current_user).each do |issue|
MergeRequestsClosingIssues.create!(merge_request: self, issue: issue) self.merge_requests_closing_issues.create!(issue: issue)
end end
end end
end end
...@@ -845,9 +842,4 @@ class MergeRequest < ActiveRecord::Base ...@@ -845,9 +842,4 @@ class MergeRequest < ActiveRecord::Base
@conflicts_can_be_resolved_in_ui = false @conflicts_can_be_resolved_in_ui = false
end end
end end
def record_metrics
metrics = Metrics.find_or_create_by(merge_request_id: self.id)
metrics.record!
end
end end
...@@ -6,6 +6,6 @@ class MergeRequest::Metrics < ActiveRecord::Base ...@@ -6,6 +6,6 @@ class MergeRequest::Metrics < ActiveRecord::Base
self.merged_at = Time.now self.merged_at = Time.now
end end
self.save if self.changed? self.save
end end
end end
...@@ -780,7 +780,7 @@ Rails.application.routes.draw do ...@@ -780,7 +780,7 @@ Rails.application.routes.draw do
resources :environments resources :environments
resource :cycle_analytics resource :cycle_analytics, only: [:show]
resources :builds, only: [:index, :show], constraints: { id: /\d+/ } do resources :builds, only: [:index, :show], constraints: { id: /\d+/ } do
collection do collection do
......
module Gitlab
module Database
module DateTime
# Find the first of the `end_time_attrs` that isn't `NULL`. Subtract from it
# the first of the `start_time_attrs` that isn't NULL. `SELECT` the resulting interval
# along with an alias specified by the `as` parameter.
#
# Note: For MySQL, the interval is returned in seconds.
# For PostgreSQL, the interval is returned as an INTERVAL type.
def subtract_datetimes(query_so_far, end_time_attrs, start_time_attrs, as)
diff_fn = if Gitlab::Database.postgresql?
Arel::Nodes::Subtraction.new(
Arel::Nodes::NamedFunction.new("COALESCE", Array.wrap(end_time_attrs)),
Arel::Nodes::NamedFunction.new("COALESCE", Array.wrap(start_time_attrs)))
elsif Gitlab::Database.mysql?
Arel::Nodes::NamedFunction.new(
"TIMESTAMPDIFF",
[Arel.sql('second'),
Arel::Nodes::NamedFunction.new("COALESCE", Array.wrap(start_time_attrs)),
Arel::Nodes::NamedFunction.new("COALESCE", Array.wrap(end_time_attrs))])
end
query_so_far.project(diff_fn.as(as))
end
end
end
end
...@@ -3,13 +3,22 @@ module Gitlab ...@@ -3,13 +3,22 @@ module Gitlab
module Database module Database
module Median module Median
def median_datetime(arel_table, query_so_far, column_sym) def median_datetime(arel_table, query_so_far, column_sym)
case ActiveRecord::Base.connection.adapter_name if Gitlab::Database.postgresql?
when 'PostgreSQL'
pg_median_datetime(arel_table, query_so_far, column_sym) pg_median_datetime(arel_table, query_so_far, column_sym)
when 'Mysql2' elsif Gitlab::Database.mysql?
mysql_median_datetime(arel_table, query_so_far, column_sym) mysql_median_datetime(arel_table, query_so_far, column_sym)
else end
raise NotImplementedError, "We haven't implemented a database median strategy for your database type." end
def extract_median(results)
result = results.compact.first
if Gitlab::Database.postgresql?
result = result.first.presence
median = result['median'] if result
median.to_f if median
elsif Gitlab::Database.mysql?
result.to_a.flatten.first
end end
end end
...@@ -17,17 +26,23 @@ module Gitlab ...@@ -17,17 +26,23 @@ module Gitlab
query = arel_table. query = arel_table.
from(arel_table.project(Arel.sql('*')).order(arel_table[column_sym]).as(arel_table.table_name)). from(arel_table.project(Arel.sql('*')).order(arel_table[column_sym]).as(arel_table.table_name)).
project(average([arel_table[column_sym]], 'median')). project(average([arel_table[column_sym]], 'median')).
where(Arel::Nodes::Between.new(Arel.sql("(select @row_id := @row_id + 1)"), where(Arel::Nodes::Between.new(
Arel::Nodes::And.new([Arel.sql('@ct/2.0'), Arel.sql("(select @row_id := @row_id + 1)"),
Arel.sql('@ct/2.0 + 1')]))). Arel::Nodes::And.new(
[Arel.sql('@ct/2.0'),
Arel.sql('@ct/2.0 + 1')]
)
)).
# Disallow negative values # Disallow negative values
where(arel_table[column_sym].gteq(0)) where(arel_table[column_sym].gteq(0))
[Arel.sql("CREATE TEMPORARY TABLE IF NOT EXISTS #{query_so_far.to_sql}"), [
Arel.sql("CREATE TEMPORARY TABLE IF NOT EXISTS #{query_so_far.to_sql}"),
Arel.sql("set @ct := (select count(1) from #{arel_table.table_name});"), Arel.sql("set @ct := (select count(1) from #{arel_table.table_name});"),
Arel.sql("set @row_id := 0;"), Arel.sql("set @row_id := 0;"),
query, query,
Arel.sql("DROP TEMPORARY TABLE IF EXISTS #{arel_table.table_name};")] Arel.sql("DROP TEMPORARY TABLE IF EXISTS #{arel_table.table_name};")
]
end end
def pg_median_datetime(arel_table, query_so_far, column_sym) def pg_median_datetime(arel_table, query_so_far, column_sym)
...@@ -42,7 +57,8 @@ module Gitlab ...@@ -42,7 +57,8 @@ module Gitlab
# 9 | 2 | 3 # 9 | 2 | 3
# 15 | 3 | 3 # 15 | 3 | 3
cte_table = Arel::Table.new("ordered_records") cte_table = Arel::Table.new("ordered_records")
cte = Arel::Nodes::As.new(cte_table, cte = Arel::Nodes::As.new(
cte_table,
arel_table. arel_table.
project(arel_table[column_sym].as(column_sym.to_s), project(arel_table[column_sym].as(column_sym.to_s),
Arel::Nodes::Over.new(Arel::Nodes::NamedFunction.new("row_number", []), Arel::Nodes::Over.new(Arel::Nodes::NamedFunction.new("row_number", []),
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment