From 7f102819a56b55607e657447b51d2eeb45b2fe94 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Mon, 26 Aug 2019 14:57:59 +0100 Subject: Fix Peek on Puma Peek's `Peek.request_id` method doesn't work well with a multi-threaded server and concurrent requests, because requests can 'steal' another request's ID, or unset it before it was due. The upstream change resolves this; the commit here is just to ensure that GitLab works with that upstream change, mostly by not using `Peek.request_id` any more (as the method doesn't exist). --- app/controllers/concerns/with_performance_bar.rb | 6 ++++++ app/views/peek/_bar.html.haml | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) (limited to 'app') diff --git a/app/controllers/concerns/with_performance_bar.rb b/app/controllers/concerns/with_performance_bar.rb index 4e0ae3c59eb..b19d6eb9439 100644 --- a/app/controllers/concerns/with_performance_bar.rb +++ b/app/controllers/concerns/with_performance_bar.rb @@ -3,6 +3,12 @@ module WithPerformanceBar extend ActiveSupport::Concern + included do + before_action :peek_enabled? # Warm cache + end + + protected + def peek_enabled? return false unless Gitlab::PerformanceBar.enabled?(current_user) diff --git a/app/views/peek/_bar.html.haml b/app/views/peek/_bar.html.haml index 5228930293c..3c2c4d30535 100644 --- a/app/views/peek/_bar.html.haml +++ b/app/views/peek/_bar.html.haml @@ -1,6 +1,6 @@ - return unless peek_enabled? #js-peek{ data: { env: Peek.env, - request_id: Peek.request_id, + request_id: peek_request_id, peek_url: "#{peek_routes_path}/results" }, class: Peek.env } -- cgit v1.2.3 From f9c456bd0c34002952fa4ac812068b38258b108d Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 27 Aug 2019 12:40:44 +0100 Subject: Make performance bar enabled checks consistent Previously, we called the `peek_enabled?` method like so: prepend_before_action :set_peek_request_id, if: :peek_enabled? Now we don't have a `set_peek_request_id` method, so we don't need that line. However, the `peek_enabled?` part had a side-effect: it would also populate the request store cache for whether the performance bar was enabled for the current request or not. This commit makes that side-effect explicit, and replaces all uses of `peek_enabled?` with the more explicit `Gitlab::PerformanceBar.enabled_for_request?`. There is one spec that still sets `SafeRequestStore[:peek_enabled]` directly, because it is contrasting behaviour with and without a request store enabled. The upshot is: 1. We still set the value in one place. We make it more explicit that that's what we're doing. 2. Reading that value uses a consistent method so it's easier to find in future. --- app/controllers/concerns/with_performance_bar.rb | 16 ++++++++++------ app/helpers/performance_bar_helper.rb | 4 +--- app/views/peek/_bar.html.haml | 2 +- 3 files changed, 12 insertions(+), 10 deletions(-) (limited to 'app') diff --git a/app/controllers/concerns/with_performance_bar.rb b/app/controllers/concerns/with_performance_bar.rb index b19d6eb9439..93ded59900d 100644 --- a/app/controllers/concerns/with_performance_bar.rb +++ b/app/controllers/concerns/with_performance_bar.rb @@ -4,20 +4,24 @@ module WithPerformanceBar extend ActiveSupport::Concern included do - before_action :peek_enabled? # Warm cache + before_action :set_peek_enabled_for_current_request end - protected - - def peek_enabled? - return false unless Gitlab::PerformanceBar.enabled?(current_user) + private + def set_peek_enabled_for_current_request Gitlab::SafeRequestStore.fetch(:peek_enabled) { cookie_or_default_value } end - private + # Needed for Peek's routing to work; + # Peek::ResultsController#restrict_non_access calls this method. + def peek_enabled? + Gitlab::PerformanceBar.enabled_for_request? + end def cookie_or_default_value + return false unless Gitlab::PerformanceBar.enabled_for_user?(current_user) + if cookies[:perf_bar_enabled].present? cookies[:perf_bar_enabled] == 'true' else diff --git a/app/helpers/performance_bar_helper.rb b/app/helpers/performance_bar_helper.rb index 7518cec160c..b225e4206a9 100644 --- a/app/helpers/performance_bar_helper.rb +++ b/app/helpers/performance_bar_helper.rb @@ -1,9 +1,7 @@ # frozen_string_literal: true module PerformanceBarHelper - # This is a hack since using `alias_method :performance_bar_enabled?, :peek_enabled?` - # in WithPerformanceBar breaks tests (but works in the browser). def performance_bar_enabled? - peek_enabled? + Gitlab::PerformanceBar.enabled_for_request? end end diff --git a/app/views/peek/_bar.html.haml b/app/views/peek/_bar.html.haml index 3c2c4d30535..9725f640be9 100644 --- a/app/views/peek/_bar.html.haml +++ b/app/views/peek/_bar.html.haml @@ -1,4 +1,4 @@ -- return unless peek_enabled? +- return unless performance_bar_enabled? #js-peek{ data: { env: Peek.env, request_id: peek_request_id, -- cgit v1.2.3