Specify time window for additional metrics apisupport-time-windows-api

Adds support for start and end parameters in the #additional_metrics
endpoint of the EnvironmentsController. start and end are meant to be
unix timestamps, per the Prometheus API (as the consumer of this
endpoint will eventually be transitioned to a prometheus endpoint).
This functionality is behind the :metrics_time_window feature flag
for development.

8 files changed, 143 insertions, 14 deletions

diff --git a/app/controllers/projects/environments_controller.rb b/app/controllers/projects/environments_controller.rb
index e9cd475a199..4fa6cd94ae5 100644
--- a/app/controllers/projects/environments_controller.rb
+++ b/app/controllers/projects/environments_controller.rb
@@ -10,6 +10,9 @@ class Projects::EnvironmentsController < Projects::ApplicationController
   before_action :environment, only: [:show, :edit, :update, :stop, :terminal, :terminal_websocket_authorize, :metrics]
   before_action :verify_api_request!, only: :terminal_websocket_authorize
   before_action :expire_etag_cache, only: [:index]
+  before_action only: [:metrics, :additional_metrics] do
+    push_frontend_feature_flag(:metrics_time_window)
+  end
 
   def index
     @environments = project.environments
@@ -146,7 +149,7 @@ class Projects::EnvironmentsController < Projects::ApplicationController
   def additional_metrics
     respond_to do |format|
       format.json do
-        additional_metrics = environment.additional_metrics || {}
+        additional_metrics = environment.additional_metrics(*metrics_params) || {}
 
         render json: additional_metrics, status: additional_metrics.any? ? :ok : :no_content
       end
@@ -186,6 +189,13 @@ class Projects::EnvironmentsController < Projects::ApplicationController
     @environment ||= project.environments.find(params[:id])
   end
 
+  def metrics_params
+    return unless Feature.enabled?(:metrics_time_window, project)
+    return unless params[:start].present? || params[:end].present?
+
+    params.require([:start, :end]).values_at(:start, :end)
+  end
+
   def search_environment_names
     return [] unless params[:query]
 
diff --git a/app/models/concerns/prometheus_adapter.rb b/app/models/concerns/prometheus_adapter.rb
index a29e80fe0c1..decbbbd87f2 100644
--- a/app/models/concerns/prometheus_adapter.rb
+++ b/app/models/concerns/prometheus_adapter.rb
@@ -51,7 +51,7 @@ module PrometheusAdapter
     end
 
     def build_query_args(*args)
-      args.map(&:id)
+      args.map { |arg| arg.respond_to?(:id) ? arg.id : arg }
     end
   end
 end
diff --git a/app/models/environment.rb b/app/models/environment.rb
index 25373c7a1f7..fa29a83e517 100644
--- a/app/models/environment.rb
+++ b/app/models/environment.rb
@@ -170,8 +170,10 @@ class Environment < ApplicationRecord
     prometheus_adapter.query(:environment, self) if has_metrics?
   end
 
-  def additional_metrics
-    prometheus_adapter.query(:additional_metrics_environment, self) if has_metrics?
+  def additional_metrics(*args)
+    return unless has_metrics?
+
+    prometheus_adapter.query(:additional_metrics_environment, self, *args.map(&:to_f))
   end
 
   # rubocop: disable CodeReuse/ServiceClass
diff --git a/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb b/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb
index 34b705138ba..c49877ddf9d 100644
--- a/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb
+++ b/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb
@@ -7,12 +7,16 @@ module Gitlab
         include QueryAdditionalMetrics
 
         # rubocop: disable CodeReuse/ActiveRecord
-        def query(environment_id)
+        def query(environment_id, timeframe_start = 8.hours.ago, timeframe_end = Time.now)
           ::Environment.find_by(id: environment_id).try do |environment|
             query_metrics(
               environment.project,
               environment,
-              common_query_context(environment, timeframe_start: 8.hours.ago.to_f, timeframe_end: Time.now.to_f)
+              common_query_context(
+                environment,
+                timeframe_start: timeframe_start.to_f,
+                timeframe_end: timeframe_end.to_f
+              )
             )
           end
         end
