Commit 196adbe4 authored by Paul Slaughter's avatar Paul Slaughter Committed by Sean McGivern

Read feature_category from request for GraphQL controller

- We also update `web_transaction` to read from `ApplicationContext`.
- We tried using `around_action` and `ApplicationContext.with`
  in the GraphqlController, but the `web_transaction` methods
  were called **after** the `ApplicationController` set the
  context, but **before** the `GraphqlController` could kick
  in.
parent 2697d4ae
# frozen_string_literal: true # frozen_string_literal: true
class GraphqlController < ApplicationController class GraphqlController < ApplicationController
extend ::Gitlab::Utils::Override
# Unauthenticated users have access to the API for public data # Unauthenticated users have access to the API for public data
skip_before_action :authenticate_user! skip_before_action :authenticate_user!
...@@ -35,6 +37,7 @@ class GraphqlController < ApplicationController ...@@ -35,6 +37,7 @@ class GraphqlController < ApplicationController
# callback execution order here # callback execution order here
around_action :sessionless_bypass_admin_mode!, if: :sessionless_user? around_action :sessionless_bypass_admin_mode!, if: :sessionless_user?
# The default feature category is overridden to read from request
feature_category :not_owned feature_category :not_owned
def execute def execute
...@@ -64,6 +67,11 @@ class GraphqlController < ApplicationController ...@@ -64,6 +67,11 @@ class GraphqlController < ApplicationController
render_error(exception.message, status: :unprocessable_entity) render_error(exception.message, status: :unprocessable_entity)
end end
override :feature_category
def feature_category
::Gitlab::FeatureCategories.default.from_request(request) || super
end
private private
def disallow_mutations_for_get def disallow_mutations_for_get
......
# frozen_string_literal: true
module Gitlab
class FeatureCategories
FEATURE_CATEGORY_DEFAULT = 'unknown'
attr_reader :categories
def self.default
@default ||= self.load_from_yaml
end
def self.load_from_yaml
categories = YAML.load_file(Rails.root.join('config', 'feature_categories.yml'))
new(categories)
end
def initialize(categories)
@categories = categories.to_set
end
# If valid, returns a feature category from the given request.
def from_request(request)
category = request.headers["HTTP_X_GITLAB_FEATURE_CATEGORY"].presence
return unless category && valid?(category)
return unless ::Gitlab::RequestForgeryProtection.verified?(request.env)
category
end
def valid?(category)
categories.include?(category.to_s)
end
end
end
...@@ -15,7 +15,7 @@ module Gitlab ...@@ -15,7 +15,7 @@ module Gitlab
HEALTH_ENDPOINT = %r{^/-/(liveness|readiness|health|metrics)/?$}.freeze HEALTH_ENDPOINT = %r{^/-/(liveness|readiness|health|metrics)/?$}.freeze
FEATURE_CATEGORY_DEFAULT = 'unknown' FEATURE_CATEGORY_DEFAULT = ::Gitlab::FeatureCategories::FEATURE_CATEGORY_DEFAULT
ENDPOINT_MISSING = 'unknown' ENDPOINT_MISSING = 'unknown'
# These were the top 5 categories at a point in time, chosen as a # These were the top 5 categories at a point in time, chosen as a
......
...@@ -57,10 +57,6 @@ module Gitlab ...@@ -57,10 +57,6 @@ module Gitlab
action = "#{controller.action_name}" action = "#{controller.action_name}"
# Try to get the feature category, but don't fail when the controller is
# not an ApplicationController.
feature_category = controller.class.try(:feature_category_for_action, action).to_s
# 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.
...@@ -91,9 +87,6 @@ module Gitlab ...@@ -91,9 +87,6 @@ module Gitlab
if route if route
path = endpoint_paths_cache[route.request_method][route.path] path = endpoint_paths_cache[route.request_method][route.path]
grape_class = endpoint.options[:for]
feature_category = grape_class.try(:feature_category_for_app, endpoint).to_s
{ controller: 'Grape', action: "#{route.request_method} #{path}", feature_category: feature_category } { controller: 'Grape', action: "#{route.request_method} #{path}", feature_category: feature_category }
end end
end end
...@@ -109,6 +102,10 @@ module Gitlab ...@@ -109,6 +102,10 @@ module Gitlab
def endpoint_instrumentable_path(raw_path) def endpoint_instrumentable_path(raw_path)
raw_path.sub('(.:format)', '').sub('/:version', '') raw_path.sub('(.:format)', '').sub('/:version', '')
end end
def feature_category
::Gitlab::ApplicationContext.current_context_attribute(:feature_category) || ::Gitlab::FeatureCategories::FEATURE_CATEGORY_DEFAULT
end
end end
end end
end end
...@@ -38,6 +38,14 @@ RSpec.describe GraphqlController do ...@@ -38,6 +38,14 @@ RSpec.describe GraphqlController do
sign_in(user) sign_in(user)
end end
it 'sets feature category in ApplicationContext from request' do
request.headers["HTTP_X_GITLAB_FEATURE_CATEGORY"] = "web_ide"
post :execute
expect(::Gitlab::ApplicationContext.current_context_attribute(:feature_category)).to eq('web_ide')
end
it 'returns 200 when user can access API' do it 'returns 200 when user can access API' do
post :execute post :execute
......
# frozen_string_literal: true
require 'fast_spec_helper'
RSpec.describe Gitlab::FeatureCategories do
let(:fake_categories) { %w(foo bar) }
subject { described_class.new(fake_categories) }
describe "#valid?" do
it "returns true if category is known", :aggregate_failures do
expect(subject.valid?('foo')).to be(true)
expect(subject.valid?('zzz')).to be(false)
end
end
describe "#from_request" do
let(:request_env) { {} }
let(:verified) { true }
def fake_request(request_feature_category)
double('request', env: request_env, headers: { "HTTP_X_GITLAB_FEATURE_CATEGORY" => request_feature_category })
end
before do
allow(::Gitlab::RequestForgeryProtection).to receive(:verified?).with(request_env).and_return(verified)
end
it "returns category from request when valid, otherwise returns nil", :aggregate_failures do
expect(subject.from_request(fake_request("foo"))).to be("foo")
expect(subject.from_request(fake_request("zzz"))).to be_nil
end
context "when request is not verified" do
let(:verified) { false }
it "returns nil" do
expect(subject.from_request(fake_request("foo"))).to be_nil
end
end
end
describe "#categories" do
it "returns a set of the given categories" do
expect(subject.categories).to be_a(Set)
expect(subject.categories).to contain_exactly(*fake_categories)
end
end
describe ".load_from_yaml" do
subject { described_class.load_from_yaml }
it "creates FeatureCategories from feature_categories.yml file" do
contents = YAML.load_file(Rails.root.join('config', 'feature_categories.yml'))
expect(subject.categories).to contain_exactly(*contents)
end
end
describe ".default" do
it "returns a memoization of load_from_yaml", :aggregate_failures do
# FeatureCategories.default could have been referenced in another spec, so we need to clean it up here
described_class.instance_variable_set(:@default, nil)
expect(described_class).to receive(:load_from_yaml).once.and_call_original
2.times { described_class.default }
# Uses reference equality to verify memoization
expect(described_class.default).to equal(described_class.default)
expect(described_class.default).to be_a(described_class)
end
end
end
...@@ -348,7 +348,7 @@ RSpec.describe Gitlab::Metrics::RequestsRackMiddleware, :aggregate_failures do ...@@ -348,7 +348,7 @@ RSpec.describe Gitlab::Metrics::RequestsRackMiddleware, :aggregate_failures do
end end
it 'has every label in config/feature_categories.yml' do it 'has every label in config/feature_categories.yml' do
defaults = [described_class::FEATURE_CATEGORY_DEFAULT, 'not_owned'] defaults = [::Gitlab::FeatureCategories::FEATURE_CATEGORY_DEFAULT, 'not_owned']
feature_categories = YAML.load_file(Rails.root.join('config', 'feature_categories.yml')).map(&:strip) + defaults feature_categories = YAML.load_file(Rails.root.join('config', 'feature_categories.yml')).map(&:strip) + defaults
expect(described_class::FEATURE_CATEGORIES_TO_INITIALIZE).to all(be_in(feature_categories)) expect(described_class::FEATURE_CATEGORIES_TO_INITIALIZE).to all(be_in(feature_categories))
......
...@@ -32,7 +32,7 @@ RSpec.describe Gitlab::Metrics::WebTransaction do ...@@ -32,7 +32,7 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
it 'measures with correct labels and value' do it 'measures with correct labels and value' do
value = 1 value = 1
expect(prometheus_metric).to receive(metric_method).with({ controller: 'TestController', action: 'show', feature_category: '' }, value) expect(prometheus_metric).to receive(metric_method).with({ controller: 'TestController', action: 'show', feature_category: ::Gitlab::FeatureCategories::FEATURE_CATEGORY_DEFAULT }, value)
transaction.send(metric_method, :bau, value) transaction.send(metric_method, :bau, value)
end end
...@@ -105,6 +105,9 @@ RSpec.describe Gitlab::Metrics::WebTransaction do ...@@ -105,6 +105,9 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
namespace: "/projects") namespace: "/projects")
env['api.endpoint'] = endpoint env['api.endpoint'] = endpoint
# This is needed since we're not actually making a request, which would trigger the controller pushing to the context
::Gitlab::ApplicationContext.push(feature_category: 'projects')
end end
it 'provides labels with the method and path of the route in the grape endpoint' do it 'provides labels with the method and path of the route in the grape endpoint' do
...@@ -129,7 +132,7 @@ RSpec.describe Gitlab::Metrics::WebTransaction do ...@@ -129,7 +132,7 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
include_context 'ActionController request' include_context 'ActionController request'
it 'tags a transaction with the name and action of a controller' do it 'tags a transaction with the name and action of a controller' do
expect(transaction.labels).to eq({ controller: 'TestController', action: 'show', feature_category: '' }) expect(transaction.labels).to eq({ controller: 'TestController', action: 'show', feature_category: ::Gitlab::FeatureCategories::FEATURE_CATEGORY_DEFAULT })
end end
it 'contains only the labels defined for transactions' do it 'contains only the labels defined for transactions' do
...@@ -140,7 +143,7 @@ RSpec.describe Gitlab::Metrics::WebTransaction do ...@@ -140,7 +143,7 @@ RSpec.describe Gitlab::Metrics::WebTransaction 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
expect(transaction.labels).to eq({ controller: 'TestController', action: 'show.json', feature_category: '' }) expect(transaction.labels).to eq({ controller: 'TestController', action: 'show.json', feature_category: ::Gitlab::FeatureCategories::FEATURE_CATEGORY_DEFAULT })
end end
end end
...@@ -148,13 +151,15 @@ RSpec.describe Gitlab::Metrics::WebTransaction do ...@@ -148,13 +151,15 @@ RSpec.describe Gitlab::Metrics::WebTransaction do
let(:request) { double(:request, format: double(:format, ref: 'http://example.com')) } let(:request) { double(:request, format: double(:format, ref: 'http://example.com')) }
it 'does not append the MIME type to the transaction action' do it 'does not append the MIME type to the transaction action' do
expect(transaction.labels).to eq({ controller: 'TestController', action: 'show', feature_category: '' }) expect(transaction.labels).to eq({ controller: 'TestController', action: 'show', feature_category: ::Gitlab::FeatureCategories::FEATURE_CATEGORY_DEFAULT })
end end
end end
context 'when the feature category is known' do context 'when the feature category is known' do
it 'includes it in the feature category label' do it 'includes it in the feature category label' do
expect(controller_class).to receive(:feature_category_for_action).with('show').and_return(:source_code_management) # This is needed since we're not actually making a request, which would trigger the controller pushing to the context
::Gitlab::ApplicationContext.push(feature_category: 'source_code_management')
expect(transaction.labels).to eq({ controller: 'TestController', action: 'show', feature_category: "source_code_management" }) expect(transaction.labels).to eq({ controller: 'TestController', action: 'show', feature_category: "source_code_management" })
end end
end end
......
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