Make global helpers work in error method callbacks (#1216)

When using `Appsignal.report_error` and `Appsignal.send_error` (and
`Appsignal.set_error`, more on that later), it was required to customize
the metadata by calling the `set_*` methods directly on the Transaction
object.

This is the only time that is needed in our public API and wasn't ever
really documented what methods applications can call, and how they might
be different from the helpers on `Appsignal`.

(I don't think there are any differences now, but there were in the past
when we didn't have global helpers for custom data, parameters, headers
and session data.)

To make matters simpler for our end-user, make the `Appsignal.set_*`
helpers work in the callback of `Appsignal.report_error` and
`Appsignal.send_error` by temporarily switching the current transaction
for the duration of that callback block.

In the `Appsignal.set_error` helper callback, applications could already
use `Appsignal.set_*` helpers, because it can only set errors an active
transaction so I only updated the API docs for it.

.changesets/global-helpers-work-inside-report_error-and-send_error-callbacks.md

@@ -0,0 +1,36 @@
+---
+bump: major
+type: change
+---
+
+Global transaction metadata helpers now work inside the `Appsignal.report_error` and `Appsignal.send_error` callbacks. The transaction yield parameter will continue to work, but we recommend using the global `Appsignal.set_*` helpers.
+
+```ruby
+# Before
+Appsignal.report_error(error) do |transaction|
+  transaction.set_namespace("my namespace")
+  transaction.set_action("my action")
+  transaction.add_tags(:tag_a => "value", :tag_b => "value")
+  # etc.
+end
+Appsignal.send_error(error) do |transaction|
+  transaction.set_namespace("my namespace")
+  transaction.set_action("my action")
+  transaction.add_tags(:tag_a => "value", :tag_b => "value")
+  # etc.
+end
+
+# After
+Appsignal.report_error(error) do
+  Appsignal.set_namespace("my namespace")
+  Appsignal.set_action("my action")
+  Appsignal.add_tags(:tag_a => "value", :tag_b => "value")
+  # etc.
+end
+Appsignal.send_error(error) do
+  Appsignal.set_namespace("my namespace")
+  Appsignal.set_action("my action")
+  Appsignal.add_tags(:tag_a => "value", :tag_b => "value")
+  # etc.
+end
+```

lib/appsignal/helpers/instrumentation.rb

@@ -179,11 +179,11 @@ def monitor_and_stop(action:, namespace: nil, &block)
       #   end
       #
       # @example Add more metadata to transaction
-      #   Appsignal.send_error(e) do |transaction|
-      #     transaction.set_namespace("my_namespace")
-      #     transaction.set_action("my_action_name")
-      #     transaction.add_params(:search_query => params[:search_query])
-      #     transaction.add_tags(:key => "value")
+      #   Appsignal.send_error(e) do
+      #     Appsignal.set_namespace("my_namespace")
+      #     Appsignal.set_action("my_action_name")
+      #     Appsignal.add_params(:search_query => params[:search_query])
+      #     Appsignal.add_tags(:key => "value")
       #   end
       #
       # @param error [Exception] The error to send to AppSignal.
@@ -213,6 +213,7 @@ def send_error(error, &block)
           Appsignal::Transaction::HTTP_REQUEST
         )
         transaction.set_error(error, &block)
+
         transaction.complete
       end
       alias :send_exception :send_error
@@ -245,11 +246,11 @@ def send_error(error, &block)
       #   end
       #
       # @example Add more metadata to transaction
-      #   Appsignal.set_error(e) do |transaction|
-      #     transaction.set_namespace("my_namespace")
-      #     transaction.set_action("my_action_name")
-      #     transaction.add_params(:search_query => params[:search_query])
-      #     transaction.add_tags(:key => "value")
+      #   Appsignal.set_error(e) do
+      #     Appsignal.set_namespace("my_namespace")
+      #     Appsignal.set_action("my_action_name")
+      #     Appsignal.add_params(:search_query => params[:search_query])
+      #     Appsignal.add_tags(:key => "value")
       #   end
       #
       # @param exception [Exception] The error to add to the current
@@ -301,11 +302,11 @@ def set_error(exception)
       #   end
       #
       # @example Add more metadata to transaction
-      #   Appsignal.report_error(error) do |transaction|
-      #     transaction.set_namespace("my_namespace")
-      #     transaction.set_action("my_action_name")
-      #     transaction.add_params(:search_query => params[:search_query])
-      #     transaction.add_tags(:key => "value")
+      #   Appsignal.report_error(error) do
+      #     Appsignal.set_namespace("my_namespace")
+      #     Appsignal.set_action("my_action_name")
+      #     Appsignal.add_params(:search_query => params[:search_query])
+      #     Appsignal.add_tags(:key => "value")
       #   end
       #
       # @param exception [Exception] The error to add to the current

lib/appsignal/integrations/railtie.rb

@@ -83,9 +83,10 @@ def report(error, handled:, severity:, context: {}, source: nil)
           # and are caught and recorded by our Rails middleware.
           return if !handled && !is_rails_runner
 
+          namespace, action_name, path, method, params, tags, custom_data =
+            context_for(context.dup)
+
           Appsignal.send_error(error) do |transaction|
-            namespace, action_name, path, method, params, tags, custom_data =
-              context_for(context.dup)
             if namespace
               transaction.set_namespace(namespace)
             elsif is_rails_runner