diff --git a/spec/controllers/projects/environments_controller_spec.rb b/spec/controllers/projects/environments_controller_spec.rb
index 36ce1119100..2dca2c3976f 100644
--- a/spec/controllers/projects/environments_controller_spec.rb
+++ b/spec/controllers/projects/environments_controller_spec.rb
@@ -392,7 +392,7 @@ describe Projects::EnvironmentsController do
 
       context 'when requesting metrics as JSON' do
         it 'returns a metrics JSON document' do
-          get :additional_metrics, params: environment_params(format: :json)
+          additional_metrics
 
           expect(response).to have_gitlab_http_status(204)
           expect(json_response).to eq({})
@@ -412,7 +412,7 @@ describe Projects::EnvironmentsController do
       end
 
       it 'returns a metrics JSON document' do
-        get :additional_metrics, params: environment_params(format: :json)
+        additional_metrics
 
         expect(response).to be_ok
         expect(json_response['success']).to be(true)
@@ -420,6 +420,32 @@ describe Projects::EnvironmentsController do
         expect(json_response['last_update']).to eq(42)
       end
     end
+
+    context 'when only one time param is provided' do
+      context 'when :metrics_time_window feature flag is disabled' do
+        before do
+          stub_feature_flags(metrics_time_window: false)
+          expect(environment).to receive(:additional_metrics).with(no_args).and_return(nil)
+        end
+
+        it 'returns a time-window agnostic response' do
+          additional_metrics(start: '1552647300.651094')
+
+          expect(response).to have_gitlab_http_status(204)
+          expect(json_response).to eq({})
+        end
+      end
+
+      it 'raises an error when start is missing' do
+        expect { additional_metrics(start: '1552647300.651094') }
+          .to raise_error(ActionController::ParameterMissing)
+      end
+
+      it 'raises an error when end is missing' do
+        expect { additional_metrics(start: '1552647300.651094') }
+          .to raise_error(ActionController::ParameterMissing)
+      end
+    end
   end
 
   describe 'GET #search' do
@@ -500,4 +526,8 @@ describe Projects::EnvironmentsController do
                        project_id: project,
                        id: environment.id)
   end
+
+  def additional_metrics(opts = {})
+    get :additional_metrics, params: environment_params(format: :json, **opts)
+  end
 end
diff --git a/spec/lib/gitlab/prometheus/queries/additional_metrics_environment_query_spec.rb b/spec/lib/gitlab/prometheus/queries/additional_metrics_environment_query_spec.rb
index 5a88b23aa82..a6589f0c0a3 100644
--- a/spec/lib/gitlab/prometheus/queries/additional_metrics_environment_query_spec.rb
+++ b/spec/lib/gitlab/prometheus/queries/additional_metrics_environment_query_spec.rb
@@ -9,9 +9,35 @@ describe Gitlab::Prometheus::Queries::AdditionalMetricsEnvironmentQuery do
     let(:query_params) { [environment.id] }
 
     it 'queries using specific time' do
-      expect(client).to receive(:query_range).with(anything, start: 8.hours.ago.to_f, stop: Time.now.to_f)
-
+      expect(client).to receive(:query_range)
+        .with(anything, start: 8.hours.ago.to_f, stop: Time.now.to_f)
       expect(query_result).not_to be_nil
     end
