From 1e6d0b315577176d4dd37db0a8f5fde89c66e8a4 Mon Sep 17 00:00:00 2001
From: Tom de Bruijn <tom@tomdebruijn.com>
Date: Mon, 8 Jul 2024 13:50:43 +0200
Subject: [PATCH] Deprecate the request env keys to set the action (#1155)

Configuring the action name with the request env keys `appsignal.action`
and `appsignal.route` don't give a reasonable guarantee when the action
name is set. The `Appsignal.set_action` helper sets the action name
immediately.

The method via the request env sets the action name using
`set_action_if_nil`. If the action name is set by one of our
instrumentations beforehand or another `Appsiganl.set_action` call,
there's no guarantee this value is used as the action name.

We also never documented either of these keys, so I don't expect a lot
of people to be using them.

Closes #1116
---
 ...-route-and-appsignal-action-request-env.md | 15 ++++++
 lib/appsignal/rack/abstract_middleware.rb     | 22 ++++++++-
 .../rack/abstract_middleware_spec.rb          | 46 +++++++++++++++++--
 3 files changed, 79 insertions(+), 4 deletions(-)
 create mode 100644 .changesets/deprecate-appsignal-route-and-appsignal-action-request-env.md

diff --git a/.changesets/deprecate-appsignal-route-and-appsignal-action-request-env.md b/.changesets/deprecate-appsignal-route-and-appsignal-action-request-env.md
new file mode 100644
index 000000000..192b9a667
--- /dev/null
+++ b/.changesets/deprecate-appsignal-route-and-appsignal-action-request-env.md
@@ -0,0 +1,15 @@
+---
+bump: patch
+type: deprecate
+---
+
+Deprecate the `appsignal.action` and `appsignal.route` request env methods to set the transaction action name. Use the `Appsignal.set_action` helper instead.
+
+```ruby
+# Before
+env["appsignal.action"] = "POST /my-action"
+env["appsignal.route"] = "POST /my-action"
+
+# After
+Appsignal.set_action("POST /my-action")
+```
diff --git a/lib/appsignal/rack/abstract_middleware.rb b/lib/appsignal/rack/abstract_middleware.rb
index 478cd138e..26d976cb3 100644
--- a/lib/appsignal/rack/abstract_middleware.rb
+++ b/lib/appsignal/rack/abstract_middleware.rb
@@ -127,7 +127,7 @@ def add_transaction_metadata_before(transaction, request)
       # Call `super` to also include the default set metadata.
       def add_transaction_metadata_after(transaction, request)
         default_action =
-          request.env["appsignal.route"] || request.env["appsignal.action"]
+          appsignal_route_env_value(request) || appsignal_action_env_value(request)
         transaction.set_action_if_nil(default_action)
         transaction.set_metadata("path", request.path)
 
@@ -160,6 +160,26 @@ def request_method_for(request)
       def request_for(env)
         @request_class.new(env)
       end
+
+      def appsignal_route_env_value(request)
+        request.env["appsignal.route"].tap do |value|
+          next unless value
+
+          Appsignal::Utils::StdoutAndLoggerMessage.warning \
+            "Setting the action name with the request env 'appsignal.route' is deprecated. " \
+              "Please use `Appsignal.set_action` instead. "
+        end
+      end
+
+      def appsignal_action_env_value(request)
+        request.env["appsignal.action"].tap do |value|
+          next unless value
+
+          Appsignal::Utils::StdoutAndLoggerMessage.warning \
+            "Setting the action name with the request env 'appsignal.action' is deprecated. " \
+              "Please use `Appsignal.set_action` instead. "
+        end
+      end
     end
   end
 end
diff --git a/spec/lib/appsignal/rack/abstract_middleware_spec.rb b/spec/lib/appsignal/rack/abstract_middleware_spec.rb
index 69bfbf5f8..523591e53 100644
--- a/spec/lib/appsignal/rack/abstract_middleware_spec.rb
+++ b/spec/lib/appsignal/rack/abstract_middleware_spec.rb
@@ -160,21 +160,61 @@ def make_request_with_error(error_class, error_message)
       end
 
       context "with appsignal.route env" do
+        before { env["appsignal.route"] = "POST /my-route" }
+
         it "reports the appsignal.route value as the action name" do
-          env["appsignal.route"] = "POST /my-route"
           make_request
 
           expect(last_transaction).to have_action("POST /my-route")
         end
+
+        it "prints a deprecation warning" do
+          err_stream = std_stream
+          capture_std_streams(std_stream, err_stream) do
+            make_request
+          end
+
+          expect(err_stream.read).to include(
+            "Setting the action name with the request env 'appsignal.route' is deprecated."
+          )
+        end
+
+        it "logs a deprecation warning" do
+          logs = capture_logs { make_request }
+          expect(logs).to contains_log(
+            :warn,
+            "Setting the action name with the request env 'appsignal.route' is deprecated."
+          )
+        end
       end
 
       context "with appsignal.action env" do
-        it "reports the appsignal.route value as the action name" do
-          env["appsignal.action"] = "POST /my-action"
+        before { env["appsignal.action"] = "POST /my-action" }
+
+        it "reports the appsignal.action value as the action name" do
           make_request
 
           expect(last_transaction).to have_action("POST /my-action")
         end
+
+        it "prints a deprecation warning" do
+          err_stream = std_stream
+          capture_std_streams(std_stream, err_stream) do
+            make_request
+          end
+
+          expect(err_stream.read).to include(
+            "Setting the action name with the request env 'appsignal.action' is deprecated."
+          )
+        end
+
+        it "logs a deprecation warning" do
+          logs = capture_logs { make_request }
+          expect(logs).to contains_log(
+            :warn,
+            "Setting the action name with the request env 'appsignal.action' is deprecated."
+          )
+        end
       end
 
       describe "request metadata" do