Commit d73e68de authored by Yorick Peterse's avatar Yorick Peterse

Limit the action suffixes in transaction metrics

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".
parent 1a0ee662
---
title: Limit the action suffixes in transaction metrics
merge_request:
author:
type: fixed
...@@ -3,6 +3,7 @@ module Gitlab ...@@ -3,6 +3,7 @@ module Gitlab
class WebTransaction < Transaction class WebTransaction < Transaction
CONTROLLER_KEY = 'action_controller.instance'.freeze CONTROLLER_KEY = 'action_controller.instance'.freeze
ENDPOINT_KEY = 'api.endpoint'.freeze ENDPOINT_KEY = 'api.endpoint'.freeze
ALLOWED_SUFFIXES = Set.new(%w[json js atom rss xml zip])
def initialize(env) def initialize(env)
super() super()
...@@ -32,9 +33,13 @@ module Gitlab ...@@ -32,9 +33,13 @@ module Gitlab
# Devise exposes a method called "request_format" that does the below. # Devise exposes a method called "request_format" that does the below.
# However, this method is not available to all controllers (e.g. certain # However, this method is not available to all controllers (e.g. certain
# Doorkeeper controllers). As such we use the underlying code directly. # 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}" action += ".#{suffix}"
end end
......
...@@ -194,7 +194,7 @@ describe Gitlab::Metrics::WebTransaction do ...@@ -194,7 +194,7 @@ describe Gitlab::Metrics::WebTransaction do
expect(transaction.action).to eq('TestController#show') expect(transaction.action).to eq('TestController#show')
end 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)) } let(:request) { double(:request, format: double(:format, ref: :json)) }
it 'appends the mime type to the transaction action' do it 'appends the mime type to the transaction action' do
...@@ -202,6 +202,15 @@ describe Gitlab::Metrics::WebTransaction do ...@@ -202,6 +202,15 @@ describe Gitlab::Metrics::WebTransaction do
expect(transaction.action).to eq('TestController#show.json') expect(transaction.action).to eq('TestController#show.json')
end end
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 end
it 'returns no labels when no route information is present in env' do it 'returns no labels when no route information is present in env' do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment