From ce83e5635d1903cfadf4e2d9a7088b505f6c28ff Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Fri, 28 Jul 2017 23:02:21 +0200 Subject: [PATCH 1/7] add kube_namespace and standardize common variables for additional metrics queries --- .../queries/additional_metrics_deployment_query.rb | 12 ++++++------ .../queries/additional_metrics_environment_query.rb | 12 ++++++------ .../prometheus/queries/query_additional_metrics.rb | 12 ++++++++++++ 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb b/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb index 67c69d9ccf3..51d934b9ae2 100644 --- a/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb +++ b/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb @@ -6,12 +6,12 @@ module Gitlab def query(deployment_id) Deployment.find_by(id: deployment_id).try do |deployment| - query_context = { - environment_slug: deployment.environment.slug, - environment_filter: %{container_name!="POD",environment="#{deployment.environment.slug}"}, - timeframe_start: (deployment.created_at - 30.minutes).to_f, - timeframe_end: (deployment.created_at + 30.minutes).to_f - } + query_context = common_query_context(deployment.environment).merge( + { + timeframe_start: (deployment.created_at - 30.minutes).to_f, + timeframe_end: (deployment.created_at + 30.minutes).to_f + } + ) query_metrics(query_context) end diff --git a/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb b/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb index b5a679ddd79..9f798f5b892 100644 --- a/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb +++ b/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb @@ -6,12 +6,12 @@ module Gitlab def query(environment_id) Environment.find_by(id: environment_id).try do |environment| - query_context = { - environment_slug: environment.slug, - environment_filter: %{container_name!="POD",environment="#{environment.slug}"}, - timeframe_start: 8.hours.ago.to_f, - timeframe_end: Time.now.to_f - } + query_context = common_query_context(environment).merge( + { + timeframe_start: 8.hours.ago.to_f, + timeframe_end: Time.now.to_f + } + ) query_metrics(query_context) end diff --git a/lib/gitlab/prometheus/queries/query_additional_metrics.rb b/lib/gitlab/prometheus/queries/query_additional_metrics.rb index e44be770544..003e6aa6c87 100644 --- a/lib/gitlab/prometheus/queries/query_additional_metrics.rb +++ b/lib/gitlab/prometheus/queries/query_additional_metrics.rb @@ -67,6 +67,18 @@ module Gitlab result.select { |group| group.metrics.any? } end + + def common_query_context(environment) + variables = { + ci_environment_slug: environment.slug, + kube_namespace: environment.project.kubernetes_service.actual_namespace, + }.flat_map { |k, v| [[k, v], [k.upcase, v]] }.to_h + + macros = { + environment_filter: %{container_name!="POD",environment="#{environment.slug}"} + } + variables.merge(macros) + end end end end From 6df5bd8b848322aa3e2ce5fd8cad0e2a20b06000 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Mon, 31 Jul 2017 13:29:00 +0200 Subject: [PATCH 2/7] Context handling and tests cleanup + simple test kube_namespace context test --- .../project_services/prometheus_service.rb | 5 ++++ .../queries/query_additional_metrics.rb | 20 ++++++------- ...dditional_metrics_deployment_query_spec.rb | 11 ++----- ...ditional_metrics_environment_query_spec.rb | 10 ++----- .../additional_metrics_shared_examples.rb | 29 +++++++++++++++++++ 5 files changed, 49 insertions(+), 26 deletions(-) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 217f753f05f..73cf4e97b56 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -75,6 +75,11 @@ class PrometheusService < MonitoringService with_reactive_cache(Gitlab::Prometheus::Queries::MatchedMetricsQuery.name, &:itself) end + def with_reactive_cache(name, *args, &block) + vals = args.map(&:to_s) + yield calculate_reactive_cache(name, *vals) + end + # Cache metrics for specific environment def calculate_reactive_cache(query_class_name, *args) return unless active? && project && !project.pending_delete? diff --git a/lib/gitlab/prometheus/queries/query_additional_metrics.rb b/lib/gitlab/prometheus/queries/query_additional_metrics.rb index 003e6aa6c87..5827c117556 100644 --- a/lib/gitlab/prometheus/queries/query_additional_metrics.rb +++ b/lib/gitlab/prometheus/queries/query_additional_metrics.rb @@ -42,15 +42,17 @@ module Gitlab end def process_query(context, query) - query_with_result = query.dup + query = query.dup result = if query.key?(:query_range) - client_query_range(query[:query_range] % context, start: context[:timeframe_start], stop: context[:timeframe_end]) + query[:query_range] %= context + client_query_range(query[:query_range], start: context[:timeframe_start], stop: context[:timeframe_end]) else - client_query(query[:query] % context, time: context[:timeframe_end]) + query[:query] %= context + client_query(query[:query], time: context[:timeframe_end]) end - query_with_result[:result] = result&.map(&:deep_symbolize_keys) - query_with_result + query[:result] = result&.map(&:deep_symbolize_keys) + query end def available_metrics @@ -69,15 +71,11 @@ module Gitlab end def common_query_context(environment) - variables = { + { ci_environment_slug: environment.slug, - kube_namespace: environment.project.kubernetes_service.actual_namespace, - }.flat_map { |k, v| [[k, v], [k.upcase, v]] }.to_h - - macros = { + kube_namespace: environment.project.kubernetes_service&.actual_namespace || '', environment_filter: %{container_name!="POD",environment="#{environment.slug}"} } - variables.merge(macros) end end end diff --git a/spec/lib/gitlab/prometheus/queries/additional_metrics_deployment_query_spec.rb b/spec/lib/gitlab/prometheus/queries/additional_metrics_deployment_query_spec.rb index e42e034f4fb..c7169717fc1 100644 --- a/spec/lib/gitlab/prometheus/queries/additional_metrics_deployment_query_spec.rb +++ b/spec/lib/gitlab/prometheus/queries/additional_metrics_deployment_query_spec.rb @@ -1,19 +1,14 @@ require 'spec_helper' describe Gitlab::Prometheus::Queries::AdditionalMetricsDeploymentQuery do - include Prometheus::MetricBuilders - - let(:client) { double('prometheus_client') } - let(:environment) { create(:environment, slug: 'environment-slug') } - let(:deployment) { create(:deployment, environment: environment) } - - subject(:query_result) { described_class.new(client).query(deployment.id) } - around do |example| Timecop.freeze(Time.local(2008, 9, 1, 12, 0, 0)) { example.run } end include_examples 'additional metrics query' do + let(:deployment) { create(:deployment, environment: environment) } + let(:query_params) { [deployment.id] } + it 'queries using specific time' do expect(client).to receive(:query_range).with(anything, start: (deployment.created_at - 30.minutes).to_f, 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 e9fd66d45fe..a02d2a90b97 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 @@ -1,18 +1,14 @@ require 'spec_helper' describe Gitlab::Prometheus::Queries::AdditionalMetricsEnvironmentQuery do - include Prometheus::MetricBuilders - - let(:client) { double('prometheus_client') } - let(:environment) { create(:environment, slug: 'environment-slug') } - - subject(:query_result) { described_class.new(client).query(environment.id) } - around do |example| Timecop.freeze { example.run } end include_examples 'additional metrics query' do + let(:query_result) { described_class.new(client).query(environment.id) } + 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(query_result).not_to be_nil diff --git a/spec/support/prometheus/additional_metrics_shared_examples.rb b/spec/support/prometheus/additional_metrics_shared_examples.rb index 016e16fc8d4..fcb5c315b98 100644 --- a/spec/support/prometheus/additional_metrics_shared_examples.rb +++ b/spec/support/prometheus/additional_metrics_shared_examples.rb @@ -10,12 +10,39 @@ RSpec.shared_examples 'additional metrics query' do [{ 'metric': {}, 'values': [[1488758662.506, '0.00002996364761904785'], [1488758722.506, '0.00003090239047619091']] }] end + let(:client) { double('prometheus_client') } + let(:environment) { create(:environment, slug: 'environment-slug') } + before do allow(client).to receive(:label_values).and_return(metric_names) allow(metric_group_class).to receive(:all).and_return([simple_metric_group(metrics: [simple_metric])]) end + context 'metrics rendering' do + subject! { described_class.new(client) } + + before do + + end + + describe 'project has kubernetes service' do + let(:project) { create(:kubernetes_project) } + let(:environment) { create(:environment, slug: 'environment-slug', project: project) } + let(:kube_namespace) { project.kubernetes_service.actual_namespace } + + it 'query context contains kube namespace' do + expect(subject).to receive(:query_metrics).with( + hash_including( + kube_namespace: kube_namespace) + ) + subject.query(*query_params) + end + end + end + context 'with one group where two metrics is found' do + let(:query_result) { described_class.new(client).query(*query_params) } + before do allow(metric_group_class).to receive(:all).and_return([simple_metric_group]) end @@ -50,7 +77,9 @@ RSpec.shared_examples 'additional metrics query' do end context 'with two groups with one metric each' do + let(:query_result) { described_class.new(client).query(*query_params) } let(:metrics) { [simple_metric(queries: [simple_query])] } + before do allow(metric_group_class).to receive(:all).and_return( [ From 48778ac58973fa7cab09deaaf2e93806aa37113d Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Mon, 31 Jul 2017 15:26:38 +0200 Subject: [PATCH 3/7] Tests for query context variables --- .../project_services/prometheus_service.rb | 5 -- ...ditional_metrics_environment_query_spec.rb | 2 +- .../additional_metrics_shared_examples.rb | 47 ++++++++++++++----- 3 files changed, 35 insertions(+), 19 deletions(-) diff --git a/app/models/project_services/prometheus_service.rb b/app/models/project_services/prometheus_service.rb index 73cf4e97b56..217f753f05f 100644 --- a/app/models/project_services/prometheus_service.rb +++ b/app/models/project_services/prometheus_service.rb @@ -75,11 +75,6 @@ class PrometheusService < MonitoringService with_reactive_cache(Gitlab::Prometheus::Queries::MatchedMetricsQuery.name, &:itself) end - def with_reactive_cache(name, *args, &block) - vals = args.map(&:to_s) - yield calculate_reactive_cache(name, *vals) - end - # Cache metrics for specific environment def calculate_reactive_cache(query_class_name, *args) return unless active? && project && !project.pending_delete? 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 a02d2a90b97..5a88b23aa82 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 @@ -6,11 +6,11 @@ describe Gitlab::Prometheus::Queries::AdditionalMetricsEnvironmentQuery do end include_examples 'additional metrics query' do - let(:query_result) { described_class.new(client).query(environment.id) } 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(query_result).not_to be_nil end end diff --git a/spec/support/prometheus/additional_metrics_shared_examples.rb b/spec/support/prometheus/additional_metrics_shared_examples.rb index fcb5c315b98..174297af158 100644 --- a/spec/support/prometheus/additional_metrics_shared_examples.rb +++ b/spec/support/prometheus/additional_metrics_shared_examples.rb @@ -11,6 +11,7 @@ RSpec.shared_examples 'additional metrics query' do end let(:client) { double('prometheus_client') } + let(:query_result) { described_class.new(client).query(*query_params) } let(:environment) { create(:environment, slug: 'environment-slug') } before do @@ -18,31 +19,52 @@ RSpec.shared_examples 'additional metrics query' do allow(metric_group_class).to receive(:all).and_return([simple_metric_group(metrics: [simple_metric])]) end - context 'metrics rendering' do + context 'metrics query context' do subject! { described_class.new(client) } - before do + shared_examples 'query context containing environment slug and filter' do + it 'query context contains ci_environment_slug' do + expect(subject).to receive(:query_metrics).with(hash_including(ci_environment_slug: environment.slug)) - end + subject.query(*query_params) + end - describe 'project has kubernetes service' do - let(:project) { create(:kubernetes_project) } - let(:environment) { create(:environment, slug: 'environment-slug', project: project) } - let(:kube_namespace) { project.kubernetes_service.actual_namespace } - - it 'query context contains kube namespace' do + it 'query context contains environment filter' do expect(subject).to receive(:query_metrics).with( hash_including( - kube_namespace: kube_namespace) + environment_filter: "container_name!=\"POD\",environment=\"#{environment.slug}\"" + ) ) subject.query(*query_params) end end + + describe 'project has Kubernetes service' do + let(:project) { create(:kubernetes_project) } + let(:environment) { create(:environment, slug: 'environment-slug', project: project) } + let(:kube_namespace) { project.kubernetes_service.actual_namespace } + + it_behaves_like 'query context containing environment slug and filter' + + it 'query context contains kube_namespace' do + expect(subject).to receive(:query_metrics).with(hash_including(kube_namespace: kube_namespace)) + + subject.query(*query_params) + end + end + + describe 'project without Kubernetes service' do + it_behaves_like 'query context containing environment slug and filter' + + it 'query context contains empty kube_namespace' do + expect(subject).to receive(:query_metrics).with(hash_including(kube_namespace: '')) + + subject.query(*query_params) + end + end end context 'with one group where two metrics is found' do - let(:query_result) { described_class.new(client).query(*query_params) } - before do allow(metric_group_class).to receive(:all).and_return([simple_metric_group]) end @@ -77,7 +99,6 @@ RSpec.shared_examples 'additional metrics query' do end context 'with two groups with one metric each' do - let(:query_result) { described_class.new(client).query(*query_params) } let(:metrics) { [simple_metric(queries: [simple_query])] } before do From 6232543db3a784744e1cb03bb6a1a1648bb84b15 Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Mon, 31 Jul 2017 16:31:07 +0200 Subject: [PATCH 4/7] Add changelog: add support for kube_namespace in Metrics queries + small whitespace fix to better separate tests --- .../pawel-add_more_variables_to_additional_metrics-35267.yml | 4 ++++ spec/support/prometheus/additional_metrics_shared_examples.rb | 1 + 2 files changed, 5 insertions(+) create mode 100644 changelogs/unreleased/pawel-add_more_variables_to_additional_metrics-35267.yml diff --git a/changelogs/unreleased/pawel-add_more_variables_to_additional_metrics-35267.yml b/changelogs/unreleased/pawel-add_more_variables_to_additional_metrics-35267.yml new file mode 100644 index 00000000000..c1e831306df --- /dev/null +++ b/changelogs/unreleased/pawel-add_more_variables_to_additional_metrics-35267.yml @@ -0,0 +1,4 @@ +--- +title: Add support for kube_namespace in Metrics queries +merge_request: 16169 +author: diff --git a/spec/support/prometheus/additional_metrics_shared_examples.rb b/spec/support/prometheus/additional_metrics_shared_examples.rb index 174297af158..18b02920104 100644 --- a/spec/support/prometheus/additional_metrics_shared_examples.rb +++ b/spec/support/prometheus/additional_metrics_shared_examples.rb @@ -35,6 +35,7 @@ RSpec.shared_examples 'additional metrics query' do environment_filter: "container_name!=\"POD\",environment=\"#{environment.slug}\"" ) ) + subject.query(*query_params) end end From b243c3ea783df5070f889abbefc918f6599a916e Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 1 Aug 2017 20:28:10 +0200 Subject: [PATCH 5/7] Give metric query context rspec examples correctly sounding names. + missing whitespace --- lib/gitlab/prometheus/queries/query_additional_metrics.rb | 1 + spec/support/prometheus/additional_metrics_shared_examples.rb | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/prometheus/queries/query_additional_metrics.rb b/lib/gitlab/prometheus/queries/query_additional_metrics.rb index 5827c117556..d96921a9ee7 100644 --- a/lib/gitlab/prometheus/queries/query_additional_metrics.rb +++ b/lib/gitlab/prometheus/queries/query_additional_metrics.rb @@ -51,6 +51,7 @@ module Gitlab query[:query] %= context client_query(query[:query], time: context[:timeframe_end]) end + query[:result] = result&.map(&:deep_symbolize_keys) query end diff --git a/spec/support/prometheus/additional_metrics_shared_examples.rb b/spec/support/prometheus/additional_metrics_shared_examples.rb index 18b02920104..620fa37d455 100644 --- a/spec/support/prometheus/additional_metrics_shared_examples.rb +++ b/spec/support/prometheus/additional_metrics_shared_examples.rb @@ -23,13 +23,13 @@ RSpec.shared_examples 'additional metrics query' do subject! { described_class.new(client) } shared_examples 'query context containing environment slug and filter' do - it 'query context contains ci_environment_slug' do + it 'contains ci_environment_slug' do expect(subject).to receive(:query_metrics).with(hash_including(ci_environment_slug: environment.slug)) subject.query(*query_params) end - it 'query context contains environment filter' do + it 'contains environment filter' do expect(subject).to receive(:query_metrics).with( hash_including( environment_filter: "container_name!=\"POD\",environment=\"#{environment.slug}\"" From feb94e8ea3b003938f5df963d3c61757ffe27bcb Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 1 Aug 2017 21:51:53 +0200 Subject: [PATCH 6/7] Move timeframe_start and timeframe_end to common query context --- .../queries/additional_metrics_deployment_query.rb | 9 ++++----- .../queries/additional_metrics_environment_query.rb | 9 +-------- .../prometheus/queries/query_additional_metrics.rb | 4 +++- 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb b/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb index 51d934b9ae2..69d055c901c 100644 --- a/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb +++ b/lib/gitlab/prometheus/queries/additional_metrics_deployment_query.rb @@ -6,14 +6,13 @@ module Gitlab def query(deployment_id) Deployment.find_by(id: deployment_id).try do |deployment| - query_context = common_query_context(deployment.environment).merge( - { + query_metrics( + common_query_context( + deployment.environment, timeframe_start: (deployment.created_at - 30.minutes).to_f, timeframe_end: (deployment.created_at + 30.minutes).to_f - } + ) ) - - query_metrics(query_context) end end end diff --git a/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb b/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb index 9f798f5b892..580153556ea 100644 --- a/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb +++ b/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb @@ -6,14 +6,7 @@ module Gitlab def query(environment_id) Environment.find_by(id: environment_id).try do |environment| - query_context = common_query_context(environment).merge( - { - timeframe_start: 8.hours.ago.to_f, - timeframe_end: Time.now.to_f - } - ) - - query_metrics(query_context) + query_metrics(common_query_context(environment)) end end end diff --git a/lib/gitlab/prometheus/queries/query_additional_metrics.rb b/lib/gitlab/prometheus/queries/query_additional_metrics.rb index d96921a9ee7..d5f219ce6f9 100644 --- a/lib/gitlab/prometheus/queries/query_additional_metrics.rb +++ b/lib/gitlab/prometheus/queries/query_additional_metrics.rb @@ -71,8 +71,10 @@ module Gitlab result.select { |group| group.metrics.any? } end - def common_query_context(environment) + def common_query_context(environment, timeframe_start: 8.hours.ago.to_f, timeframe_end: Time.now.to_f) { + timeframe_start: timeframe_start, + timeframe_end: timeframe_end, ci_environment_slug: environment.slug, kube_namespace: environment.project.kubernetes_service&.actual_namespace || '', environment_filter: %{container_name!="POD",environment="#{environment.slug}"} From ba97a42193d85182cf0974b8aac508d40b6f368a Mon Sep 17 00:00:00 2001 From: Pawel Chojnacki Date: Tue, 1 Aug 2017 22:11:59 +0200 Subject: [PATCH 7/7] Remove default arguments for common query context --- .../queries/additional_metrics_environment_query.rb | 4 +++- lib/gitlab/prometheus/queries/query_additional_metrics.rb | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb b/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb index 580153556ea..db4708b22e4 100644 --- a/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb +++ b/lib/gitlab/prometheus/queries/additional_metrics_environment_query.rb @@ -6,7 +6,9 @@ module Gitlab def query(environment_id) Environment.find_by(id: environment_id).try do |environment| - query_metrics(common_query_context(environment)) + query_metrics( + common_query_context(environment, timeframe_start: 8.hours.ago.to_f, timeframe_end: Time.now.to_f) + ) end end end diff --git a/lib/gitlab/prometheus/queries/query_additional_metrics.rb b/lib/gitlab/prometheus/queries/query_additional_metrics.rb index d5f219ce6f9..7ac6162b54d 100644 --- a/lib/gitlab/prometheus/queries/query_additional_metrics.rb +++ b/lib/gitlab/prometheus/queries/query_additional_metrics.rb @@ -71,7 +71,7 @@ module Gitlab result.select { |group| group.metrics.any? } end - def common_query_context(environment, timeframe_start: 8.hours.ago.to_f, timeframe_end: Time.now.to_f) + def common_query_context(environment, timeframe_start:, timeframe_end:) { timeframe_start: timeframe_start, timeframe_end: timeframe_end,