Commit 5d639f3f authored by Markus Koller's avatar Markus Koller

Use webhook context when logging rate-limit

Include the hook metadata in the log, and remove the redundant call
to `Gitlab::AppLogger` since `auth.log` is visible in Kibana as well.
parent 772754bc
...@@ -91,14 +91,12 @@ class WebHookService ...@@ -91,14 +91,12 @@ class WebHookService
end end
def async_execute def async_execute
if rate_limited?(hook)
log_rate_limit(hook)
else
Gitlab::ApplicationContext.with_context(hook.application_context) do Gitlab::ApplicationContext.with_context(hook.application_context) do
break log_rate_limit if rate_limited?
WebHookWorker.perform_async(hook.id, data, hook_name) WebHookWorker.perform_async(hook.id, data, hook_name)
end end
end end
end
private private
...@@ -177,7 +175,7 @@ class WebHookService ...@@ -177,7 +175,7 @@ class WebHookService
response.body.encode('UTF-8', invalid: :replace, undef: :replace, replace: '') response.body.encode('UTF-8', invalid: :replace, undef: :replace, replace: '')
end end
def rate_limited?(hook) def rate_limited?
return false unless Feature.enabled?(:web_hooks_rate_limit, default_enabled: :yaml) return false unless Feature.enabled?(:web_hooks_rate_limit, default_enabled: :yaml)
return false if rate_limit.nil? return false if rate_limit.nil?
...@@ -192,18 +190,13 @@ class WebHookService ...@@ -192,18 +190,13 @@ class WebHookService
@rate_limit ||= hook.rate_limit @rate_limit ||= hook.rate_limit
end end
def log_rate_limit(hook) def log_rate_limit
payload = { Gitlab::AuthLogger.error(
message: 'Webhook rate limit exceeded', message: 'Webhook rate limit exceeded',
hook_id: hook.id, hook_id: hook.id,
hook_type: hook.type, hook_type: hook.type,
hook_name: hook_name hook_name: hook_name,
} **Gitlab::ApplicationContext.current
)
Gitlab::AuthLogger.error(payload)
# Also log into application log for now, so we can use this information
# to determine suitable limits for gitlab.com
Gitlab::AppLogger.error(payload)
end end
end end
...@@ -375,15 +375,18 @@ RSpec.describe WebHookService do ...@@ -375,15 +375,18 @@ RSpec.describe WebHookService do
it 'does not queue a worker and logs an error' do it 'does not queue a worker and logs an error' do
expect(WebHookWorker).not_to receive(:perform_async) expect(WebHookWorker).not_to receive(:perform_async)
payload = { expect(Gitlab::AuthLogger).to receive(:error).with(
include(
message: 'Webhook rate limit exceeded', message: 'Webhook rate limit exceeded',
hook_id: project_hook.id, hook_id: project_hook.id,
hook_type: 'ProjectHook', hook_type: 'ProjectHook',
hook_name: 'push_hooks' hook_name: 'push_hooks',
} "correlation_id" => kind_of(String),
"meta.project" => project.full_path,
expect(Gitlab::AuthLogger).to receive(:error).with(payload) "meta.related_class" => 'ProjectHook',
expect(Gitlab::AppLogger).to receive(:error).with(payload) "meta.root_namespace" => project.root_namespace.full_path
)
)
service_instance.async_execute service_instance.async_execute
end end
...@@ -403,7 +406,6 @@ RSpec.describe WebHookService do ...@@ -403,7 +406,6 @@ RSpec.describe WebHookService do
it 'stops queueing workers and logs errors' do it 'stops queueing workers and logs errors' do
expect(Gitlab::AuthLogger).to receive(:error).twice expect(Gitlab::AuthLogger).to receive(:error).twice
expect(Gitlab::AppLogger).to receive(:error).twice
2.times { service_instance.async_execute } 2.times { service_instance.async_execute }
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