diff options
Diffstat (limited to 'spec/lib/gitlab/metrics')
26 files changed, 1049 insertions, 252 deletions
diff --git a/spec/lib/gitlab/metrics/background_transaction_spec.rb b/spec/lib/gitlab/metrics/background_transaction_spec.rb index 640bbebf0da..b2a53fe1626 100644 --- a/spec/lib/gitlab/metrics/background_transaction_spec.rb +++ b/spec/lib/gitlab/metrics/background_transaction_spec.rb @@ -4,16 +4,30 @@ require 'spec_helper' RSpec.describe Gitlab::Metrics::BackgroundTransaction do let(:test_worker_class) { double(:class, name: 'TestWorker') } + let(:prometheus_metric) { instance_double(Prometheus::Client::Metric, base_labels: {}) } + + before do + allow(described_class).to receive(:prometheus_metric).and_return(prometheus_metric) + end subject { described_class.new(test_worker_class) } + RSpec.shared_examples 'metric with worker labels' do |metric_method| + it 'measures with correct labels and value' do + value = 1 + expect(prometheus_metric).to receive(metric_method).with({ controller: 'TestWorker', action: 'perform', feature_category: '' }, value) + + subject.send(metric_method, :bau, value) + end + end + describe '#label' do it 'returns labels based on class name' do expect(subject.labels).to eq(controller: 'TestWorker', action: 'perform', feature_category: '') end it 'contains only the labels defined for metrics' do - expect(subject.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABELS.keys) + expect(subject.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABEL_KEYS) end it 'includes the feature category if there is one' do @@ -21,4 +35,22 @@ RSpec.describe Gitlab::Metrics::BackgroundTransaction do expect(subject.labels).to include(feature_category: 'source_code_management') end end + + describe '#increment' do + let(:prometheus_metric) { instance_double(Prometheus::Client::Counter, :increment, base_labels: {}) } + + it_behaves_like 'metric with worker labels', :increment + end + + describe '#set' do + let(:prometheus_metric) { instance_double(Prometheus::Client::Gauge, :set, base_labels: {}) } + + it_behaves_like 'metric with worker labels', :set + end + + describe '#observe' do + let(:prometheus_metric) { instance_double(Prometheus::Client::Histogram, :observe, base_labels: {}) } + + it_behaves_like 'metric with worker labels', :observe + end end diff --git a/spec/lib/gitlab/metrics/dashboard/cache_spec.rb b/spec/lib/gitlab/metrics/dashboard/cache_spec.rb new file mode 100644 index 00000000000..9467d441ae1 --- /dev/null +++ b/spec/lib/gitlab/metrics/dashboard/cache_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Metrics::Dashboard::Cache, :use_clean_rails_memory_store_caching do + let_it_be(:project1) { build_stubbed(:project) } + let_it_be(:project2) { build_stubbed(:project) } + + let(:project1_key1) { "#{project1.id}_key1" } + let(:project1_key2) { "#{project1.id}_key2" } + let(:project2_key1) { "#{project2.id}_key1" } + + let(:cache1) { described_class.for(project1) } + let(:cache2) { described_class.for(project2) } + + before do + cache1.fetch(project1_key1) { 'data1' } + cache1.fetch(project1_key2) { 'data2' } + cache2.fetch(project2_key1) { 'data3' } + end + + describe '.fetch' do + it 'stores data correctly' do + described_class.fetch('key1') { 'data1' } + described_class.fetch('key2') { 'data2' } + + expect(described_class.fetch('key1')).to eq('data1') + expect(described_class.fetch('key2')).to eq('data2') + end + end + + describe '.for' do + it 'returns a new instance' do + expect(described_class.for(project1)).to be_instance_of(described_class) + end + end + + describe '#fetch' do + it 'stores data correctly' do + expect(cache1.fetch(project1_key1)).to eq('data1') + expect(cache1.fetch(project1_key2)).to eq('data2') + expect(cache2.fetch(project2_key1)).to eq('data3') + end + end + + describe '#delete_all!' do + it 'deletes keys of the given project', :aggregate_failures do + cache1.delete_all! + + expect(Rails.cache.exist?(project1_key1)).to be(false) + expect(Rails.cache.exist?(project1_key2)).to be(false) + expect(cache2.fetch(project2_key1)).to eq('data3') + + cache2.delete_all! + + expect(Rails.cache.exist?(project2_key1)).to be(false) + end + + it 'does not fail when nothing to delete' do + project3 = build_stubbed(:project) + cache3 = described_class.for(project3) + + expect { cache3.delete_all! }.not_to raise_error + end + end + + context 'multiple fetches and deletes' do + specify :aggregate_failures do + cache1.delete_all! + + expect(Rails.cache.exist?(project1_key1)).to be(false) + expect(Rails.cache.exist?(project1_key2)).to be(false) + + cache1.fetch("#{project1.id}_key3") { 'data1' } + cache1.fetch("#{project1.id}_key4") { 'data2' } + + expect(cache1.fetch("#{project1.id}_key3")).to eq('data1') + expect(cache1.fetch("#{project1.id}_key4")).to eq('data2') + + cache1.delete_all! + + expect(Rails.cache.exist?("#{project1.id}_key3")).to be(false) + expect(Rails.cache.exist?("#{project1.id}_key4")).to be(false) + end + end +end diff --git a/spec/lib/gitlab/metrics/dashboard/defaults_spec.rb b/spec/lib/gitlab/metrics/dashboard/defaults_spec.rb index dd61f8ebc4d..1f306753c39 100644 --- a/spec/lib/gitlab/metrics/dashboard/defaults_spec.rb +++ b/spec/lib/gitlab/metrics/dashboard/defaults_spec.rb @@ -4,5 +4,4 @@ require 'spec_helper' RSpec.describe Gitlab::Metrics::Dashboard::Defaults do it { is_expected.to be_const_defined(:DEFAULT_PANEL_TYPE) } - it { is_expected.to be_const_defined(:DEFAULT_PANEL_WEIGHT) } end diff --git a/spec/lib/gitlab/metrics/dashboard/finder_spec.rb b/spec/lib/gitlab/metrics/dashboard/finder_spec.rb index 60e1e29d4c5..730a31346d7 100644 --- a/spec/lib/gitlab/metrics/dashboard/finder_spec.rb +++ b/spec/lib/gitlab/metrics/dashboard/finder_spec.rb @@ -142,20 +142,42 @@ RSpec.describe Gitlab::Metrics::Dashboard::Finder, :use_clean_rails_memory_store describe '.find_all_paths' do let(:all_dashboard_paths) { described_class.find_all_paths(project) } - let(:system_dashboard) { { path: system_dashboard_path, display_name: 'Default dashboard', default: true, system_dashboard: true, out_of_the_box_dashboard: true } } + let(:system_dashboard) { { path: system_dashboard_path, display_name: 'Overview', default: true, system_dashboard: true, out_of_the_box_dashboard: true } } + let(:k8s_pod_health_dashboard) { { path: pod_dashboard_path, display_name: 'K8s pod health', default: false, system_dashboard: false, out_of_the_box_dashboard: true } } - it 'includes only the system dashboard by default' do - expect(all_dashboard_paths).to eq([system_dashboard]) + it 'includes OOTB dashboards by default' do + expect(all_dashboard_paths).to eq([k8s_pod_health_dashboard, system_dashboard]) end context 'when the project contains dashboards' do - let(:dashboard_path) { '.gitlab/dashboards/test.yml' } - let(:project) { project_with_dashboard(dashboard_path) } + let(:dashboard_content) { fixture_file('lib/gitlab/metrics/dashboard/sample_dashboard.yml') } + let(:project) { project_with_dashboards(dashboards) } - it 'includes system and project dashboards' do - project_dashboard = { path: dashboard_path, display_name: 'test.yml', default: false, system_dashboard: false, out_of_the_box_dashboard: false } + let(:dashboards) do + { + '.gitlab/dashboards/metrics.yml' => dashboard_content, + '.gitlab/dashboards/better_metrics.yml' => dashboard_content + } + end - expect(all_dashboard_paths).to contain_exactly(system_dashboard, project_dashboard) + it 'includes OOTB and project dashboards' do + project_dashboard1 = { + path: '.gitlab/dashboards/metrics.yml', + display_name: 'metrics.yml', + default: false, + system_dashboard: false, + out_of_the_box_dashboard: false + } + + project_dashboard2 = { + path: '.gitlab/dashboards/better_metrics.yml', + display_name: 'better_metrics.yml', + default: false, + system_dashboard: false, + out_of_the_box_dashboard: false + } + + expect(all_dashboard_paths).to eq([project_dashboard2, k8s_pod_health_dashboard, project_dashboard1, system_dashboard]) end end @@ -163,12 +185,13 @@ RSpec.describe Gitlab::Metrics::Dashboard::Finder, :use_clean_rails_memory_store let(:self_monitoring_dashboard) do { path: self_monitoring_dashboard_path, - display_name: 'Default dashboard', + display_name: 'Overview', default: true, - system_dashboard: false, + system_dashboard: true, out_of_the_box_dashboard: true } end + let(:dashboard_path) { '.gitlab/dashboards/test.yml' } let(:project) { project_with_dashboard(dashboard_path) } @@ -185,7 +208,7 @@ RSpec.describe Gitlab::Metrics::Dashboard::Finder, :use_clean_rails_memory_store out_of_the_box_dashboard: false } - expect(all_dashboard_paths).to contain_exactly(self_monitoring_dashboard, project_dashboard) + expect(all_dashboard_paths).to eq([self_monitoring_dashboard, project_dashboard]) end end end diff --git a/spec/lib/gitlab/metrics/dashboard/processor_spec.rb b/spec/lib/gitlab/metrics/dashboard/processor_spec.rb index 7f7070dfafb..14a4c01fce3 100644 --- a/spec/lib/gitlab/metrics/dashboard/processor_spec.rb +++ b/spec/lib/gitlab/metrics/dashboard/processor_spec.rb @@ -16,7 +16,6 @@ RSpec.describe Gitlab::Metrics::Dashboard::Processor do Gitlab::Metrics::Dashboard::Stages::CustomMetricsInserter, Gitlab::Metrics::Dashboard::Stages::CustomMetricsDetailsInserter, Gitlab::Metrics::Dashboard::Stages::MetricEndpointInserter, - Gitlab::Metrics::Dashboard::Stages::Sorter, Gitlab::Metrics::Dashboard::Stages::AlertsInserter, Gitlab::Metrics::Dashboard::Stages::PanelIdsInserter, Gitlab::Metrics::Dashboard::Stages::UrlValidator @@ -26,12 +25,6 @@ RSpec.describe Gitlab::Metrics::Dashboard::Processor do let(:process_params) { [project, dashboard_yml, sequence, { environment: environment }] } let(:dashboard) { described_class.new(*process_params).process } - it 'includes a path for the prometheus endpoint with each metric' do - expect(all_metrics).to satisfy_all do |metric| - metric[:prometheus_endpoint_path] == prometheus_path(metric[:query_range]) - end - end - it 'includes an id for each dashboard panel' do expect(all_panels).to satisfy_all do |panel| panel[:id].present? @@ -72,14 +65,14 @@ RSpec.describe Gitlab::Metrics::Dashboard::Processor do expect(all_metrics).to include get_metric_details(project_business_metric) end - it 'orders groups by priority and panels by weight' do + it 'display groups and panels in the order they are defined' do expected_metrics_order = [ - 'metric_b', # group priority 10, panel weight 1 - 'metric_a2', # group priority 1, panel weight 2 - 'metric_a1', # group priority 1, panel weight 1 - project_business_metric.id, # group priority 0, panel weight nil (0) - project_response_metric.id, # group priority -5, panel weight nil (0) - project_system_metric.id # group priority -10, panel weight nil (0) + 'metric_b', + 'metric_a2', + 'metric_a1', + project_business_metric.id, + project_response_metric.id, + project_system_metric.id ] actual_metrics_order = all_metrics.map { |m| m[:id] || m[:metric_id] } @@ -100,10 +93,10 @@ RSpec.describe Gitlab::Metrics::Dashboard::Processor do let(:sequence) do [ Gitlab::Metrics::Dashboard::Stages::CommonMetricsInserter, - Gitlab::Metrics::Dashboard::Stages::MetricEndpointInserter, - Gitlab::Metrics::Dashboard::Stages::Sorter + Gitlab::Metrics::Dashboard::Stages::MetricEndpointInserter ] end + let(:dashboard) { described_class.new(*process_params).process } it 'includes only dashboard metrics' do diff --git a/spec/lib/gitlab/metrics/dashboard/repo_dashboard_finder_spec.rb b/spec/lib/gitlab/metrics/dashboard/repo_dashboard_finder_spec.rb new file mode 100644 index 00000000000..a2c9906c0e9 --- /dev/null +++ b/spec/lib/gitlab/metrics/dashboard/repo_dashboard_finder_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Metrics::Dashboard::RepoDashboardFinder do + include MetricsDashboardHelpers + + let_it_be(:project) { create(:project) } + + describe '.list_dashboards' do + it 'deletes dashboard cache entries' do + cache = instance_double(Gitlab::Metrics::Dashboard::Cache) + allow(Gitlab::Metrics::Dashboard::Cache).to receive(:for).and_return(cache) + + expect(cache).to receive(:delete_all!) + + described_class.list_dashboards(project) + end + + it 'returns empty array when there are no dashboards' do + expect(described_class.list_dashboards(project)).to eq([]) + end + + context 'when there are project dashboards available' do + let_it_be(:dashboard_path) { '.gitlab/dashboards/test.yml' } + let_it_be(:project) { project_with_dashboard(dashboard_path) } + + it 'returns the dashboard list' do + expect(described_class.list_dashboards(project)).to contain_exactly(dashboard_path) + end + end + end + + describe '.read_dashboard' do + it 'raises error when dashboard does not exist' do + dashboard_path = '.gitlab/dashboards/test.yml' + + expect { described_class.read_dashboard(project, dashboard_path) }.to raise_error( + Gitlab::Metrics::Dashboard::Errors::NOT_FOUND_ERROR + ) + end + + context 'when there are project dashboards available' do + let_it_be(:dashboard_path) { '.gitlab/dashboards/test.yml' } + let_it_be(:project) { project_with_dashboard(dashboard_path) } + + it 'reads dashboard' do + expect(described_class.read_dashboard(project, dashboard_path)).to eq( + fixture_file('lib/gitlab/metrics/dashboard/sample_dashboard.yml') + ) + end + end + end +end diff --git a/spec/lib/gitlab/metrics/dashboard/stages/metric_endpoint_inserter_spec.rb b/spec/lib/gitlab/metrics/dashboard/stages/metric_endpoint_inserter_spec.rb new file mode 100644 index 00000000000..bb3c8626d32 --- /dev/null +++ b/spec/lib/gitlab/metrics/dashboard/stages/metric_endpoint_inserter_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Metrics::Dashboard::Stages::MetricEndpointInserter do + include MetricsDashboardHelpers + + let(:project) { build_stubbed(:project) } + let(:environment) { build_stubbed(:environment, project: project) } + + describe '#transform!' do + subject(:transform!) { described_class.new(project, dashboard, environment: environment).transform! } + + let(:dashboard) { load_sample_dashboard.deep_symbolize_keys } + + it 'generates prometheus_endpoint_path without newlines' do + query = 'avg( sum( container_memory_usage_bytes{ container_name!="POD", '\ + 'pod_name=~"^{{ci_environment_slug}}-(.*)", namespace="{{kube_namespace}}" } ) '\ + 'by (job) ) without (job) /1024/1024/1024' + + transform! + + expect(all_metrics[2][:prometheus_endpoint_path]).to eq(prometheus_path(query)) + end + + it 'includes a path for the prometheus endpoint with each metric' do + transform! + + expect(all_metrics).to satisfy_all do |metric| + metric[:prometheus_endpoint_path].present? && !metric[:prometheus_endpoint_path].include?("\n") + end + end + + it 'works when query/query_range is a number' do + query = 2000 + + transform! + + expect(all_metrics[1][:prometheus_endpoint_path]).to eq(prometheus_path(query)) + end + end + + private + + def all_metrics + dashboard[:panel_groups].flat_map do |group| + group[:panels].flat_map { |panel| panel[:metrics] } + end + end + + def prometheus_path(query) + Gitlab::Routing.url_helpers.prometheus_api_project_environment_path( + project, + environment, + proxy_path: :query_range, + query: query + ) + end +end diff --git a/spec/lib/gitlab/metrics/dashboard/stages/track_panel_type_spec.rb b/spec/lib/gitlab/metrics/dashboard/stages/track_panel_type_spec.rb new file mode 100644 index 00000000000..d9987b67127 --- /dev/null +++ b/spec/lib/gitlab/metrics/dashboard/stages/track_panel_type_spec.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Metrics::Dashboard::Stages::TrackPanelType do + include MetricsDashboardHelpers + + let(:project) { build_stubbed(:project) } + let(:environment) { build_stubbed(:environment, project: project) } + + describe '#transform!' do + subject { described_class.new(project, dashboard, environment: environment) } + + let(:dashboard) { load_sample_dashboard.deep_symbolize_keys } + + it 'creates tracking event' do + stub_application_setting(snowplow_enabled: true, snowplow_collector_hostname: 'localhost') + allow(Gitlab::Tracking).to receive(:event).and_call_original + + subject.transform! + + expect(Gitlab::Tracking).to have_received(:event) + .with('MetricsDashboard::Chart', 'chart_rendered', { label: 'area-chart' }) + .at_least(:once) + end + end +end diff --git a/spec/lib/gitlab/metrics/dashboard/url_spec.rb b/spec/lib/gitlab/metrics/dashboard/url_spec.rb index 56556423b05..205e1000376 100644 --- a/spec/lib/gitlab/metrics/dashboard/url_spec.rb +++ b/spec/lib/gitlab/metrics/dashboard/url_spec.rb @@ -102,6 +102,34 @@ RSpec.describe Gitlab::Metrics::Dashboard::Url do it_behaves_like 'regex which matches url when expected' end + describe '#alert_regex' do + let(:url) do + Gitlab::Routing.url_helpers.metrics_dashboard_namespace_project_prometheus_alert_url( + 'foo', + 'bar', + '1', + start: '2020-02-10T12:59:49.938Z', + end: '2020-02-10T20:59:49.938Z', + anchor: "anchor" + ) + end + + let(:expected_params) do + { + 'url' => url, + 'namespace' => 'foo', + 'project' => 'bar', + 'alert' => '1', + 'query' => "?end=2020-02-10T20%3A59%3A49.938Z&start=2020-02-10T12%3A59%3A49.938Z", + 'anchor' => '#anchor' + } + end + + subject { described_class.alert_regex } + + it_behaves_like 'regex which matches url when expected' + end + describe '#build_dashboard_url' do it 'builds the url for the dashboard endpoint' do url = described_class.build_dashboard_url('foo', 'bar', 1) diff --git a/spec/lib/gitlab/metrics/dashboard/validator/client_spec.rb b/spec/lib/gitlab/metrics/dashboard/validator/client_spec.rb new file mode 100644 index 00000000000..4b07f9dbbab --- /dev/null +++ b/spec/lib/gitlab/metrics/dashboard/validator/client_spec.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Metrics::Dashboard::Validator::Client do + include MetricsDashboardHelpers + + let_it_be(:schema_path) { 'lib/gitlab/metrics/dashboard/validator/schemas/dashboard.json' } + + subject { described_class.new(dashboard, schema_path) } + + describe '#execute' do + context 'with no validation errors' do + let(:dashboard) { load_sample_dashboard } + + it 'returns empty array' do + expect(subject.execute).to eq([]) + end + end + + context 'with validation errors' do + let(:dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/invalid_dashboard.yml')) } + + it 'returns array of error objects' do + expect(subject.execute).to include(Gitlab::Metrics::Dashboard::Validator::Errors::SchemaValidationError) + end + end + end +end diff --git a/spec/lib/gitlab/metrics/dashboard/validator/custom_formats_spec.rb b/spec/lib/gitlab/metrics/dashboard/validator/custom_formats_spec.rb new file mode 100644 index 00000000000..129fb631f3e --- /dev/null +++ b/spec/lib/gitlab/metrics/dashboard/validator/custom_formats_spec.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Metrics::Dashboard::Validator::CustomFormats do + describe '#format_handlers' do + describe 'add_to_metric_id_cache' do + it 'adds data to metric id cache' do + subject.format_handlers['add_to_metric_id_cache'].call('metric_id', '_schema') + + expect(subject.metric_ids_cache).to eq(["metric_id"]) + end + end + end +end diff --git a/spec/lib/gitlab/metrics/dashboard/validator/errors_spec.rb b/spec/lib/gitlab/metrics/dashboard/validator/errors_spec.rb new file mode 100644 index 00000000000..f0db1bd0d33 --- /dev/null +++ b/spec/lib/gitlab/metrics/dashboard/validator/errors_spec.rb @@ -0,0 +1,140 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Metrics::Dashboard::Validator::Errors do + describe Gitlab::Metrics::Dashboard::Validator::Errors::SchemaValidationError do + context 'empty error hash' do + let(:error_hash) { {} } + + it 'uses default error message' do + expect(described_class.new(error_hash).message).to eq('Dashboard failed schema validation') + end + end + + context 'formatted message' do + subject { described_class.new(error_hash).message } + + let(:error_hash) do + { + 'data' => 'property_name', + 'data_pointer' => pointer, + 'type' => type, + 'schema' => 'schema', + 'details' => details + } + end + + context 'for root object' do + let(:pointer) { '' } + + context 'when required keys are missing' do + let(:type) { 'required' } + let(:details) { { 'missing_keys' => ['one'] } } + + it { is_expected.to eq 'root is missing required keys: one' } + end + end + + context 'for nested object' do + let(:pointer) { '/nested_objects/0' } + + context 'when required keys are missing' do + let(:type) { 'required' } + let(:details) { { 'missing_keys' => ['two'] } } + + it { is_expected.to eq '/nested_objects/0 is missing required keys: two' } + end + + context 'when there is type mismatch' do + %w(null string boolean integer number array object).each do |expected_type| + context "on type: #{expected_type}" do + let(:type) { expected_type } + let(:details) { nil } + + subject { described_class.new(error_hash).message } + + it { is_expected.to eq "'property_name' at /nested_objects/0 is not of type: #{expected_type}" } + end + end + end + + context 'when data does not match pattern' do + let(:type) { 'pattern' } + let(:error_hash) do + { + 'data' => 'property_name', + 'data_pointer' => pointer, + 'type' => type, + 'schema' => { 'pattern' => 'aa.*' } + } + end + + it { is_expected.to eq "'property_name' at /nested_objects/0 does not match pattern: aa.*" } + end + + context 'when data does not match format' do + let(:type) { 'format' } + let(:error_hash) do + { + 'data' => 'property_name', + 'data_pointer' => pointer, + 'type' => type, + 'schema' => { 'format' => 'date-time' } + } + end + + it { is_expected.to eq "'property_name' at /nested_objects/0 does not match format: date-time" } + end + + context 'when data is not const' do + let(:type) { 'const' } + let(:error_hash) do + { + 'data' => 'property_name', + 'data_pointer' => pointer, + 'type' => type, + 'schema' => { 'const' => 'one' } + } + end + + it { is_expected.to eq "'property_name' at /nested_objects/0 is not: \"one\"" } + end + + context 'when data is not included in enum' do + let(:type) { 'enum' } + let(:error_hash) do + { + 'data' => 'property_name', + 'data_pointer' => pointer, + 'type' => type, + 'schema' => { 'enum' => %w(one two) } + } + end + + it { is_expected.to eq "'property_name' at /nested_objects/0 is not one of: [\"one\", \"two\"]" } + end + + context 'when data is not included in enum' do + let(:type) { 'unknown' } + let(:error_hash) do + { + 'data' => 'property_name', + 'data_pointer' => pointer, + 'type' => type, + 'schema' => 'schema' + } + end + + it { is_expected.to eq "'property_name' at /nested_objects/0 is invalid: error_type=unknown" } + end + end + end + end + + describe Gitlab::Metrics::Dashboard::Validator::Errors::DuplicateMetricIds do + it 'has custom error message' do + expect(described_class.new.message).to eq('metric_id must be unique across a project') + end + end +end diff --git a/spec/lib/gitlab/metrics/dashboard/validator/post_schema_validator_spec.rb b/spec/lib/gitlab/metrics/dashboard/validator/post_schema_validator_spec.rb new file mode 100644 index 00000000000..e7cb1429ca9 --- /dev/null +++ b/spec/lib/gitlab/metrics/dashboard/validator/post_schema_validator_spec.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Metrics::Dashboard::Validator::PostSchemaValidator do + describe '#validate' do + context 'with no project and dashboard_path provided' do + context 'unique local metric_ids' do + it 'returns empty array' do + expect(described_class.new(metric_ids: [1, 2, 3]).validate).to eq([]) + end + end + + context 'duplicate local metrics_ids' do + it 'returns error' do + expect(described_class.new(metric_ids: [1, 1]).validate) + .to eq([Gitlab::Metrics::Dashboard::Validator::Errors::DuplicateMetricIds]) + end + end + end + + context 'with project and dashboard_path' do + let(:project) { create(:project) } + + subject do + described_class.new( + project: project, + metric_ids: ['some_identifier'], + dashboard_path: 'test/path.yml' + ).validate + end + + context 'with unique metric identifiers' do + before do + create(:prometheus_metric, + project: project, + identifier: 'some_other_identifier', + dashboard_path: 'test/path.yml' + ) + end + + it 'returns empty array' do + expect(subject).to eq([]) + end + end + + context 'duplicate metric identifiers in database' do + context 'with different dashboard_path' do + before do + create(:prometheus_metric, + project: project, + identifier: 'some_identifier', + dashboard_path: 'some/other/path.yml' + ) + end + + it 'returns error' do + expect(subject).to include(Gitlab::Metrics::Dashboard::Validator::Errors::DuplicateMetricIds) + end + end + + context 'with same dashboard_path' do + before do + create(:prometheus_metric, + project: project, + identifier: 'some_identifier', + dashboard_path: 'test/path.yml' + ) + end + + it 'returns empty array' do + expect(subject).to eq([]) + end + end + end + end + end +end diff --git a/spec/lib/gitlab/metrics/dashboard/validator_spec.rb b/spec/lib/gitlab/metrics/dashboard/validator_spec.rb new file mode 100644 index 00000000000..c4cda271408 --- /dev/null +++ b/spec/lib/gitlab/metrics/dashboard/validator_spec.rb @@ -0,0 +1,146 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Metrics::Dashboard::Validator do + include MetricsDashboardHelpers + + let_it_be(:valid_dashboard) { load_sample_dashboard } + let_it_be(:invalid_dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/invalid_dashboard.yml')) } + let_it_be(:duplicate_id_dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/duplicate_id_dashboard.yml')) } + + let_it_be(:project) { create(:project) } + + describe '#validate' do + context 'valid dashboard schema' do + it 'returns true' do + expect(described_class.validate(valid_dashboard)).to be true + end + + context 'with duplicate metric_ids' do + it 'returns false' do + expect(described_class.validate(duplicate_id_dashboard)).to be false + end + end + + context 'with dashboard_path and project' do + subject { described_class.validate(valid_dashboard, dashboard_path: 'test/path.yml', project: project) } + + context 'with no conflicting metric identifiers in db' do + it { is_expected.to be true } + end + + context 'with metric identifier present in current dashboard' do + before do + create(:prometheus_metric, + identifier: 'metric_a1', + dashboard_path: 'test/path.yml', + project: project + ) + end + + it { is_expected.to be true } + end + + context 'with metric identifier present in another dashboard' do + before do + create(:prometheus_metric, + identifier: 'metric_a1', + dashboard_path: 'some/other/dashboard/path.yml', + project: project + ) + end + + it { is_expected.to be false } + end + end + end + + context 'invalid dashboard schema' do + it 'returns false' do + expect(described_class.validate(invalid_dashboard)).to be false + end + end + end + + describe '#validate!' do + shared_examples 'validation failed' do |errors_message| + it 'raises error with corresponding messages', :aggregate_failures do + expect { subject }.to raise_error do |error| + expect(error).to be_kind_of(Gitlab::Metrics::Dashboard::Validator::Errors::InvalidDashboardError) + expect(error.message).to eq(errors_message) + end + end + end + + context 'valid dashboard schema' do + it 'returns true' do + expect(described_class.validate!(valid_dashboard)).to be true + end + + context 'with duplicate metric_ids' do + subject { described_class.validate!(duplicate_id_dashboard) } + + it_behaves_like 'validation failed', 'metric_id must be unique across a project' + end + + context 'with dashboard_path and project' do + subject { described_class.validate!(valid_dashboard, dashboard_path: 'test/path.yml', project: project) } + + context 'with no conflicting metric identifiers in db' do + it { is_expected.to be true } + end + + context 'with metric identifier present in current dashboard' do + before do + create(:prometheus_metric, + identifier: 'metric_a1', + dashboard_path: 'test/path.yml', + project: project + ) + end + + it { is_expected.to be true } + end + + context 'with metric identifier present in another dashboard' do + before do + create(:prometheus_metric, + identifier: 'metric_a1', + dashboard_path: 'some/other/dashboard/path.yml', + project: project + ) + end + + it_behaves_like 'validation failed', 'metric_id must be unique across a project' + end + end + end + + context 'invalid dashboard schema' do + subject { described_class.validate!(invalid_dashboard) } + + context 'wrong property type' do + it_behaves_like 'validation failed', "'this_should_be_a_int' at /panel_groups/0/panels/0/weight is not of type: number" + end + + context 'panel groups missing' do + let_it_be(:invalid_dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/dashboard_missing_panel_groups.yml')) } + + it_behaves_like 'validation failed', 'root is missing required keys: panel_groups' + end + + context 'groups are missing panels and group keys' do + let_it_be(:invalid_dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/dashboard_groups_missing_panels_and_group.yml')) } + + it_behaves_like 'validation failed', '/panel_groups/0 is missing required keys: group' + end + + context 'panel is missing metrics key' do + let_it_be(:invalid_dashboard) { load_dashboard_yaml(fixture_file('lib/gitlab/metrics/dashboard/dashboard_panel_is_missing_metrics.yml')) } + + it_behaves_like 'validation failed', '/panel_groups/0/panels/0 is missing required keys: metrics' + end + end + end +end diff --git a/spec/lib/gitlab/metrics/elasticsearch_rack_middleware_spec.rb b/spec/lib/gitlab/metrics/elasticsearch_rack_middleware_spec.rb index 1fbd41bcc88..78b73f148e4 100644 --- a/spec/lib/gitlab/metrics/elasticsearch_rack_middleware_spec.rb +++ b/spec/lib/gitlab/metrics/elasticsearch_rack_middleware_spec.rb @@ -9,8 +9,6 @@ RSpec.describe Gitlab::Metrics::ElasticsearchRackMiddleware do let(:transaction) { Gitlab::Metrics::WebTransaction.new(env) } describe '#call' do - let(:counter) { instance_double(Prometheus::Client::Counter, increment: nil) } - let(:histogram) { instance_double(Prometheus::Client::Histogram, observe: nil) } let(:elasticsearch_query_time) { 0.1 } let(:elasticsearch_requests_count) { 2 } @@ -18,19 +16,6 @@ RSpec.describe Gitlab::Metrics::ElasticsearchRackMiddleware do allow(Gitlab::Instrumentation::ElasticsearchTransport).to receive(:query_time) { elasticsearch_query_time } allow(Gitlab::Instrumentation::ElasticsearchTransport).to receive(:get_request_count) { elasticsearch_requests_count } - allow(Gitlab::Metrics).to receive(:counter) - .with(:http_elasticsearch_requests_total, - an_instance_of(String), - Gitlab::Metrics::Transaction::BASE_LABELS) - .and_return(counter) - - allow(Gitlab::Metrics).to receive(:histogram) - .with(:http_elasticsearch_requests_duration_seconds, - an_instance_of(String), - Gitlab::Metrics::Transaction::BASE_LABELS, - described_class::HISTOGRAM_BUCKETS) - .and_return(histogram) - allow(Gitlab::Metrics).to receive(:current_transaction).and_return(transaction) end @@ -39,19 +24,30 @@ RSpec.describe Gitlab::Metrics::ElasticsearchRackMiddleware do end it 'records elasticsearch metrics' do - expect(counter).to receive(:increment).with(transaction.labels, elasticsearch_requests_count) - expect(histogram).to receive(:observe).with(transaction.labels, elasticsearch_query_time) + expect(transaction).to receive(:increment).with(:http_elasticsearch_requests_total, elasticsearch_requests_count) + expect(transaction).to receive(:observe).with(:http_elasticsearch_requests_duration_seconds, elasticsearch_query_time) middleware.call(env) end it 'records elasticsearch metrics if an error is raised' do - expect(counter).to receive(:increment).with(transaction.labels, elasticsearch_requests_count) - expect(histogram).to receive(:observe).with(transaction.labels, elasticsearch_query_time) + expect(transaction).to receive(:increment).with(:http_elasticsearch_requests_total, elasticsearch_requests_count) + expect(transaction).to receive(:observe).with(:http_elasticsearch_requests_duration_seconds, elasticsearch_query_time) allow(app).to receive(:call).with(env).and_raise(StandardError) expect { middleware.call(env) }.to raise_error(StandardError) end + + context 'when there are no elasticsearch requests' do + let(:elasticsearch_requests_count) { 0 } + + it 'does not record any metrics' do + expect(transaction).not_to receive(:observe).with(:http_elasticsearch_requests_duration_seconds) + expect(transaction).not_to receive(:increment).with(:http_elasticsearch_requests_total, 0) + + middleware.call(env) + end + end end end diff --git a/spec/lib/gitlab/metrics/method_call_spec.rb b/spec/lib/gitlab/metrics/method_call_spec.rb index 42361cbc36a..825c91b6cb4 100644 --- a/spec/lib/gitlab/metrics/method_call_spec.rb +++ b/spec/lib/gitlab/metrics/method_call_spec.rb @@ -3,12 +3,12 @@ require 'spec_helper' RSpec.describe Gitlab::Metrics::MethodCall do - let(:transaction) { double(:transaction, labels: {}) } + let(:transaction) { Gitlab::Metrics::WebTransaction.new({}) } let(:method_call) { described_class.new('Foo#bar', :Foo, '#bar', transaction) } describe '#measure' do after do - described_class.reload_metric!(:gitlab_method_call_duration_seconds) + ::Gitlab::Metrics::Transaction.reload_metric!(:gitlab_method_call_duration_seconds) end it 'measures the performance of the supplied block' do @@ -36,13 +36,13 @@ RSpec.describe Gitlab::Metrics::MethodCall do end it 'metric is not a NullMetric' do - expect(described_class).not_to be_instance_of(Gitlab::Metrics::NullMetric) + method_call.measure { 'foo' } + expect(::Gitlab::Metrics::Transaction.prometheus_metric(:gitlab_method_call_duration_seconds, :histogram)).not_to be_instance_of(Gitlab::Metrics::NullMetric) end it 'observes the performance of the supplied block' do - expect(described_class.gitlab_method_call_duration_seconds) - .to receive(:observe) - .with({ module: :Foo, method: '#bar' }, be_a_kind_of(Numeric)) + expect(transaction) + .to receive(:observe).with(:gitlab_method_call_duration_seconds, be_a_kind_of(Numeric), { method: "#bar", module: :Foo }) method_call.measure { 'foo' } end @@ -53,11 +53,17 @@ RSpec.describe Gitlab::Metrics::MethodCall do stub_feature_flags(prometheus_metrics_method_instrumentation: false) end - it 'observes using NullMetric' do - expect(described_class.gitlab_method_call_duration_seconds).to be_instance_of(Gitlab::Metrics::NullMetric) - expect(described_class.gitlab_method_call_duration_seconds).to receive(:observe) + it 'observes the performance of the supplied block' do + expect(transaction) + .to receive(:observe).with(:gitlab_method_call_duration_seconds, be_a_kind_of(Numeric), { method: "#bar", module: :Foo }) + + method_call.measure { 'foo' } + end + it 'observes using NullMetric' do method_call.measure { 'foo' } + + expect(::Gitlab::Metrics::Transaction.prometheus_metric(:gitlab_method_call_duration_seconds, :histogram)).to be_instance_of(Gitlab::Metrics::NullMetric) end end end @@ -68,8 +74,9 @@ RSpec.describe Gitlab::Metrics::MethodCall do end it 'does not observe the performance' do - expect(described_class.gitlab_method_call_duration_seconds) + expect(transaction) .not_to receive(:observe) + .with(:gitlab_method_call_duration_seconds, be_a_kind_of(Numeric)) method_call.measure { 'foo' } end diff --git a/spec/lib/gitlab/metrics/methods_spec.rb b/spec/lib/gitlab/metrics/methods_spec.rb index 3c171680272..71135a6e9c5 100644 --- a/spec/lib/gitlab/metrics/methods_spec.rb +++ b/spec/lib/gitlab/metrics/methods_spec.rb @@ -9,9 +9,9 @@ RSpec.describe Gitlab::Metrics::Methods do let(:docstring) { 'description' } let(:metric_name) { :sample_metric } - describe "#define_#{metric_type}" do + describe "#define_metrics" do define_method(:call_define_metric_method) do |**args| - subject.__send__("define_#{metric_type}", metric_name, **args) + subject.__send__(:define_metric, metric_type, metric_name, **args) end context 'metrics access method not defined' do @@ -55,11 +55,11 @@ RSpec.describe Gitlab::Metrics::Methods do end end - describe "#fetch_#{metric_type}" do + describe "#fetch_metric" do let(:null_metric) { Gitlab::Metrics::NullMetric.instance } define_method(:call_fetch_metric_method) do |**args| - subject.__send__("fetch_#{metric_type}", metric_name, **args) + subject.__send__(:fetch_metric, metric_type, metric_name, **args) end context "when #{metric_type} is not cached" do @@ -135,5 +135,5 @@ RSpec.describe Gitlab::Metrics::Methods do include_examples 'metric', :counter, {} include_examples 'metric', :gauge, {}, :all - include_examples 'metric', :histogram, {}, [0.005, 0.01, 0.1, 1, 10] + include_examples 'metric', :histogram, {}, ::Prometheus::Client::Histogram::DEFAULT_BUCKETS end diff --git a/spec/lib/gitlab/metrics/rack_middleware_spec.rb b/spec/lib/gitlab/metrics/rack_middleware_spec.rb index 335e5a490a6..ab56f38f0c1 100644 --- a/spec/lib/gitlab/metrics/rack_middleware_spec.rb +++ b/spec/lib/gitlab/metrics/rack_middleware_spec.rb @@ -25,12 +25,4 @@ RSpec.describe Gitlab::Metrics::RackMiddleware do expect { middleware.call(env) }.to raise_error(RuntimeError) end end - - describe '#transaction_from_env' do - let(:transaction) { middleware.transaction_from_env(env) } - - it 'returns a Transaction' do - expect(transaction).to be_an_instance_of(Gitlab::Metrics::WebTransaction) - end - end end diff --git a/spec/lib/gitlab/metrics/redis_rack_middleware_spec.rb b/spec/lib/gitlab/metrics/redis_rack_middleware_spec.rb deleted file mode 100644 index a85968dbd43..00000000000 --- a/spec/lib/gitlab/metrics/redis_rack_middleware_spec.rb +++ /dev/null @@ -1,61 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::Metrics::RedisRackMiddleware do - let(:app) { double(:app) } - let(:middleware) { described_class.new(app) } - let(:env) { {} } - let(:transaction) { Gitlab::Metrics::WebTransaction.new(env) } - - before do - allow(app).to receive(:call).with(env).and_return('wub wub') - end - - describe '#call' do - let(:counter) { double(Prometheus::Client::Counter, increment: nil) } - let(:histogram) { double(Prometheus::Client::Histogram, observe: nil) } - let(:redis_query_time) { 0.1 } - let(:redis_requests_count) { 2 } - - before do - allow(Gitlab::Instrumentation::Redis).to receive(:query_time) { redis_query_time } - allow(Gitlab::Instrumentation::Redis).to receive(:get_request_count) { redis_requests_count } - - allow(Gitlab::Metrics).to receive(:counter) - .with(:http_redis_requests_total, - an_instance_of(String), - Gitlab::Metrics::Transaction::BASE_LABELS) - .and_return(counter) - - allow(Gitlab::Metrics).to receive(:histogram) - .with(:http_redis_requests_duration_seconds, - an_instance_of(String), - Gitlab::Metrics::Transaction::BASE_LABELS, - Gitlab::Instrumentation::Redis::QUERY_TIME_BUCKETS) - .and_return(histogram) - - allow(Gitlab::Metrics).to receive(:current_transaction).and_return(transaction) - end - - it 'calls the app' do - expect(middleware.call(env)).to eq('wub wub') - end - - it 'records redis metrics' do - expect(counter).to receive(:increment).with(transaction.labels, redis_requests_count) - expect(histogram).to receive(:observe).with(transaction.labels, redis_query_time) - - middleware.call(env) - end - - it 'records redis metrics if an error is raised' do - expect(counter).to receive(:increment).with(transaction.labels, redis_requests_count) - expect(histogram).to receive(:observe).with(transaction.labels, redis_query_time) - - allow(app).to receive(:call).with(env).and_raise(StandardError) - - expect { middleware.call(env) }.to raise_error(StandardError) - end - end -end diff --git a/spec/lib/gitlab/metrics/samplers/threads_sampler_spec.rb b/spec/lib/gitlab/metrics/samplers/threads_sampler_spec.rb new file mode 100644 index 00000000000..19477589289 --- /dev/null +++ b/spec/lib/gitlab/metrics/samplers/threads_sampler_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Gitlab::Metrics::Samplers::ThreadsSampler do + subject { described_class.new } + + describe '#interval' do + it 'samples every five seconds by default' do + expect(subject.interval).to eq(5) + end + + it 'samples at other intervals if requested' do + expect(described_class.new(11).interval).to eq(11) + end + end + + describe '#sample' do + before do + described_class::METRIC_DESCRIPTIONS.each_key do |metric| + allow(subject.metrics[metric]).to receive(:set) + end + end + + it 'sets the gauge for the concurrency total' do + expect(Gitlab::Runtime).to receive(:max_threads).and_return(9000) + expect(subject.metrics[:max_expected_threads]).to receive(:set).with({}, 9000) + + subject.sample + end + + context 'thread counts' do + it 'reports if any of the threads per group uses the db' do + threads = [ + fake_thread(described_class::SIDEKIQ_WORKER_THREAD_NAME, true), fake_thread(described_class::SIDEKIQ_WORKER_THREAD_NAME, false), + fake_thread(described_class::SIDEKIQ_WORKER_THREAD_NAME, nil) + ] + allow(Thread).to receive(:list).and_return(threads) + + expect(subject.metrics[:running_threads]).to receive(:set) + .with({ uses_db_connection: 'yes', thread_name: described_class::SIDEKIQ_WORKER_THREAD_NAME }, 1) + expect(subject.metrics[:running_threads]).to receive(:set) + .with({ uses_db_connection: 'no', thread_name: described_class::SIDEKIQ_WORKER_THREAD_NAME }, 2) + + subject.sample + end + + context 'thread names', :aggregate_failures do + where(:thread_names, :expected_names) do + [ + [[nil], %w(unnamed)], + [['puma threadpool 1', 'puma threadpool 001', 'puma threadpool 002'], ['puma threadpool']], + [%w(sidekiq_worker_thread), %w(sidekiq_worker_thread)], + [%w(some_sampler some_exporter), %w(some_sampler some_exporter)], + [%w(unknown thing), %w(unrecognized)] + ] + end + + with_them do + it do + allow(Thread).to receive(:list).and_return(thread_names.map { |name| fake_thread(name) }) + + expected_names.each do |expected_name| + expect(subject.metrics[:running_threads]).to receive(:set) + .with({ uses_db_connection: 'yes', thread_name: expected_name }, instance_of(Integer)) + expect(subject.metrics[:running_threads]).to receive(:set) + .with({ uses_db_connection: 'no', thread_name: expected_name }, instance_of(Integer)) + end + + subject.sample + end + end + end + end + + def fake_thread(name = nil, db_connection = nil) + thready = { uses_db_connection: db_connection } + allow(thready).to receive(:name).and_return(name) + + thready + end + end +end diff --git a/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb b/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb index c66d8b1075c..047d1e5d205 100644 --- a/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb +++ b/spec/lib/gitlab/metrics/sidekiq_middleware_spec.rb @@ -11,8 +11,8 @@ RSpec.describe Gitlab::Metrics::SidekiqMiddleware do worker = double(:worker, class: double(:class, name: 'TestWorker')) expect_next_instance_of(Gitlab::Metrics::BackgroundTransaction) do |transaction| - expect(transaction).to receive(:set).with(:sidekiq_queue_duration, instance_of(Float)) - expect(transaction).to receive(:increment).with(:db_count, 1) + expect(transaction).to receive(:set).with(:gitlab_transaction_sidekiq_queue_duration_total, instance_of(Float)) + expect(transaction).to receive(:increment).with(:gitlab_transaction_db_count_total, 1) end middleware.call(worker, message, :test) do @@ -42,7 +42,7 @@ RSpec.describe Gitlab::Metrics::SidekiqMiddleware do .and_call_original expect_any_instance_of(Gitlab::Metrics::Transaction).to receive(:set) - .with(:sidekiq_queue_duration, instance_of(Float)) + .with(:gitlab_transaction_sidekiq_queue_duration_total, instance_of(Float)) middleware.call(worker, {}, :test) { nil } end diff --git a/spec/lib/gitlab/metrics/subscribers/action_view_spec.rb b/spec/lib/gitlab/metrics/subscribers/action_view_spec.rb index 161527c01aa..adbc474343f 100644 --- a/spec/lib/gitlab/metrics/subscribers/action_view_spec.rb +++ b/spec/lib/gitlab/metrics/subscribers/action_view_spec.rb @@ -22,15 +22,15 @@ RSpec.describe Gitlab::Metrics::Subscribers::ActionView do describe '#render_template' do it 'tracks rendering of a template' do expect(transaction).to receive(:increment) - .with(:view_duration, 2.1) + .with(:gitlab_transaction_view_duration_total, 2.1) subscriber.render_template(event) end it 'observes view rendering time' do - expect(described_class.gitlab_view_rendering_duration_seconds) + expect(transaction) .to receive(:observe) - .with({ view: 'app/views/x.html.haml' }, 2.1) + .with(:gitlab_view_rendering_duration_seconds, 2.1, { view: "app/views/x.html.haml" }) subscriber.render_template(event) end diff --git a/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb b/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb index 2fd5dd1d83b..a31686b8061 100644 --- a/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb +++ b/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb @@ -37,10 +37,11 @@ RSpec.describe Gitlab::Metrics::Subscribers::ActiveRecord do it 'increments only db count value' do described_class::DB_COUNTERS.each do |counter| + prometheus_counter = "gitlab_transaction_#{counter}_total".to_sym if expected_counters[counter] > 0 - expect(transaction).to receive(:increment).with(counter, 1) + expect(transaction).to receive(:increment).with(prometheus_counter, 1) else - expect(transaction).not_to receive(:increment).with(counter, 1) + expect(transaction).not_to receive(:increment).with(prometheus_counter, 1) end end @@ -74,10 +75,18 @@ RSpec.describe Gitlab::Metrics::Subscribers::ActiveRecord do expect(subscriber).to receive(:current_transaction) .at_least(:once) .and_return(transaction) - expect(described_class.send(:gitlab_sql_duration_seconds)).to receive(:observe).with({}, 0.002) + expect(transaction).to receive(:observe).with(:gitlab_sql_duration_seconds, 0.002) + subscriber.sql(event) end + it 'marks the current thread as using the database' do + # since it would already have been toggled by other specs + Thread.current[:uses_db_connection] = nil + + expect { subscriber.sql(event) }.to change { Thread.current[:uses_db_connection] }.from(nil).to(true) + end + context 'with read query' do let(:expected_counters) do { @@ -217,7 +226,7 @@ RSpec.describe Gitlab::Metrics::Subscribers::ActiveRecord do end it 'skips schema/begin/commit sql commands' do - expect(subscriber).to receive(:current_transaction) + allow(subscriber).to receive(:current_transaction) .at_least(:once) .and_return(transaction) diff --git a/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb b/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb index f7ac719c16a..9aba6ac293c 100644 --- a/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb +++ b/spec/lib/gitlab/metrics/subscribers/rails_cache_spec.rb @@ -26,21 +26,12 @@ RSpec.describe Gitlab::Metrics::Subscribers::RailsCache do context 'with hit event' do let(:event) { double(:event, duration: 15.2, payload: { hit: true }) } - it 'increments the cache_read_hit count' do - expect(transaction).to receive(:increment) - .with(:cache_read_hit_count, 1, false) - expect(transaction).to receive(:increment) - .with(any_args).at_least(1) # Other calls - - subscriber.cache_read(event) - end - context 'when super operation is fetch' do let(:event) { double(:event, duration: 15.2, payload: { hit: true, super_operation: :fetch }) } - it 'does not increment cache read miss' do + it 'does not increment cache read miss total' do expect(transaction).not_to receive(:increment) - .with(:cache_read_hit_count, 1) + .with(:gitlab_cache_misses_total, 1) subscriber.cache_read(event) end @@ -50,33 +41,21 @@ RSpec.describe Gitlab::Metrics::Subscribers::RailsCache do context 'with miss event' do let(:event) { double(:event, duration: 15.2, payload: { hit: false }) } - it 'increments the cache_read_miss count' do + it 'increments the cache_read_miss total' do expect(transaction).to receive(:increment) - .with(:cache_read_miss_count, 1, false) + .with(:gitlab_cache_misses_total, 1) expect(transaction).to receive(:increment) .with(any_args).at_least(1) # Other calls subscriber.cache_read(event) end - it 'increments the cache_read_miss total' do - expect(subscriber.send(:metric_cache_misses_total)).to receive(:increment).with({}) - - subscriber.cache_read(event) - end - context 'when super operation is fetch' do let(:event) { double(:event, duration: 15.2, payload: { hit: false, super_operation: :fetch }) } - it 'does not increment cache read miss' do + it 'does not increment cache read miss total' do expect(transaction).not_to receive(:increment) - .with(:cache_read_miss_count, 1) - - subscriber.cache_read(event) - end - - it 'does not increment cache_read_miss total' do - expect(subscriber.send(:metric_cache_misses_total)).not_to receive(:increment).with({}) + .with(:gitlab_cache_misses_total, 1) subscriber.cache_read(event) end @@ -129,7 +108,7 @@ RSpec.describe Gitlab::Metrics::Subscribers::RailsCache do it 'increments the cache_read_hit count' do expect(transaction).to receive(:increment) - .with(:cache_read_hit_count, 1) + .with(:gitlab_transaction_cache_read_hit_count_total, 1) subscriber.cache_fetch_hit(event) end @@ -146,25 +125,17 @@ RSpec.describe Gitlab::Metrics::Subscribers::RailsCache do end context 'with a transaction' do - let(:metric_cache_misses_total) { double('metric_cache_misses_total', increment: nil) } - before do - allow(subscriber).to receive(:metric_cache_misses_total).and_return(metric_cache_misses_total) allow(subscriber).to receive(:current_transaction) .and_return(transaction) end - it 'increments the cache_fetch_miss count' do + it 'increments the cache_fetch_miss count and cache_read_miss total' do + expect(transaction).to receive(:increment).with(:gitlab_cache_misses_total, 1) expect(transaction).to receive(:increment) - .with(:cache_read_miss_count, 1) - - subscriber.cache_generate(event) - end + .with(:gitlab_transaction_cache_read_miss_count_total, 1) - it 'increments the cache_read_miss total' do subscriber.cache_generate(event) - - expect(metric_cache_misses_total).to have_received(:increment).with({}) end end end @@ -184,22 +155,6 @@ RSpec.describe Gitlab::Metrics::Subscribers::RailsCache do .and_return(transaction) end - it 'increments the total and specific cache duration' do - expect(transaction).to receive(:increment) - .with(:cache_duration, event.duration, false) - - expect(transaction).to receive(:increment) - .with(:cache_count, 1, false) - - expect(transaction).to receive(:increment) - .with(:cache_delete_duration, event.duration, false) - - expect(transaction).to receive(:increment) - .with(:cache_delete_count, 1, false) - - subscriber.observe(:delete, event.duration) - end - it 'observes cache metric' do expect(subscriber.send(:metric_cache_operation_duration_seconds)) .to receive(:observe) @@ -209,9 +164,9 @@ RSpec.describe Gitlab::Metrics::Subscribers::RailsCache do end it 'increments the operations total' do - expect(subscriber.send(:metric_cache_operations_total)) + expect(transaction) .to receive(:increment) - .with(transaction.labels.merge(operation: :delete)) + .with(:gitlab_cache_operations_total, 1, { operation: :delete }) subscriber.observe(:delete, event.duration) end diff --git a/spec/lib/gitlab/metrics/transaction_spec.rb b/spec/lib/gitlab/metrics/transaction_spec.rb index e64179bd5c1..88293f11149 100644 --- a/spec/lib/gitlab/metrics/transaction_spec.rb +++ b/spec/lib/gitlab/metrics/transaction_spec.rb @@ -28,14 +28,6 @@ RSpec.describe Gitlab::Metrics::Transaction do end end - describe '#allocated_memory' do - it 'returns the allocated memory in bytes' do - transaction.run { 'a' * 32 } - - expect(transaction.allocated_memory).to be_a_kind_of(Numeric) - end - end - describe '#run' do it 'yields the supplied block' do expect { |b| transaction.run(&b) }.to yield_control @@ -63,7 +55,7 @@ RSpec.describe Gitlab::Metrics::Transaction do end describe '#add_event' do - let(:prometheus_metric) { instance_double(Prometheus::Client::Counter, increment: nil) } + let(:prometheus_metric) { instance_double(Prometheus::Client::Counter, increment: nil, base_labels: {}) } it 'adds a metric' do expect(prometheus_metric).to receive(:increment) @@ -82,7 +74,7 @@ RSpec.describe Gitlab::Metrics::Transaction do context 'with sensitive tags' do before do transaction.add_event(:baubau, **sensitive_tags.merge(sane: 'yes')) - allow(described_class).to receive(:transaction_metric).and_return(prometheus_metric) + allow(described_class).to receive(:prometheus_metric).and_return(prometheus_metric) end it 'filters tags' do @@ -94,24 +86,119 @@ RSpec.describe Gitlab::Metrics::Transaction do end describe '#increment' do - let(:prometheus_metric) { instance_double(Prometheus::Client::Counter, increment: nil) } + let(:prometheus_metric) { instance_double(Prometheus::Client::Counter, increment: nil, base_labels: {}) } it 'adds a metric' do - expect(prometheus_metric).to receive(:increment).with(hash_including(:action, :controller), 1) - expect(described_class).to receive(:fetch_metric).with(:counter, :gitlab_transaction_meow_total).and_return(prometheus_metric) + expect(prometheus_metric).to receive(:increment) + expect(::Gitlab::Metrics).to receive(:counter).with(:meow, 'Meow counter', hash_including(:controller, :action)).and_return(prometheus_metric) transaction.increment(:meow, 1) end + + context 'with block' do + it 'overrides docstring' do + expect(::Gitlab::Metrics).to receive(:counter).with(:block_docstring, 'test', hash_including(:controller, :action)).and_return(prometheus_metric) + + transaction.increment(:block_docstring, 1) do + docstring 'test' + end + end + + it 'overrides labels' do + expect(::Gitlab::Metrics).to receive(:counter).with(:block_labels, 'Block labels counter', hash_including(:controller, :action, :sane)).and_return(prometheus_metric) + + labels = { sane: 'yes' } + transaction.increment(:block_labels, 1, labels) do + label_keys %i(sane) + end + end + + it 'filters sensitive tags' do + expect(::Gitlab::Metrics).to receive(:counter).with(:metric_with_sensitive_block, 'Metric with sensitive block counter', hash_excluding(sensitive_tags)).and_return(prometheus_metric) + + labels_keys = sensitive_tags.keys + transaction.increment(:metric_with_sensitive_block, 1, sensitive_tags) do + label_keys labels_keys + end + end + end end describe '#set' do - let(:prometheus_metric) { instance_double(Prometheus::Client::Gauge, set: nil) } + let(:prometheus_metric) { instance_double(Prometheus::Client::Gauge, set: nil, base_labels: {}) } it 'adds a metric' do - expect(prometheus_metric).to receive(:set).with(hash_including(:action, :controller), 1) - expect(described_class).to receive(:fetch_metric).with(:gauge, :gitlab_transaction_meow_total).and_return(prometheus_metric) + expect(prometheus_metric).to receive(:set) + expect(::Gitlab::Metrics).to receive(:gauge).with(:meow_set, 'Meow set gauge', hash_including(:controller, :action), :all).and_return(prometheus_metric) + + transaction.set(:meow_set, 1) + end + + context 'with block' do + it 'overrides docstring' do + expect(::Gitlab::Metrics).to receive(:gauge).with(:block_docstring_set, 'test', hash_including(:controller, :action), :all).and_return(prometheus_metric) + + transaction.set(:block_docstring_set, 1) do + docstring 'test' + end + end + + it 'overrides labels' do + expect(::Gitlab::Metrics).to receive(:gauge).with(:block_labels_set, 'Block labels set gauge', hash_including(:controller, :action, :sane), :all).and_return(prometheus_metric) - transaction.set(:meow, 1) + labels = { sane: 'yes' } + transaction.set(:block_labels_set, 1, labels) do + label_keys %i(sane) + end + end + + it 'filters sensitive tags' do + expect(::Gitlab::Metrics).to receive(:gauge).with(:metric_set_with_sensitive_block, 'Metric set with sensitive block gauge', hash_excluding(sensitive_tags), :all).and_return(prometheus_metric) + + label_keys = sensitive_tags.keys + transaction.set(:metric_set_with_sensitive_block, 1, sensitive_tags) do + label_keys label_keys + end + end + end + end + + describe '#observe' do + let(:prometheus_metric) { instance_double(Prometheus::Client::Histogram, observe: nil, base_labels: {}) } + + it 'adds a metric' do + expect(prometheus_metric).to receive(:observe) + expect(::Gitlab::Metrics).to receive(:histogram).with(:meow_observe, 'Meow observe histogram', hash_including(:controller, :action), kind_of(Array)).and_return(prometheus_metric) + + transaction.observe(:meow_observe, 1) + end + + context 'with block' do + it 'overrides docstring' do + expect(::Gitlab::Metrics).to receive(:histogram).with(:block_docstring_observe, 'test', hash_including(:controller, :action), kind_of(Array)).and_return(prometheus_metric) + + transaction.observe(:block_docstring_observe, 1) do + docstring 'test' + end + end + + it 'overrides labels' do + expect(::Gitlab::Metrics).to receive(:histogram).with(:block_labels_observe, 'Block labels observe histogram', hash_including(:controller, :action, :sane), kind_of(Array)).and_return(prometheus_metric) + + labels = { sane: 'yes' } + transaction.observe(:block_labels_observe, 1, labels) do + label_keys %i(sane) + end + end + + it 'filters sensitive tags' do + expect(::Gitlab::Metrics).to receive(:histogram).with(:metric_observe_with_sensitive_block, 'Metric observe with sensitive block histogram', hash_excluding(sensitive_tags), kind_of(Array)).and_return(prometheus_metric) + + label_keys = sensitive_tags.keys + transaction.observe(:metric_observe_with_sensitive_block, 1, sensitive_tags) do + label_keys label_keys + end + end end end end diff --git a/spec/lib/gitlab/metrics/web_transaction_spec.rb b/spec/lib/gitlab/metrics/web_transaction_spec.rb index 12e98089066..6903ce53f65 100644 --- a/spec/lib/gitlab/metrics/web_transaction_spec.rb +++ b/spec/lib/gitlab/metrics/web_transaction_spec.rb @@ -5,29 +5,52 @@ require 'spec_helper' RSpec.describe Gitlab::Metrics::WebTransaction do let(:env) { {} } let(:transaction) { described_class.new(env) } - let(:prometheus_metric) { double("prometheus metric") } + let(:prometheus_metric) { instance_double(Prometheus::Client::Metric, base_labels: {}) } before do - allow(described_class).to receive(:transaction_metric).and_return(prometheus_metric) + allow(described_class).to receive(:prometheus_metric).and_return(prometheus_metric) end - describe '#duration' do - it 'returns the duration of a transaction in seconds' do - transaction.run { sleep(0.5) } + RSpec.shared_context 'ActionController request' do + let(:request) { double(:request, format: double(:format, ref: :html)) } + let(:controller_class) { double(:controller_class, name: 'TestController') } - expect(transaction.duration).to be >= 0.5 + before do + controller = double(:controller, class: controller_class, action_name: 'show', request: request) + env['action_controller.instance'] = controller + end + end + + RSpec.shared_context 'transaction observe metrics' do + before do + allow(transaction).to receive(:observe) + end + end + + RSpec.shared_examples 'metric with labels' do |metric_method| + include_context 'ActionController request' + + it 'measures with correct labels and value' do + value = 1 + expect(prometheus_metric).to receive(metric_method).with({ controller: 'TestController', action: 'show', feature_category: '' }, value) + + transaction.send(metric_method, :bau, value) end end - describe '#allocated_memory' do - it 'returns the allocated memory in bytes' do - transaction.run { 'a' * 32 } + describe '#duration' do + include_context 'transaction observe metrics' + + it 'returns the duration of a transaction in seconds' do + transaction.run { sleep(0.5) } - expect(transaction.allocated_memory).to be_a_kind_of(Numeric) + expect(transaction.duration).to be >= 0.5 end end describe '#run' do + include_context 'transaction observe metrics' + it 'yields the supplied block' do expect { |b| transaction.run(&b) }.to yield_control end @@ -53,26 +76,7 @@ RSpec.describe Gitlab::Metrics::WebTransaction do end end - describe '#increment' do - it 'increments a counter' do - expect(prometheus_metric).to receive(:increment).with({}, 1) - - transaction.increment(:time, 1) - end - end - - describe '#set' do - it 'sets a value' do - expect(prometheus_metric).to receive(:set).with({}, 10) - - transaction.set(:number, 10) - end - end - describe '#labels' do - let(:request) { double(:request, format: double(:format, ref: :html)) } - let(:controller_class) { double(:controller_class, name: 'TestController') } - context 'when request goes to Grape endpoint' do before do route = double(:route, request_method: 'GET', path: '/:version/projects/:id/archive(.:format)') @@ -86,7 +90,7 @@ RSpec.describe Gitlab::Metrics::WebTransaction do end it 'contains only the labels defined for transactions' do - expect(transaction.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABELS.keys) + expect(transaction.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABEL_KEYS) end it 'does not provide labels if route infos are missing' do @@ -100,18 +104,14 @@ RSpec.describe Gitlab::Metrics::WebTransaction do end context 'when request goes to ActionController' do - before do - controller = double(:controller, class: controller_class, action_name: 'show', request: request) - - env['action_controller.instance'] = controller - end + include_context 'ActionController request' it 'tags a transaction with the name and action of a controller' do expect(transaction.labels).to eq({ controller: 'TestController', action: 'show', feature_category: '' }) end it 'contains only the labels defined for transactions' do - expect(transaction.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABELS.keys) + expect(transaction.labels.keys).to contain_exactly(*described_class.superclass::BASE_LABEL_KEYS) end context 'when the request content type is not :html' do @@ -144,6 +144,8 @@ RSpec.describe Gitlab::Metrics::WebTransaction do end describe '#add_event' do + let(:prometheus_metric) { instance_double(Prometheus::Client::Counter, :increment, base_labels: {}) } + it 'adds a metric' do expect(prometheus_metric).to receive(:increment) @@ -156,4 +158,22 @@ RSpec.describe Gitlab::Metrics::WebTransaction do transaction.add_event(:bau, animal: 'dog') end end + + describe '#increment' do + let(:prometheus_metric) { instance_double(Prometheus::Client::Counter, :increment, base_labels: {}) } + + it_behaves_like 'metric with labels', :increment + end + + describe '#set' do + let(:prometheus_metric) { instance_double(Prometheus::Client::Gauge, :set, base_labels: {}) } + + it_behaves_like 'metric with labels', :set + end + + describe '#observe' do + let(:prometheus_metric) { instance_double(Prometheus::Client::Histogram, :observe, base_labels: {}) } + + it_behaves_like 'metric with labels', :observe + end end |