Skip to content

Commit

Permalink
Rails error reporter Sidekiq action and namespace (#955)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
tombruijn authored Apr 14, 2023
1 parent 5857f37 commit b4f58af
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 6 deletions.
6 changes: 6 additions & 0 deletions .changesets/support-sidekiq-in-rails-error-reporter.md
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 1 addition & 1 deletion lib/appsignal/hooks/active_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
10 changes: 5 additions & 5 deletions lib/appsignal/integrations/sidekiq.rb
Original file line number Diff line number Diff line change
Expand Up @@ -52,21 +52,21 @@ 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
job_status = :failed
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

Expand Down
28 changes: 28 additions & 0 deletions spec/lib/appsignal/integrations/sidekiq_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 11 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit b4f58af

Please sign in to comment.