lib/appsignal/transaction.rb

@@ -30,11 +30,9 @@ def create(namespace)
         # Check if we already have a running transaction
         if Thread.current[:appsignal_transaction].nil?
           # If not, start a new transaction
-          Thread.current[:appsignal_transaction] =
-            Appsignal::Transaction.new(
-              SecureRandom.uuid,
-              namespace
-            )
+          set_current_transaction(
+            Appsignal::Transaction.new(SecureRandom.uuid, namespace)
+          )
         else
           # Otherwise, log the issue about trying to start another transaction
           Appsignal.internal_logger.warn(
@@ -48,6 +46,23 @@ def create(namespace)
         end
       end
 
+      # @api private
+      def set_current_transaction(transaction)
+        Thread.current[:appsignal_transaction] = transaction
+      end
+
+      # Set the current for the duration of the given block.
+      # It restores the original transaction (if any) when the block has executed.
+      #
+      # @api private
+      def with_transaction(transaction)
+        original_transaction = current if current?
+        set_current_transaction(transaction)
+        yield
+      ensure
+        set_current_transaction(original_transaction)
+      end
+
       # Returns currently active transaction or a {NilTransaction} if none is
       # active.
       #
@@ -167,7 +182,13 @@ def complete
         end
       end
 
-      @error_blocks[@error_set].each { |block| block.call(self) } if @error_set
+      if @error_set && @error_blocks[@error_set].any?
+        self.class.with_transaction(self) do
+          @error_blocks[@error_set].each do |block|
+            block.call(self)
+          end
+        end
+      end
       sample_data if should_sample
       ext.complete
     end

spec/lib/appsignal_spec.rb

@@ -1312,15 +1312,50 @@ def on_start
 
       context "when given a block" do
         it "yields the transaction and allows additional metadata to be set" do
-          keep_transactions do
-            Appsignal.send_error(StandardError.new("my_error")) do |transaction|
-              transaction.set_action("my_action")
-              transaction.set_namespace("my_namespace")
-            end
+          Appsignal.send_error(StandardError.new("my_error")) do |transaction|
+            transaction.set_action("my_action")
+            transaction.set_namespace("my_namespace")
+          end
+
+          expect(last_transaction).to have_namespace("my_namespace")
+          expect(last_transaction).to have_action("my_action")
+          expect(last_transaction).to have_error("StandardError", "my_error")
+        end
+
+        it "yields and allows additional metadata to be set with global helpers" do
+          Appsignal.send_error(StandardError.new("my_error")) do
+            Appsignal.set_action("my_action")
+            Appsignal.set_namespace("my_namespace")
+          end
+
+          expect(last_transaction).to have_namespace("my_namespace")
+          expect(last_transaction).to have_action("my_action")
+          expect(last_transaction).to have_error("StandardError", "my_error")
+        end
+
+        it "yields to set metadata and doesn't modify the active transaction" do
+          active_transaction = http_request_transaction
+          active_transaction.set_action("active action")
+          active_transaction.set_namespace("active namespace")
+          set_current_transaction(active_transaction)
+          expect(current_transaction).to eq(active_transaction)
+
+          Appsignal.send_error(StandardError.new("my_error")) do
+            Appsignal.set_action("my_action")
+            Appsignal.set_namespace("my_namespace")
           end
+
+          # Restores the active_transaction as the current transaction
+          expect(current_transaction).to eq(active_transaction)
+
           expect(last_transaction).to have_namespace("my_namespace")
           expect(last_transaction).to have_action("my_action")
           expect(last_transaction).to have_error("StandardError", "my_error")
+          expect(last_transaction).to be_completed
+
+          expect(active_transaction).to have_namespace("active namespace")
+          expect(active_transaction).to have_action("active action")
+          expect(active_transaction).to_not be_completed
         end
       end
     end
@@ -1442,6 +1477,21 @@ def on_start
             expect(transaction).to include_tags("tag1" => "value1")
             expect(transaction).to be_completed
           end
+
+          it "yields and allows additional metadata to be set with the global helpers" do
+            Appsignal.report_error(error) do
+              Appsignal.set_action("my_action")
+              Appsignal.set_namespace("my_namespace")
+              Appsignal.set_tags(:tag1 => "value1")
+            end
+
+            transaction = last_transaction
+            expect(transaction).to have_namespace("my_namespace")
+            expect(transaction).to have_action("my_action")
+            expect(transaction).to have_error("ExampleException", "error message")
+            expect(transaction).to include_tags("tag1" => "value1")
+            expect(transaction).to be_completed
+          end
         end
       end
 
@@ -1546,6 +1596,22 @@ def on_start
           it "does not create a new transaction" do
             expect(created_transactions).to eq([transaction])
           end
+
+          it "yields and allows additional metadata to be set with the global helpers" do
+            Appsignal.report_error(error) do
+              Appsignal.set_action("my_action")
+              Appsignal.set_namespace("my_namespace")
+              Appsignal.set_tags(:tag1 => "value1")
+            end
+
+            expect(transaction).to_not be_completed
+
+            transaction.complete
+            expect(transaction).to have_namespace("my_namespace")
+            expect(transaction).to have_action("my_action")
+            expect(transaction).to have_error("ExampleException", "error message")
+            expect(transaction).to include_tags("tag1" => "value1")
+          end
         end
       end
     end