diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 905a4a7212e5f402615e39d9f86b8fda620d50dc..b2c29c602e737db0763fc0158661b7fe38880f98 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? @@ -472,7 +472,9 @@ def set_current_context(&block) feature_category: feature_category) do yield ensure - @current_context = Labkit::Context.current.to_h + @current_context = Labkit::Context.current + + 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 ada0da28749a53399d60bc27a3c8df785f675cd1..6ec88e0fe0bf5bd65c4e2205d869adb448e75922 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -64,6 +64,10 @@ class API < ::API::Base ) end + after do + header[Gitlab::ApplicationContext::HEADER] = Gitlab::ApplicationContext.current_to_json + end + before do set_peek_enabled_for_current_request end diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 3742c3b1c3644f1be03cc30f7de57ec731291009..807dfdce93bebc02dcae0149d25dc4fefa5c0468 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(Gitlab::ApplicationContext::HEADER => Gitlab::ApplicationContext.current_to_json)) + 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 12bb6e77c3e97bb101b3ae44fa20b0d396cdfd10..cb94f57f44492f0df21f5101c3e83fc2bba7a040 100644 --- a/lib/api/internal/base.rb +++ b/lib/api/internal/base.rb @@ -21,6 +21,10 @@ class Base < ::API::Base ) end + after do + header[Gitlab::ApplicationContext::HEADER] = Gitlab::ApplicationContext.current_to_json + end + helpers ::API::Helpers::InternalHelpers UNKNOWN_CHECK_RESULT_ERROR = 'Unknown check result'.freeze diff --git a/lib/gitlab/application_context.rb b/lib/gitlab/application_context.rb index cefe983848cd909a77c6e339d78c0dacf021dcb7..d1d2f327259b9622b11fce01be734f619048103e 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/lib/gitlab/metrics/requests_rack_middleware.rb b/lib/gitlab/metrics/requests_rack_middleware.rb index 23d7eb673129c851b45117d6bf972c90e550c38c..de546a3f30449d9d0fddfeb12def909db7eabd4f 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 9342513d224e53f8ff3ef76681483245417f3510..bf50dab526715b72c5d1b7f1dc97e4c2dfa62007 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 @@ -914,15 +916,37 @@ 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(meta_header).to match('caller_id' => 'AnonymousController#index', + 'feature_category' => 'issue_tracking', + 'user' => user.username, + 'version' => '1') + end - expect { get :index, format: :json }.to raise_error('Broken') + context 'when the action causes an error' do + before do + allow(controller).to receive(:index) { raise 'Broken' } + end - expect(assigns(:current_context)).to include('meta.user' => user.username) + it 'assigns the context to a variable' do + expect { get :index, format: :json }.to raise_error('Broken') + + expect(assigns(:current_context).to_h).to include('meta.user' => user.username) + end + + it 'sets headers from the context' do + expect { get :index, format: :json }.to raise_error('Broken') + + 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 9fd302131338fc35fa0fd079b7b7cf25dc2455bc..cc9b4f5341cf0b6f5fab0e5b4875306fceaf82c1 100644 --- a/spec/requests/api/api_spec.rb +++ b/spec/requests/api/api_spec.rb @@ -93,37 +93,82 @@ 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') + 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 + 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(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 - 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(meta_header).to eq('caller_id' => '/api/:version/users/:id', + 'feature_category' => 'users', + 'version' => '1') + 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.remote_ip' => an_instance_of(String), + 'meta.feature_category' => 'users') + end + end + + get(api('/users')) + + expect(response).to have_gitlab_http_status(:forbidden) + expect(meta_header).to eq('caller_id' => '/api/:version/users', + 'feature_category' => 'users', + 'version' => '1') + end end end end