From ab1e1b55a84ffc6b09233a6831be9bdc77c05115 Mon Sep 17 00:00:00 2001 From: syasonik Date: Fri, 15 Mar 2019 19:20:59 +0800 Subject: [PATCH] Specify time window for additional metrics 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. --- .../projects/environments_controller.rb | 12 +++++- app/models/concerns/prometheus_adapter.rb | 2 +- app/models/environment.rb | 6 ++- .../additional_metrics_environment_query.rb | 8 +++- .../projects/environments_controller_spec.rb | 34 ++++++++++++++- ...ditional_metrics_environment_query_spec.rb | 30 ++++++++++++- .../concerns/prometheus_adapter_spec.rb | 42 +++++++++++++++++++ spec/models/environment_spec.rb | 23 ++++++++-- 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 -- GitLab