Deprecate Transaction.create/new arguments

Remove the transaction ID argument. Have the Transaction generate its own ID. Previously, we used request/job IDs for Transaction IDs, but that's been refactored out to use the `request_id` tag. No longer require the request argument. When none is given it will use the fallback InternalGenericRequest, but this is backwards compatibility step until we remove the request from the Transaction entirely. Transaction metadata will need to be set using the `Appsignal.set_*` helpers.
appsignal · Jul 11, 2024 · 2fc2c61 · 2fc2c61
1 parent 1c69d3f
commit 2fc2c61
Show file tree

Hide file tree

Showing 20 changed files with 1,662 additions and 1,485 deletions.
diff --git a/.changesets/deprecate-arguments-for-transaction-create-and-transaction-new.md b/.changesets/deprecate-arguments-for-transaction-create-and-transaction-new.md
@@ -0,0 +1,18 @@
+---
+bump: patch
+type: deprecate
+---
+
+Deprecate the 'ID', 'request', and 'options' arguments for the `Transaction.create` and `Transaction.new` methods. To add metadata to the transaction, use the `Appsignal.set_*` helpers. Read our [sample data guide](https://docs.appsignal.com/guides/custom-data/sample-data.html) for more information on how to set metadata on transactions.
+
+```ruby
+# Before
+Appsignal::Transaction.create(
+  SecureRandom.uuid,
+  "my_namespace",
+  Appsignal::Transaction::GenericRequest.new(env) # env is a request env Hash
+)
+
+# After
+Appsignal::Transaction.create("my_namespace")
+```
diff --git a/benchmark.rake b/benchmark.rake
@@ -67,11 +67,9 @@ def start_agent
 end
 
 def monitor_transaction(transaction_id)
-  request = Appsignal::Transaction::InternalGenericRequest.new({})
   transaction = Appsignal::Transaction.create(
     transaction_id,
-    Appsignal::Transaction::HTTP_REQUEST,
-    request
+    Appsignal::Transaction::HTTP_REQUEST
   )
   transaction.set_action("HomeController#show")
   transaction.set_params(:id => 1)

diff --git a/lib/appsignal/demo.rb b/lib/appsignal/demo.rb
@@ -39,11 +39,7 @@ def transmit
       private
 
       def create_example_error_request
-        transaction = Appsignal::Transaction.create(
-          SecureRandom.uuid,
-          Appsignal::Transaction::HTTP_REQUEST,
-          Appsignal::Transaction::InternalGenericRequest.new({})
-        )
+        transaction = Appsignal::Transaction.create(Appsignal::Transaction::HTTP_REQUEST)
         begin
           raise TestError,
             "Hello world! This is an error used for demonstration purposes."
@@ -60,11 +56,7 @@ def create_example_error_request
       end
 
       def create_example_performance_request
-        transaction = Appsignal::Transaction.create(
-          SecureRandom.uuid,
-          Appsignal::Transaction::HTTP_REQUEST,
-          Appsignal::Transaction::InternalGenericRequest.new({})
-        )
+        transaction = Appsignal::Transaction.create(Appsignal::Transaction::HTTP_REQUEST)
         Appsignal.instrument "action_view.render", "Render hello.html.erb",
           "<h1>Hello world!</h1>" do
           sleep 2

diff --git a/lib/appsignal/helpers/instrumentation.rb b/lib/appsignal/helpers/instrumentation.rb
@@ -237,8 +237,7 @@ def send_error(
         end
         transaction = Appsignal::Transaction.new(
           SecureRandom.uuid,
-          namespace || Appsignal::Transaction::HTTP_REQUEST,
-          Appsignal::Transaction::InternalGenericRequest.new({})
+          namespace || Appsignal::Transaction::HTTP_REQUEST
         )
         transaction.set_tags(tags) if tags
         transaction.set_error(error)
@@ -392,8 +391,7 @@ def report_error(exception)
           else
             Appsignal::Transaction.new(
               SecureRandom.uuid,
-              Appsignal::Transaction::HTTP_REQUEST,
-              Appsignal::Transaction::InternalGenericRequest.new({})
+              Appsignal::Transaction::HTTP_REQUEST
             )
           end
 

diff --git a/lib/appsignal/hooks/action_cable.rb b/lib/appsignal/hooks/action_cable.rb
@@ -36,11 +36,8 @@ def install_subscribe_callback
           request_id = request.request_id || SecureRandom.uuid
           env[Appsignal::Hooks::ActionCableHook::REQUEST_ID] ||= request_id
 
-          transaction = Appsignal::Transaction.create(
-            env[Appsignal::Hooks::ActionCableHook::REQUEST_ID],
-            Appsignal::Transaction::ACTION_CABLE,
-            Appsignal::Transaction::InternalGenericRequest.new({})
-          )
+          transaction =
+            Appsignal::Transaction.create(Appsignal::Transaction::ACTION_CABLE)
 
           begin
             Appsignal.instrument "subscribed.action_cable" do
@@ -75,11 +72,8 @@ def install_unsubscribe_callback
           request_id = request.request_id || SecureRandom.uuid
           env[Appsignal::Hooks::ActionCableHook::REQUEST_ID] ||= request_id
 
-          transaction = Appsignal::Transaction.create(
-            env[Appsignal::Hooks::ActionCableHook::REQUEST_ID],
-            Appsignal::Transaction::ACTION_CABLE,
-            Appsignal::Transaction::InternalGenericRequest.new({})
-          )
+          transaction =
+            Appsignal::Transaction.create(Appsignal::Transaction::ACTION_CABLE)
 
           begin
             Appsignal.instrument "unsubscribed.action_cable" do

diff --git a/lib/appsignal/hooks/active_job.rb b/lib/appsignal/hooks/active_job.rb
@@ -54,11 +54,7 @@ def execute(job)
               # we do for Sidekiq.
               #
               # Prefer job_id from provider, instead of ActiveJob's internal ID.
-              Appsignal::Transaction.create(
-                SecureRandom.uuid,
-                Appsignal::Transaction::BACKGROUND_JOB,
-                Appsignal::Transaction::InternalGenericRequest.new({})
-              )
+              Appsignal::Transaction.create(Appsignal::Transaction::BACKGROUND_JOB)
             end
 
           if transaction

diff --git a/lib/appsignal/integrations/action_cable.rb b/lib/appsignal/integrations/action_cable.rb
@@ -11,11 +11,7 @@ def perform_action(*args, &block)
         request_id = request.request_id || SecureRandom.uuid
         env[Appsignal::Hooks::ActionCableHook::REQUEST_ID] ||= request_id
 
-        transaction = Appsignal::Transaction.create(
-          SecureRandom.uuid,
-          Appsignal::Transaction::ACTION_CABLE,
-          Appsignal::Transaction::InternalGenericRequest.new({})
-        )
+        transaction = Appsignal::Transaction.create(Appsignal::Transaction::ACTION_CABLE)
 
         begin
           super

diff --git a/lib/appsignal/integrations/delayed_job_plugin.rb b/lib/appsignal/integrations/delayed_job_plugin.rb
@@ -17,11 +17,8 @@ class DelayedJobPlugin < ::Delayed::Plugin
       end
 
       def self.invoke_with_instrumentation(job, block)
-        transaction = Appsignal::Transaction.create(
-          SecureRandom.uuid,
-          Appsignal::Transaction::BACKGROUND_JOB,
-          Appsignal::Transaction::InternalGenericRequest.new({})
-        )
+        transaction =
+          Appsignal::Transaction.create(Appsignal::Transaction::BACKGROUND_JOB)
 
         Appsignal.instrument("perform_job.delayed_job") do
           block.call(job)

diff --git a/lib/appsignal/integrations/que.rb b/lib/appsignal/integrations/que.rb
@@ -5,11 +5,8 @@ module Integrations
     # @api private
     module QuePlugin
       def _run(*)
-        transaction = Appsignal::Transaction.create(
-          SecureRandom.uuid,
-          Appsignal::Transaction::BACKGROUND_JOB,
-          Appsignal::Transaction::InternalGenericRequest.new({})
-        )
+        transaction =
+          Appsignal::Transaction.create(Appsignal::Transaction::BACKGROUND_JOB)
 
         begin
           Appsignal.instrument("perform_job.que") { super }

diff --git a/lib/appsignal/integrations/rake.rb b/lib/appsignal/integrations/rake.rb
@@ -33,11 +33,7 @@ def execute(*args)
       private
 
       def _appsignal_create_transaction
-        Appsignal::Transaction.create(
-          SecureRandom.uuid,
-          Appsignal::Transaction::BACKGROUND_JOB,
-          Appsignal::Transaction::InternalGenericRequest.new({})
-        )
+        Appsignal::Transaction.create(Appsignal::Transaction::BACKGROUND_JOB)
       end
     end
 

diff --git a/lib/appsignal/integrations/resque.rb b/lib/appsignal/integrations/resque.rb
@@ -5,11 +5,7 @@ module Integrations
     # @api private
     module ResqueIntegration
       def perform
-        transaction = Appsignal::Transaction.create(
-          SecureRandom.uuid,
-          Appsignal::Transaction::BACKGROUND_JOB,
-          Appsignal::Transaction::InternalGenericRequest.new({})
-        )
+        transaction = Appsignal::Transaction.create(Appsignal::Transaction::BACKGROUND_JOB)
 
         Appsignal.instrument "perform.resque" do
           super

diff --git a/lib/appsignal/integrations/shoryuken.rb b/lib/appsignal/integrations/shoryuken.rb
@@ -5,11 +5,7 @@ module Integrations
     # @api private
     class ShoryukenMiddleware
       def call(worker_instance, queue, sqs_msg, body, &block)
-        transaction = Appsignal::Transaction.create(
-          SecureRandom.uuid,
-          Appsignal::Transaction::BACKGROUND_JOB,
-          Appsignal::Transaction::InternalGenericRequest.new({})
-        )
+        transaction = Appsignal::Transaction.create(Appsignal::Transaction::BACKGROUND_JOB)
 
         Appsignal.instrument("perform_job.shoryuken", &block)
       rescue Exception => error # rubocop:disable Lint/RescueException

diff --git a/lib/appsignal/integrations/sidekiq.rb b/lib/appsignal/integrations/sidekiq.rb
@@ -38,12 +38,7 @@ def call(exception, sidekiq_context, _sidekiq_config = nil)
           # Sidekiq error outside of the middleware scope.
           # Can be a job JSON parse error or some other error happening in
           # Sidekiq.
-          transaction =
-            Appsignal::Transaction.create(
-              SecureRandom.uuid, # Newly generated job id
-              Appsignal::Transaction::BACKGROUND_JOB,
-              Appsignal::Transaction::InternalGenericRequest.new({})
-            )
+          transaction = Appsignal::Transaction.create(Appsignal::Transaction::BACKGROUND_JOB)
           transaction.set_action_if_nil("SidekiqInternal")
           transaction.set_metadata("sidekiq_error", sidekiq_context[:context])
           transaction.set_params_if_nil(:jobstr => sidekiq_context[:jobstr])
@@ -65,11 +60,7 @@ class SidekiqMiddleware
 
       def call(_worker, item, _queue, &block)
         job_status = nil
-        transaction = Appsignal::Transaction.create(
-          SecureRandom.uuid,
-          Appsignal::Transaction::BACKGROUND_JOB,
-          Appsignal::Transaction::InternalGenericRequest.new({})
-        )
+        transaction = Appsignal::Transaction.create(Appsignal::Transaction::BACKGROUND_JOB)
         transaction.set_action_if_nil(formatted_action_name(item))
 
         formatted_metadata(item).each do |key, value|

diff --git a/lib/appsignal/integrations/webmachine.rb b/lib/appsignal/integrations/webmachine.rb
@@ -10,11 +10,7 @@ def run
           if has_parent_transaction
             Appsignal::Transaction.current
           else
-            Appsignal::Transaction.create(
-              SecureRandom.uuid,
-              Appsignal::Transaction::HTTP_REQUEST,
-              Appsignal::Transaction::InternalGenericRequest.new({})
-            )
+            Appsignal::Transaction.create(Appsignal::Transaction::HTTP_REQUEST)
           end
 
         Appsignal.instrument("process_action.webmachine") do

diff --git a/lib/appsignal/rack/abstract_middleware.rb b/lib/appsignal/rack/abstract_middleware.rb
@@ -34,11 +34,7 @@ def call(env)
             if wrapped_instrumentation
               env[Appsignal::Rack::APPSIGNAL_TRANSACTION]
             else
-              Appsignal::Transaction.create(
-                SecureRandom.uuid,
-                Appsignal::Transaction::HTTP_REQUEST,
-                Appsignal::Transaction::InternalGenericRequest.new({})
-              )
+              Appsignal::Transaction.create(Appsignal::Transaction::HTTP_REQUEST)
             end
 
           unless wrapped_instrumentation

diff --git a/lib/appsignal/rack/event_handler.rb b/lib/appsignal/rack/event_handler.rb
@@ -61,11 +61,7 @@ def on_start(request, _response)
           request.env[APPSIGNAL_EVENT_HANDLER_ID] ||= id
           return unless request_handler?(request.env[APPSIGNAL_EVENT_HANDLER_ID])
 
-          transaction = Appsignal::Transaction.create(
-            SecureRandom.uuid,
-            Appsignal::Transaction::HTTP_REQUEST,
-            Appsignal::Transaction::InternalGenericRequest.new({})
-          )
+          transaction = Appsignal::Transaction.create(Appsignal::Transaction::HTTP_REQUEST)
           request.env[APPSIGNAL_TRANSACTION] = transaction
 
           request.env[RACK_AFTER_REPLY] ||= []

diff --git a/lib/appsignal/transaction.rb b/lib/appsignal/transaction.rb
@@ -24,15 +24,57 @@ class Transaction
     class << self
       # Create a new transaction and set it as the currently active
       # transaction.
-      def create(id, namespace, request, options = {})
+      def create(id_or_namespace, arg_namespace = nil, request = nil, options = {})
+        if arg_namespace
+          id = id_or_namespace
+          namespace = arg_namespace
+        else
+          id = SecureRandom.uuid
+          namespace = id_or_namespace
+        end
+
+        if id
+          Appsignal.internal_logger.warn(
+            "Appsignal::Transaction.create: " \
+              "A new Transaction is created using the transaction ID argument. " \
+              "This argument is deprecated without replacement."
+          )
+        end
+        if arg_namespace
+          Appsignal.internal_logger.warn(
+            "Appsignal::Transaction.create: " \
+              "A Transaction is created using the namespace argument. " \
+              "Specify the namespace as the first argument to the 'create' " \
+              "method without the ID argument."
+          )
+        end
+        if request
+          Appsignal.internal_logger.warn(
+            "Appsignal::Transaction.create: " \
+              "A Transaction is created using the request argument. " \
+              "This argument is deprecated. Please use the `Appsignal.set_*` helpers instead."
+          )
+        end
         # Allow middleware to force a new transaction
-        Thread.current[:appsignal_transaction] = nil if options.include?(:force) && options[:force]
+        if options[:force]
+          Appsignal.internal_logger.warn(
+            "Appsignal::Transaction.create: " \
+              "A Transaction is created using the `:force => true` option argument. " \
+              "The options argument is deprecated without replacement."
+          )
+          Thread.current[:appsignal_transaction] = nil
+        end
 
         # 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(id, namespace, request, options)
+            Appsignal::Transaction.new(
+              id,
+              namespace,
+              request,
+              options
+            )
         else
           # Otherwise, log the issue about trying to start another transaction
           Appsignal.internal_logger.warn(
@@ -87,11 +129,15 @@ def clear_current_transaction!
     attr_reader :ext, :transaction_id, :action, :namespace, :request, :paused, :tags, :options,
       :breadcrumbs, :custom_data
 
-    def initialize(transaction_id, namespace, request, options = {})
+    # Use {.create} to create new transactions.
+    #
+    # @see create
+    # @api private
+    def initialize(transaction_id, namespace, request = nil, options = {})
       @transaction_id = transaction_id
       @action = nil
       @namespace = namespace
-      @request = request
+      @request = request || InternalGenericRequest.new({})
       @paused = false
       @discarded = false
       @tags = {}

diff --git a/spec/lib/appsignal/hooks/action_cable_spec.rb b/spec/lib/appsignal/hooks/action_cable_spec.rb
@@ -302,11 +302,6 @@ def self.to_s
           if DependencyHelper.rails6_present?
             context "with ConnectionStub" do
               let(:connection) { ActionCable::Channel::ConnectionStub.new }
-              let(:transaction_id) { "Stubbed transaction id" }
-              before do
-                # Stub future (private AppSignal) transaction id generated by the hook.
-                expect(SecureRandom).to receive(:uuid).and_return(transaction_id)
-              end
 
               it "does not fail on missing `#env` method on `ConnectionStub`" do
                 instance.unsubscribe_from_channel