+
+    context 'when start and end time parameters are provided' do
+      let(:query_params) { [environment.id, start_time, end_time] }
+
+      context 'as unix timestamps' do
+        let(:start_time) { 4.hours.ago.to_f }
+        let(:end_time) { 2.hours.ago.to_f }
+
+        it 'queries using the provided times' do
+          expect(client).to receive(:query_range)
+            .with(anything, start: start_time, stop: end_time)
+          expect(query_result).not_to be_nil
+        end
+      end
+
+      context 'as Date/Time objects' do
+        let(:start_time) { 4.hours.ago }
+        let(:end_time) { 2.hours.ago }
+
+        it 'queries using the provided times converted to unix' do
+          expect(client).to receive(:query_range)
+            .with(anything, start: start_time.to_f, stop: end_time.to_f)
+          expect(query_result).not_to be_nil
+        end
+      end
+    end
   end
 end
diff --git a/spec/models/concerns/prometheus_adapter_spec.rb b/spec/models/concerns/prometheus_adapter_spec.rb
index db20a8b4701..25a2d290f76 100644
--- a/spec/models/concerns/prometheus_adapter_spec.rb
+++ b/spec/models/concerns/prometheus_adapter_spec.rb
@@ -77,6 +77,28 @@ describe PrometheusAdapter, :use_clean_rails_memory_store_caching do
         end
       end
     end
+
+    describe 'additional_metrics' do
+      let(:additional_metrics_environment_query) { Gitlab::Prometheus::Queries::AdditionalMetricsEnvironmentQuery }
+      let(:environment) { build_stubbed(:environment, slug: 'env-slug') }
+      let(:time_window) { [1552642245.067, 1552642095.831] }
+
+      around do |example|
+        Timecop.freeze { example.run }
+      end
+
+      context 'with valid data' do
+        subject { service.query(:additional_metrics_environment, environment, *time_window) }
+
+        before do
+          stub_reactive_cache(service, prometheus_data, additional_metrics_environment_query, environment.id, *time_window)
+        end
+
+        it 'returns reactive data' do
+          expect(subject).to eq(prometheus_data)
+        end
+      end
+    end
   end
 
   describe '#calculate_reactive_cache' do
@@ -121,4 +143,24 @@ describe PrometheusAdapter, :use_clean_rails_memory_store_caching do
       end
     end
   end
+
+  describe '#build_query_args' do
+    subject { service.build_query_args(*args) }
+
+    context 'when active record models are included' do
+      let(:args) { [double(:environment, id: 12)] }
+
+      it 'serializes by id' do
+        is_expected.to eq [12]
+      end
+    end
+
+    context 'when args are safe for serialization' do
+      let(:args) { ['stringy arg', 5, 6.0, :symbolic_arg] }
+
+      it 'does nothing' do
+        is_expected.to eq args
+      end
+    end
+  end
 end
diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb
index ca5eed60b56..cfe7c7ef0b0 100644
--- a/spec/models/environment_spec.rb
+++ b/spec/models/environment_spec.rb
@@ -687,7 +687,8 @@ describe Environment do
 
   describe '#additional_metrics' do
     let(:project) { create(:prometheus_project) }
-    subject { environment.additional_metrics }
+    let(:metric_params) { [] }
+    subject { environment.additional_metrics(*metric_params) }
 
     context 'when the environment has additional metrics' do
       before do
@@ -695,12 +696,26 @@ describe Environment do
       end
 
       it 'returns the additional metrics from the deployment service' do
-        expect(environment.prometheus_adapter).to receive(:query)
-                                                .with(:additional_metrics_environment, environment)
-                                                .and_return(:fake_metrics)
+        expect(environment.prometheus_adapter)
+          .to receive(:query)
+          .with(:additional_metrics_environment, environment)
+          .and_return(:fake_metrics)
 
         is_expected.to eq(:fake_metrics)
       end
+
+      context 'when time window arguments are provided' do
+        let(:metric_params) { [1552642245.067, Time.now] }
+
+        it 'queries with the expected parameters' do
+          expect(environment.prometheus_adapter)
+            .to receive(:query)
+            .with(:additional_metrics_environment, environment, *metric_params.map(&:to_f))
+            .and_return(:fake_metrics)
+
+          is_expected.to eq(:fake_metrics)
+        end
+      end
     end
 
     context 'when the environment does not have metrics' do