From b4f58afdeb80cd1eb336ec5bd7b5daf46a4ef0a8 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Fri, 14 Apr 2023 10:48:17 +0200 Subject: [PATCH] Rails error reporter Sidekiq action and namespace (#955) Report the Sidekiq worker's action name, namespace and tags for Sidekiq jobs. Move the order in which these are set to before the Sidekiq job is performed. That way that information is available to the Rails error reporter context and we can copy the action, namespace and tags to the error reported with `Appsignal.send_error`. Change the Active Job instrumentation, to use `set_action`, rather than `set_action_if_nil`. This will overwrite the action name, rather than only set it if not already set. Because it's set before the job, this will not overwrite any action name set by the user. This is needed for the Active Job + Sidekiq instrumentation to work again and use the Active Job's instrumentation naming for Active Job jobs, run with Sidekiq. Part of #779 --- ...support-sidekiq-in-rails-error-reporter.md | 6 ++++ lib/appsignal/hooks/active_job.rb | 2 +- lib/appsignal/integrations/sidekiq.rb | 10 +++---- .../appsignal/integrations/sidekiq_spec.rb | 28 +++++++++++++++++++ spec/spec_helper.rb | 11 ++++++++ 5 files changed, 51 insertions(+), 6 deletions(-) create mode 100644 .changesets/support-sidekiq-in-rails-error-reporter.md diff --git a/.changesets/support-sidekiq-in-rails-error-reporter.md b/.changesets/support-sidekiq-in-rails-error-reporter.md new file mode 100644 index 000000000..490f96ada --- /dev/null +++ b/.changesets/support-sidekiq-in-rails-error-reporter.md @@ -0,0 +1,6 @@ +--- +bump: "patch" +type: "add" +--- + +Support Sidekiq in Rails error reporter. Track errors reported using `Rails.error.handle` in Sidekiq jobs, in the correct action. Previously it would report no action name for the incident, now it will use the worker name by default. diff --git a/lib/appsignal/hooks/active_job.rb b/lib/appsignal/hooks/active_job.rb index 5ba0f1002..7cace5633 100644 --- a/lib/appsignal/hooks/active_job.rb +++ b/lib/appsignal/hooks/active_job.rb @@ -50,7 +50,7 @@ def execute(job) transaction_tags[:provider_job_id] = provider_job_id if provider_job_id transaction.set_tags(transaction_tags) - transaction.set_action_if_nil(ActiveJobHelpers.action_name(job)) + transaction.set_action(ActiveJobHelpers.action_name(job)) end super diff --git a/lib/appsignal/integrations/sidekiq.rb b/lib/appsignal/integrations/sidekiq.rb index ee3fa5821..c8ba16d5c 100644 --- a/lib/appsignal/integrations/sidekiq.rb +++ b/lib/appsignal/integrations/sidekiq.rb @@ -52,6 +52,11 @@ def call(_worker, item, _queue, &block) :queue_start => item["enqueued_at"] ) ) + transaction.set_action_if_nil(formatted_action_name(item)) + + formatted_metadata(item).each do |key, value| + transaction.set_metadata key, value + end Appsignal.instrument "perform_job.sidekiq", &block rescue Exception => exception # rubocop:disable Lint/RescueException @@ -59,14 +64,9 @@ def call(_worker, item, _queue, &block) raise exception ensure if transaction - transaction.set_action_if_nil(formatted_action_name(item)) - params = filtered_arguments(item) transaction.params = params if params - formatted_metadata(item).each do |key, value| - transaction.set_metadata key, value - end transaction.set_http_or_background_queue_start Appsignal::Transaction.complete_current! unless exception diff --git a/spec/lib/appsignal/integrations/sidekiq_spec.rb b/spec/lib/appsignal/integrations/sidekiq_spec.rb index ed1d7e0d5..d2a985d97 100644 --- a/spec/lib/appsignal/integrations/sidekiq_spec.rb +++ b/spec/lib/appsignal/integrations/sidekiq_spec.rb @@ -264,6 +264,34 @@ class DelayedTestClass; end end end + if DependencyHelper.rails7_present? + context "with Rails error reporter" do + it "reports the worker name as the action, copies the namespace and tags" do + Appsignal::Integrations::Railtie.initialize_appsignal(MyApp::Application.new) + Appsignal.config = project_fixture_config("production") + perform_job do + Appsignal.tag_job("test_tag" => "value") + Rails.error.handle do + raise error, "uh oh" + end + end + + expect(created_transactions.count).to eq(2) + expected_transaction = { + "namespace" => "background_job", + "action" => "TestClass#perform", + "sample_data" => hash_including( + "tags" => hash_including("test_tag" => "value") + ) + } + sidekiq_transaction = created_transactions.first.to_h + error_reporter_transaction = created_transactions.last.to_h + expect(sidekiq_transaction).to include(expected_transaction) + expect(error_reporter_transaction).to include(expected_transaction) + end + end + end + context "without an error" do it "creates a transaction with events" do # TODO: additional curly brackets required for issue diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 0be6f1e48..6b50ff41c 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -121,6 +121,17 @@ def spec_system_tmp_dir # Stub system_tmp_dir to something in the project dir for specs allow(Appsignal::Config).to receive(:system_tmp_dir).and_return(spec_system_tmp_dir) + + # Unsubscribe Rails error reporter if present to avoid it reporting errors + # multiple times through multiple subscriptions. + if defined?(Rails) && Rails.respond_to?(:error) + if Rails.error.respond_to?(:unsubscribe) # Future Rails version after 7.0.4.3 + Rails.error.unsubscribe(Appsignal::Integrations::RailsErrorReporterSubscriber) + else + Rails.error.instance_variable_get(:@subscribers) + .delete(Appsignal::Integrations::RailsErrorReporterSubscriber) + end + end end # These tests are not run by default. They require a failed extension