Automatic merge of gitlab-org/gitlab-ce master

.gitlab/issue_templates/Feature proposal.md

@@ -24,6 +24,10 @@ Personas can be found at https://about.gitlab.com/handbook/marketing/product-mar
 <!-- See the Feature Change Documentation Workflow https://docs.gitlab.com/ee/development/documentation/feature-change-workflow.html
 Add all known Documentation Requirements here, per https://docs.gitlab.com/ee/development/documentation/feature-change-workflow.html#documentation-requirements -->
 
+### Testing
+
+<!-- What risks does this change pose? How might it affect the quality of the product? What additional test coverage or changes to tests will be needed? Will it require cross-browser testing? See the test engineering process for further guidelines: https://about.gitlab.com/handbook/engineering/quality/guidelines/test-engineering/ -->
+
 ### What does success look like, and how can we measure that?
 
 <!-- Define both the success metrics and acceptance criteria. Note that success metrics indicate the desired business outcomes, while acceptance criteria indicate when the solution is working correctly. If there is no way to measure success, link to an issue that will implement a way to measure this. -->

changelogs/unreleased/48090-filter-sensitive-metric-labels.yml

+---
+title: Remove `path` and `branch` labels from metrics
+merge_request: 26744
+author:
+type: fixed

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

spec/lib/gitlab/metrics/transaction_spec.rb

+# 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