From b36362905f52a1f3159a7ce6f6401ae624b22c5b Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 11 Dec 2020 15:14:01 +0000 Subject: [PATCH 1/2] Add context metadata to response headers This will allow Workhorse to pull out the headers and include them in its own logs. --- app/controllers/application_controller.rb | 9 ++- lib/api/api.rb | 8 +- lib/api/helpers.rb | 7 ++ lib/api/internal/base.rb | 8 +- .../metrics/requests_rack_middleware.rb | 2 +- .../application_controller_spec.rb | 30 +++++-- spec/requests/api/api_spec.rb | 79 ++++++++++++++----- 7 files changed, 110 insertions(+), 33 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 905a4a7212e5f4..90302671fd030f 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -151,7 +151,7 @@ def append_info_to_payload(payload) payload[:remote_ip] = request.remote_ip payload[Labkit::Correlation::CorrelationId::LOG_KEY] = Labkit::Correlation::CorrelationId.current_id - payload[:metadata] = @current_context + payload[:metadata] = @current_context.to_h logged_user = auth_user if logged_user.present? @@ -276,7 +276,6 @@ def default_headers headers['X-XSS-Protection'] = '1; mode=block' headers['X-UA-Compatible'] = 'IE=edge' headers['X-Content-Type-Options'] = 'nosniff' - headers[Gitlab::Metrics::RequestsRackMiddleware::FEATURE_CATEGORY_HEADER] = feature_category end def default_cache_headers @@ -472,7 +471,11 @@ def set_current_context(&block) feature_category: feature_category) do yield ensure - @current_context = Labkit::Context.current.to_h + @current_context = Labkit::Context.current + + @current_context.to_headers.each do |key, value| + response.headers[key] = value + end end end diff --git a/lib/api/api.rb b/lib/api/api.rb index ada0da28749a53..4a10dbb6317729 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -52,8 +52,6 @@ class API < ::API::Base api_endpoint = env['api.endpoint'] feature_category = api_endpoint.options[:for].try(:feature_category_for_app, api_endpoint).to_s - header[Gitlab::Metrics::RequestsRackMiddleware::FEATURE_CATEGORY_HEADER] = feature_category - Gitlab::ApplicationContext.push( user: -> { @current_user }, project: -> { @project }, @@ -64,6 +62,12 @@ class API < ::API::Base ) end + after do + Labkit::Context.current.to_headers.each do |key, value| + header[key] = value + end + end + before do set_peek_enabled_for_current_request end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 3742c3b1c3644f..e8c7fabe12897d 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -26,6 +26,13 @@ def check_unmodified_since!(last_modified) end end + # Override Grape::DSL::InsideRoute#error! to allow setting headers + # on all errors. See + # https://github.com/ruby-grape/grape/pull/1723#issuecomment-354129665 + def error!(message, status, additional_headers = {}) + super(message, status, additional_headers.merge(Labkit::Context.current.to_headers)) + end + def destroy_conditionally!(resource, last_updated: nil) last_updated ||= resource.updated_at diff --git a/lib/api/internal/base.rb b/lib/api/internal/base.rb index 12bb6e77c3e97b..0bfff7c104e342 100644 --- a/lib/api/internal/base.rb +++ b/lib/api/internal/base.rb @@ -10,8 +10,6 @@ class Base < ::API::Base api_endpoint = env['api.endpoint'] feature_category = api_endpoint.options[:for].try(:feature_category_for_app, api_endpoint).to_s - header[Gitlab::Metrics::RequestsRackMiddleware::FEATURE_CATEGORY_HEADER] = feature_category - Gitlab::ApplicationContext.push( user: -> { actor&.user }, project: -> { project }, @@ -21,6 +19,12 @@ class Base < ::API::Base ) end + after do + Labkit::Context.current.to_headers.each do |key, value| + header[key] = value + end + end + helpers ::API::Helpers::InternalHelpers UNKNOWN_CHECK_RESULT_ERROR = 'Unknown check result'.freeze diff --git a/lib/gitlab/metrics/requests_rack_middleware.rb b/lib/gitlab/metrics/requests_rack_middleware.rb index 23d7eb673129c8..de546a3f30449d 100644 --- a/lib/gitlab/metrics/requests_rack_middleware.rb +++ b/lib/gitlab/metrics/requests_rack_middleware.rb @@ -15,7 +15,7 @@ class RequestsRackMiddleware HEALTH_ENDPOINT = /^\/-\/(liveness|readiness|health|metrics)\/?$/.freeze - FEATURE_CATEGORY_HEADER = 'X-Gitlab-Feature-Category' + FEATURE_CATEGORY_HEADER = Labkit::Context.header_name(:feature_category) FEATURE_CATEGORY_DEFAULT = 'unknown' # These were the top 5 categories at a point in time, chosen as a diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 9342513d224e53..ff3e49792bdeda 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -914,15 +914,35 @@ def index it 'assigns the context to a variable for logging' do get :index, format: :json - expect(assigns(:current_context)).to include('meta.user' => user.username) + expect(assigns(:current_context).to_h).to include('meta.user' => user.username) end - it 'assigns the context when the action caused an error' do - allow(controller).to receive(:index) { raise 'Broken' } + it 'sets headers corresponding to the context' do + get :index, format: :json + + expect(response.headers.to_h).to include('X-Gitlab-Meta-Caller-Id' => 'AnonymousController#index', + 'X-Gitlab-Meta-Feature-Category' => 'issue_tracking', + 'X-Gitlab-Meta-User' => user.username) + end + + context 'when the action causes an error' do + before do + allow(controller).to receive(:index) { raise 'Broken' } + end + + it 'assigns the context to a variable' do + expect { get :index, format: :json }.to raise_error('Broken') - expect { get :index, format: :json }.to raise_error('Broken') + expect(assigns(:current_context).to_h).to include('meta.user' => user.username) + end - expect(assigns(:current_context)).to include('meta.user' => user.username) + it 'sets headers from the context' do + expect { get :index, format: :json }.to raise_error('Broken') + + expect(response.headers.to_h).to include('X-Gitlab-Meta-Caller-Id' => 'AnonymousController#index', + 'X-Gitlab-Meta-Feature-Category' => 'issue_tracking', + 'X-Gitlab-Meta-User' => user.username) + end end end diff --git a/spec/requests/api/api_spec.rb b/spec/requests/api/api_spec.rb index 9fd302131338fc..674a5c6c2607d9 100644 --- a/spec/requests/api/api_spec.rb +++ b/spec/requests/api/api_spec.rb @@ -93,37 +93,76 @@ end end - context 'application context' do + context 'application context', :aggregate_failures do let_it_be(:project) { create(:project) } let_it_be(:user) { project.owner } - it 'logs all application context fields' do - allow_any_instance_of(Gitlab::GrapeLogging::Loggers::ContextLogger).to receive(:parameters) do - Labkit::Context.current.to_h.tap do |log_context| - expect(log_context).to match('correlation_id' => an_instance_of(String), - 'meta.caller_id' => '/api/:version/projects/:id/issues', - 'meta.remote_ip' => an_instance_of(String), - 'meta.project' => project.full_path, - 'meta.root_namespace' => project.namespace.full_path, - 'meta.user' => user.username, - 'meta.feature_category' => 'issue_tracking') + context 'when all fields are present' do + it 'logs all application context fields' do + allow_any_instance_of(Gitlab::GrapeLogging::Loggers::ContextLogger).to receive(:parameters) do + Labkit::Context.current.to_h.tap do |log_context| + expect(log_context).to match('correlation_id' => an_instance_of(String), + 'meta.caller_id' => '/api/:version/projects/:id/issues', + 'meta.remote_ip' => an_instance_of(String), + 'meta.project' => project.full_path, + 'meta.root_namespace' => project.namespace.full_path, + 'meta.user' => user.username, + 'meta.feature_category' => 'issue_tracking') + end end + + get(api("/projects/#{project.id}/issues", user)) end - get(api("/projects/#{project.id}/issues", user)) + it 'puts context metadata in the headers' do + get(api("/projects/#{project.id}/issues", user)) + + expect(response.headers.to_h).to include('X-Gitlab-Meta-Caller-Id' => '/api/:version/projects/:id/issues', + 'X-Gitlab-Meta-Project' => project.full_path, + 'X-Gitlab-Meta-Root-Namespace' => project.namespace.full_path, + 'X-Gitlab-Meta-User' => user.username, + 'X-Gitlab-Meta-Feature-Category' => 'issue_tracking') + end end - it 'skips fields that do not apply' do - allow_any_instance_of(Gitlab::GrapeLogging::Loggers::ContextLogger).to receive(:parameters) do - Labkit::Context.current.to_h.tap do |log_context| - expect(log_context).to match('correlation_id' => an_instance_of(String), - 'meta.caller_id' => '/api/:version/users', - 'meta.remote_ip' => an_instance_of(String), - 'meta.feature_category' => 'users') + context 'when only some fields are present' do + it 'skips fields that do not apply' do + allow_any_instance_of(Gitlab::GrapeLogging::Loggers::ContextLogger).to receive(:parameters) do + Labkit::Context.current.to_h.tap do |log_context| + expect(log_context).to match('correlation_id' => an_instance_of(String), + 'meta.caller_id' => '/api/:version/users/:id', + 'meta.remote_ip' => an_instance_of(String), + 'meta.feature_category' => 'users') + end end + + get(api("/users/#{user.id}")) end - get(api('/users')) + it 'puts context metadata in the headers' do + get(api("/users/#{user.id}")) + + expect(response.headers.to_h).to include('X-Gitlab-Meta-Caller-Id' => '/api/:version/users/:id', + 'X-Gitlab-Meta-Feature-Category' => 'users') + end + end + + context 'when the request fails' do + it 'logs and puts context in headers' do + allow_any_instance_of(Gitlab::GrapeLogging::Loggers::ContextLogger).to receive(:parameters) do + Labkit::Context.current.to_h.tap do |log_context| + expect(log_context).to match('correlation_id' => an_instance_of(String), + 'meta.caller_id' => '/api/:version/users', + 'meta.feature_category' => 'users') + end + end + + get(api('/users')) + + expect(response).to have_gitlab_http_status(:forbidden) + expect(response.headers.to_h).to include('X-Gitlab-Meta-Caller-Id' => '/api/:version/users', + 'X-Gitlab-Meta-Feature-Category' => 'users') + end end end end -- GitLab From 66bfe06df9e4fb57eb6ab47d0343fc18325afb70 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 6 Jan 2021 12:54:07 +0000 Subject: [PATCH 2/2] Emit context metadata in a single JSON-encoded header --- app/controllers/application_controller.rb | 5 ++-- lib/api/api.rb | 6 ++--- lib/api/helpers.rb | 2 +- lib/api/internal/base.rb | 6 ++--- lib/gitlab/application_context.rb | 12 ++++++++++ .../application_controller_spec.rb | 16 ++++++++----- spec/requests/api/api_spec.rb | 24 ++++++++++++------- 7 files changed, 46 insertions(+), 25 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 90302671fd030f..b2c29c602e737d 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -276,6 +276,7 @@ def default_headers headers['X-XSS-Protection'] = '1; mode=block' headers['X-UA-Compatible'] = 'IE=edge' headers['X-Content-Type-Options'] = 'nosniff' + headers[Gitlab::Metrics::RequestsRackMiddleware::FEATURE_CATEGORY_HEADER] = feature_category end def default_cache_headers @@ -473,9 +474,7 @@ def set_current_context(&block) ensure @current_context = Labkit::Context.current - @current_context.to_headers.each do |key, value| - response.headers[key] = value - end + response.headers[Gitlab::ApplicationContext::HEADER] = Gitlab::ApplicationContext.current_to_json end end diff --git a/lib/api/api.rb b/lib/api/api.rb index 4a10dbb6317729..6ec88e0fe0bf5b 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -52,6 +52,8 @@ class API < ::API::Base api_endpoint = env['api.endpoint'] feature_category = api_endpoint.options[:for].try(:feature_category_for_app, api_endpoint).to_s + header[Gitlab::Metrics::RequestsRackMiddleware::FEATURE_CATEGORY_HEADER] = feature_category + Gitlab::ApplicationContext.push( user: -> { @current_user }, project: -> { @project }, @@ -63,9 +65,7 @@ class API < ::API::Base end after do - Labkit::Context.current.to_headers.each do |key, value| - header[key] = value - end + header[Gitlab::ApplicationContext::HEADER] = Gitlab::ApplicationContext.current_to_json end before do diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index e8c7fabe12897d..807dfdce93bebc 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -30,7 +30,7 @@ def check_unmodified_since!(last_modified) # on all errors. See # https://github.com/ruby-grape/grape/pull/1723#issuecomment-354129665 def error!(message, status, additional_headers = {}) - super(message, status, additional_headers.merge(Labkit::Context.current.to_headers)) + super(message, status, additional_headers.merge(Gitlab::ApplicationContext::HEADER => Gitlab::ApplicationContext.current_to_json)) end def destroy_conditionally!(resource, last_updated: nil) diff --git a/lib/api/internal/base.rb b/lib/api/internal/base.rb index 0bfff7c104e342..cb94f57f44492f 100644 --- a/lib/api/internal/base.rb +++ b/lib/api/internal/base.rb @@ -10,6 +10,8 @@ class Base < ::API::Base api_endpoint = env['api.endpoint'] feature_category = api_endpoint.options[:for].try(:feature_category_for_app, api_endpoint).to_s + header[Gitlab::Metrics::RequestsRackMiddleware::FEATURE_CATEGORY_HEADER] = feature_category + Gitlab::ApplicationContext.push( user: -> { actor&.user }, project: -> { project }, @@ -20,9 +22,7 @@ class Base < ::API::Base end after do - Labkit::Context.current.to_headers.each do |key, value| - header[key] = value - end + header[Gitlab::ApplicationContext::HEADER] = Gitlab::ApplicationContext.current_to_json end helpers ::API::Helpers::InternalHelpers diff --git a/lib/gitlab/application_context.rb b/lib/gitlab/application_context.rb index cefe983848cd90..d1d2f327259b96 100644 --- a/lib/gitlab/application_context.rb +++ b/lib/gitlab/application_context.rb @@ -3,6 +3,8 @@ module Gitlab # A GitLab-rails specific accessor for `Labkit::Logging::ApplicationContext` class ApplicationContext + HEADER = 'X-Gitlab-Meta' + include Gitlab::Utils::LazyAttributes Attribute = Struct.new(:name, :type) @@ -31,6 +33,16 @@ def self.current_context_include?(attribute_name) Labkit::Context.current.to_h.include?(Labkit::Context.log_key(attribute_name)) end + def self.current_to_json + Labkit::Context + .current + .to_h + .transform_keys { |k| k.delete_prefix('meta.') } + .except('correlation_id', 'remote_ip') + .merge('version' => '1') + .to_json + end + def initialize(**args) unknown_attributes = args.keys - APPLICATION_ATTRIBUTES.map(&:name) raise ArgumentError, "#{unknown_attributes} are not known keys" if unknown_attributes.any? diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index ff3e49792bdeda..bf50dab526715b 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -865,6 +865,8 @@ def index let_it_be(:user) { create(:user) } + let(:meta_header) { Gitlab::Json.parse(response.headers[Gitlab::ApplicationContext::HEADER]) } + before do sign_in(user) end @@ -920,9 +922,10 @@ def index it 'sets headers corresponding to the context' do get :index, format: :json - expect(response.headers.to_h).to include('X-Gitlab-Meta-Caller-Id' => 'AnonymousController#index', - 'X-Gitlab-Meta-Feature-Category' => 'issue_tracking', - 'X-Gitlab-Meta-User' => user.username) + expect(meta_header).to match('caller_id' => 'AnonymousController#index', + 'feature_category' => 'issue_tracking', + 'user' => user.username, + 'version' => '1') end context 'when the action causes an error' do @@ -939,9 +942,10 @@ def index it 'sets headers from the context' do expect { get :index, format: :json }.to raise_error('Broken') - expect(response.headers.to_h).to include('X-Gitlab-Meta-Caller-Id' => 'AnonymousController#index', - 'X-Gitlab-Meta-Feature-Category' => 'issue_tracking', - 'X-Gitlab-Meta-User' => user.username) + expect(meta_header).to match('caller_id' => 'AnonymousController#index', + 'feature_category' => 'issue_tracking', + 'user' => user.username, + 'version' => '1') end end end diff --git a/spec/requests/api/api_spec.rb b/spec/requests/api/api_spec.rb index 674a5c6c2607d9..cc9b4f5341cf0b 100644 --- a/spec/requests/api/api_spec.rb +++ b/spec/requests/api/api_spec.rb @@ -97,6 +97,8 @@ let_it_be(:project) { create(:project) } let_it_be(:user) { project.owner } + let(:meta_header) { Gitlab::Json.parse(response.headers[Gitlab::ApplicationContext::HEADER]) } + context 'when all fields are present' do it 'logs all application context fields' do allow_any_instance_of(Gitlab::GrapeLogging::Loggers::ContextLogger).to receive(:parameters) do @@ -117,11 +119,12 @@ it 'puts context metadata in the headers' do get(api("/projects/#{project.id}/issues", user)) - expect(response.headers.to_h).to include('X-Gitlab-Meta-Caller-Id' => '/api/:version/projects/:id/issues', - 'X-Gitlab-Meta-Project' => project.full_path, - 'X-Gitlab-Meta-Root-Namespace' => project.namespace.full_path, - 'X-Gitlab-Meta-User' => user.username, - 'X-Gitlab-Meta-Feature-Category' => 'issue_tracking') + expect(meta_header).to eq('caller_id' => '/api/:version/projects/:id/issues', + 'project' => project.full_path, + 'root_namespace' => project.namespace.full_path, + 'user' => user.username, + 'feature_category' => 'issue_tracking', + 'version' => '1') end end @@ -142,8 +145,9 @@ it 'puts context metadata in the headers' do get(api("/users/#{user.id}")) - expect(response.headers.to_h).to include('X-Gitlab-Meta-Caller-Id' => '/api/:version/users/:id', - 'X-Gitlab-Meta-Feature-Category' => 'users') + expect(meta_header).to eq('caller_id' => '/api/:version/users/:id', + 'feature_category' => 'users', + 'version' => '1') end end @@ -153,6 +157,7 @@ Labkit::Context.current.to_h.tap do |log_context| expect(log_context).to match('correlation_id' => an_instance_of(String), 'meta.caller_id' => '/api/:version/users', + 'meta.remote_ip' => an_instance_of(String), 'meta.feature_category' => 'users') end end @@ -160,8 +165,9 @@ get(api('/users')) expect(response).to have_gitlab_http_status(:forbidden) - expect(response.headers.to_h).to include('X-Gitlab-Meta-Caller-Id' => '/api/:version/users', - 'X-Gitlab-Meta-Feature-Category' => 'users') + expect(meta_header).to eq('caller_id' => '/api/:version/users', + 'feature_category' => 'users', + 'version' => '1') end end end -- GitLab