diff options
| -rw-r--r-- | changelogs/unreleased/48090-filter-sensitive-metric-labels.yml | 5 | ||||
| -rw-r--r-- | lib/gitlab/metrics/transaction.rb | 15 | ||||
| -rw-r--r-- | spec/lib/gitlab/metrics/transaction_spec.rb | 229 |
3 files changed, 246 insertions, 3 deletions
diff --git a/changelogs/unreleased/48090-filter-sensitive-metric-labels.yml b/changelogs/unreleased/48090-filter-sensitive-metric-labels.yml new file mode 100644 index 00000000000..e588fa79619 --- /dev/null +++ b/changelogs/unreleased/48090-filter-sensitive-metric-labels.yml @@ -0,0 +1,5 @@ +--- +title: Remove `path` and `branch` labels from metrics +merge_request: 26744 +author: +type: fixed diff --git a/lib/gitlab/metrics/transaction.rb b/lib/gitlab/metrics/transaction.rb index e91803ecd62..d7986685c65 100644 --- a/lib/gitlab/metrics/transaction.rb +++ b/lib/gitlab/metrics/transaction.rb @@ -8,6 +8,8 @@ module Gitlab # base labels shared among all transactions BASE_LABELS = { controller: nil, action: nil }.freeze + # labels that potentially contain sensitive information and will be filtered + FILTERED_LABELS = [:branch, :path].freeze THREAD_KEY = :_gitlab_metrics_transaction @@ -64,7 +66,7 @@ module Gitlab end def add_metric(series, values, tags = {}) - @metrics << Metric.new("#{::Gitlab::Metrics.series_prefix}#{series}", values, tags) + @metrics << Metric.new("#{::Gitlab::Metrics.series_prefix}#{series}", values, filter_tags(tags)) end # Tracks a business level event @@ -75,8 +77,9 @@ module Gitlab # event_name - The name of the event (e.g. "git_push"). # tags - A set of tags to attach to the event. def add_event(event_name, tags = {}) - self.class.transaction_metric(event_name, :counter, prefix: 'event_', use_feature_flag: true, tags: tags).increment(tags.merge(labels)) - @metrics << Metric.new(EVENT_SERIES, { count: 1 }, tags.merge(event: event_name), :event) + filtered_tags = filter_tags(tags) + self.class.transaction_metric(event_name, :counter, prefix: 'event_', use_feature_flag: true, tags: filtered_tags).increment(filtered_tags.merge(labels)) + @metrics << Metric.new(EVENT_SERIES, { count: 1 }, filtered_tags.merge(event: event_name), :event) end # Returns a MethodCall object for the given name. @@ -164,6 +167,12 @@ module Gitlab end end end + + private + + def filter_tags(tags) + tags.without(*FILTERED_LABELS) + end end end end diff --git a/spec/lib/gitlab/metrics/transaction_spec.rb b/spec/lib/gitlab/metrics/transaction_spec.rb new file mode 100644 index 00000000000..e70fde09edd --- /dev/null +++ b/spec/lib/gitlab/metrics/transaction_spec.rb @@ -0,0 +1,229 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Metrics::Transaction do + let(:transaction) { described_class.new } + let(:metric) { transaction.metrics[0] } + + let(:sensitive_tags) do + { + path: 'private', + branch: 'sensitive' + } + end + + shared_examples 'tag filter' do |sane_tags| + it 'filters potentially sensitive tags' do + expect(metric.tags).to eq(sane_tags) + end + end + + describe '#duration' do + it 'returns the duration of a transaction in seconds' do + transaction.run { } + + expect(transaction.duration).to be > 0 + 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 + end + + it 'stores the transaction in the current thread' do + transaction.run do + expect(described_class.current).to eq(transaction) + end + end + + it 'removes the transaction from the current thread upon completion' do + transaction.run { } + + expect(described_class.current).to be_nil + end + end + + describe '#add_metric' do + it 'adds a metric to the transaction' do + transaction.add_metric('foo', value: 1) + + expect(metric.series).to eq('rails_foo') + expect(metric.tags).to eq({}) + expect(metric.values).to eq(value: 1) + end + + context 'with sensitive tags' do + before do + transaction + .add_metric('foo', { value: 1 }, **sensitive_tags.merge(sane: 'yes')) + end + + it_behaves_like 'tag filter', sane: 'yes' + end + end + + describe '#method_call_for' do + it 'returns a MethodCall' do + method = transaction.method_call_for('Foo#bar', :Foo, '#bar') + + expect(method).to be_an_instance_of(Gitlab::Metrics::MethodCall) + end + end + + describe '#increment' do + it 'increments a counter' do + transaction.increment(:time, 1) + transaction.increment(:time, 2) + + values = metric_values(time: 3) + + expect(transaction).to receive(:add_metric) + .with('transactions', values, {}) + + transaction.track_self + end + end + + describe '#set' do + it 'sets a value' do + transaction.set(:number, 10) + + values = metric_values(number: 10) + + expect(transaction).to receive(:add_metric) + .with('transactions', values, {}) + + transaction.track_self + end + end + + describe '#finish' do + it 'tracks the transaction details and submits them to Sidekiq' do + expect(transaction).to receive(:track_self) + expect(transaction).to receive(:submit) + + transaction.finish + end + end + + describe '#track_self' do + it 'adds a metric for the transaction itself' do + values = metric_values + + expect(transaction).to receive(:add_metric) + .with('transactions', values, {}) + + transaction.track_self + end + end + + describe '#submit' do + it 'submits the metrics to Sidekiq' do + transaction.track_self + + expect(Gitlab::Metrics).to receive(:submit_metrics) + .with([an_instance_of(Hash)]) + + transaction.submit + end + + it 'adds the action as a tag for every metric' do + allow(transaction) + .to receive(:labels) + .and_return(controller: 'Foo', action: 'bar') + + transaction.track_self + + hash = { + series: 'rails_transactions', + tags: { action: 'Foo#bar' }, + values: metric_values, + timestamp: a_kind_of(Integer) + } + + expect(Gitlab::Metrics).to receive(:submit_metrics) + .with([hash]) + + transaction.submit + end + + it 'does not add an action tag for events' do + allow(transaction) + .to receive(:labels) + .and_return(controller: 'Foo', action: 'bar') + + transaction.add_event(:meow) + + hash = { + series: 'events', + tags: { event: :meow }, + values: { count: 1 }, + timestamp: a_kind_of(Integer) + } + + expect(Gitlab::Metrics).to receive(:submit_metrics) + .with([hash]) + + transaction.submit + end + end + + describe '#add_event' do + it 'adds a metric' do + transaction.add_event(:meow) + + expect(metric).to be_an_instance_of(Gitlab::Metrics::Metric) + end + + it "does not prefix the metric's series name" do + transaction.add_event(:meow) + + expect(metric.series).to eq(described_class::EVENT_SERIES) + end + + it 'tracks a counter for every event' do + transaction.add_event(:meow) + + expect(metric.values).to eq(count: 1) + end + + it 'tracks the event name' do + transaction.add_event(:meow) + + expect(metric.tags).to eq(event: :meow) + end + + it 'allows tracking of custom tags' do + transaction.add_event(:meow, animal: 'cat') + + expect(metric.tags).to eq(event: :meow, animal: 'cat') + end + + context 'with sensitive tags' do + before do + transaction.add_event(:meow, **sensitive_tags.merge(sane: 'yes')) + end + + it_behaves_like 'tag filter', event: :meow, sane: 'yes' + end + end + + private + + def metric_values(opts = {}) + { + duration: 0.0, + allocated_memory: a_kind_of(Numeric) + }.merge(opts) + end +end |