Limit the action suffixes in transaction metricslimit-metrics-content-type

There seem to be a lot of cases where the suffix of an action (e.g.
".html") is set to bogus data, such as "*/*" or entire URLs. This can
increase cardinality of our metrics, and isn't very useful for
monitoring and filtering. To work around this, we enforce a whitelist
containing a few suffixes we actually care about. Suffixes not supported
will be grouped under the action without a suffix. This means that a
request to "FooController#bar.jpeg" will be assigned to
"FooController#bar".

3 files changed, 22 insertions, 3 deletions

diff --git a/changelogs/unreleased/limit-metrics-content-type.yml b/changelogs/unreleased/limit-metrics-content-type.yml
new file mode 100644
index 00000000000..42cb4347771
--- /dev/null
+++ b/changelogs/unreleased/limit-metrics-content-type.yml
@@ -0,0 +1,5 @@
+---
+title: Limit the action suffixes in transaction metrics
+merge_request:
+author:
+type: fixed
diff --git a/lib/gitlab/metrics/web_transaction.rb b/lib/gitlab/metrics/web_transaction.rb
index 3799aaebf1c..723ca576aab 100644
--- a/lib/gitlab/metrics/web_transaction.rb
+++ b/lib/gitlab/metrics/web_transaction.rb
@@ -3,6 +3,7 @@ module Gitlab
     class WebTransaction < Transaction
       CONTROLLER_KEY = 'action_controller.instance'.freeze
       ENDPOINT_KEY = 'api.endpoint'.freeze
+      ALLOWED_SUFFIXES = Set.new(%w[json js atom rss xml zip])
 
       def initialize(env)
         super()
@@ -32,9 +33,13 @@ module Gitlab
         # Devise exposes a method called "request_format" that does the below.
         # However, this method is not available to all controllers (e.g. certain
         # Doorkeeper controllers). As such we use the underlying code directly.
-        suffix = controller.request.format.try(:ref)
+        suffix = controller.request.format.try(:ref).to_s
 
-        if suffix && suffix != :html
+        # Sometimes the request format is set to silly data such as
+        # "application/xrds+xml" or actual URLs. To prevent such values from
+        # increasing the cardinality of our metrics, we limit the number of
+        # possible suffixes.
+        if suffix && ALLOWED_SUFFIXES.include?(suffix)
           action += ".#{suffix}"
         end
 
diff --git a/spec/lib/gitlab/metrics/web_transaction_spec.rb b/spec/lib/gitlab/metrics/web_transaction_spec.rb
index 6eb0600f49e..0b3b23e930f 100644
--- a/spec/lib/gitlab/metrics/web_transaction_spec.rb
+++ b/spec/lib/gitlab/metrics/web_transaction_spec.rb
@@ -194,7 +194,7 @@ describe Gitlab::Metrics::WebTransaction do
         expect(transaction.action).to eq('TestController#show')
       end
 
-      context 'when the response content type is not :html' do
+      context 'when the request content type is not :html' do
         let(:request) { double(:request, format: double(:format, ref: :json)) }
 
         it 'appends the mime type to the transaction action' do
@@ -202,6 +202,15 @@ describe Gitlab::Metrics::WebTransaction do
           expect(transaction.action).to eq('TestController#show.json')
         end
       end
+
+      context 'when the request content type is not' do
+        let(:request) { double(:request, format: double(:format, ref: 'http://example.com')) }
+
+        it 'does not append the MIME type to the transaction action' do
+          expect(transaction.labels).to eq({ controller: 'TestController', action: 'show' })
+          expect(transaction.action).to eq('TestController#show')
+        end
+      end
     end
 
     it 'returns no labels when no route information is present in env' do