From 047ac78c68c97ddd21550d66d5652556f9488328 Mon Sep 17 00:00:00 2001 From: Alex Robbin Date: Fri, 13 Jan 2023 19:49:06 -0500 Subject: [PATCH] add the Stripe object name to instrumentation event payloads Having the object name in event payloads can be really helpful when sending Stripe-related information to an APM tool (rather than, say, the request path). --- README.md | 3 +++ lib/stripe/api_operations/request.rb | 3 +++ lib/stripe/instrumentation.rb | 8 ++++++- lib/stripe/stripe_client.rb | 21 ++++++++++++---- test/stripe/instrumentation_test.rb | 2 ++ test/stripe/stripe_client_test.rb | 36 +++++++++++++++++++++++++--- 6 files changed, 64 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index bf953f45a..a4fec8526 100644 --- a/README.md +++ b/README.md @@ -231,6 +231,7 @@ The library has various hooks that user code can tie into by passing a block to Invoked when an HTTP request starts. Receives `RequestBeginEvent` with the following properties: +- `object_name`: The Stripe object, if applicable. (`String`) - `method`: HTTP method. (`Symbol`) - `path`: Request path. (`String`) - `user_data`: A hash on which users can set arbitrary data, and which will be @@ -245,6 +246,7 @@ following properties: Invoked when an HTTP request finishes, regardless of whether it terminated with a success or error. Receives `RequestEndEvent` with the following properties: +- `object_name`: The Stripe object, if applicable. (`String`) - `duration`: Request duration in seconds. (`Float`) - `http_status`: HTTP response code (`Integer`) if available, or `nil` in case of a lower level network error. @@ -270,6 +272,7 @@ Stripe::Instrumentation.subscribe(:request_end) do |request_event| resource = path_parts.map { |part| part.match?(/\A[a-z_]+\z/) ? part : ":id" }.join("/") tags = { + object_name: request_event.object_name, method: request_event.method, resource: resource, code: request_event.http_status, diff --git a/lib/stripe/api_operations/request.rb b/lib/stripe/api_operations/request.rb index 8544dedd6..b1b348ee2 100644 --- a/lib/stripe/api_operations/request.rb +++ b/lib/stripe/api_operations/request.rb @@ -43,6 +43,8 @@ def execute_resource_request_stream(method, url, opts[:client] ||= StripeClient.active_client + object_name = self::OBJECT_NAME if defined?(self::OBJECT_NAME) + headers = opts.clone api_key = headers.delete(:api_key) api_base = headers.delete(:api_base) @@ -52,6 +54,7 @@ def execute_resource_request_stream(method, url, resp, opts[:api_key] = client.send( client_request_method_sym, method, url, + object_name: object_name, api_base: api_base, api_key: api_key, headers: headers, params: params, &read_body_chunk_block diff --git a/lib/stripe/instrumentation.rb b/lib/stripe/instrumentation.rb index a30e4c776..87210f5b0 100644 --- a/lib/stripe/instrumentation.rb +++ b/lib/stripe/instrumentation.rb @@ -4,6 +4,7 @@ module Stripe class Instrumentation # Event emitted on `request_begin` callback. class RequestBeginEvent + attr_reader :object_name attr_reader :method attr_reader :path @@ -17,7 +18,8 @@ class RequestBeginEvent # set by other subscribers. attr_reader :user_data - def initialize(method:, path:, user_data:) + def initialize(object_name:, method:, path:, user_data:) + @object_name = object_name @method = method @path = path @user_data = user_data @@ -27,6 +29,7 @@ def initialize(method:, path:, user_data:) # Event emitted on `request_end` callback. class RequestEndEvent + attr_reader :object_name attr_reader :duration attr_reader :http_status attr_reader :method @@ -46,6 +49,7 @@ class RequestEndEvent def initialize(request_context:, response_context:, num_retries:, user_data: nil) + @object_name = request_context.object_name @duration = request_context.duration @http_status = response_context.http_status @method = request_context.method @@ -62,6 +66,7 @@ def initialize(request_context:, response_context:, end class RequestContext + attr_reader :object_name attr_reader :duration attr_reader :method attr_reader :path @@ -70,6 +75,7 @@ class RequestContext attr_reader :header def initialize(duration:, context:, header:) + @object_name = context.object_name @duration = duration @method = context.method @path = context.path diff --git a/lib/stripe/stripe_client.rb b/lib/stripe/stripe_client.rb index 54f06c4c8..f1814e08e 100644 --- a/lib/stripe/stripe_client.rb +++ b/lib/stripe/stripe_client.rb @@ -212,9 +212,10 @@ def request end def execute_request(method, path, + object_name: nil, api_base: nil, api_key: nil, headers: {}, params: {}) http_resp, api_key = execute_request_internal( - method, path, api_base, api_key, headers, params + method, path, object_name, api_base, api_key, headers, params ) begin @@ -242,7 +243,8 @@ def execute_request(method, path, # block is expected to do all the necessary body processing. If no block is # passed, then a StripeStreamResponse is returned containing an IO stream # with the response body. - def execute_request_stream(method, path, + def execute_request_stream(method, path, # rubocop:disable Metrics/ParameterLists + object_name: nil, api_base: nil, api_key: nil, headers: {}, params: {}, &read_body_chunk_block) @@ -252,7 +254,11 @@ def execute_request_stream(method, path, end http_resp, api_key = execute_request_internal( - method, path, api_base, api_key, headers, params, &read_body_chunk_block + method, path, + object_name, + api_base, api_key, + headers, params, + &read_body_chunk_block ) # When the read_body_chunk_block is given, we no longer have access to the @@ -430,8 +436,10 @@ def self.maybe_gc_connection_managers pruned_contexts.count end - private def execute_request_internal(method, path, - api_base, api_key, headers, params, + private def execute_request_internal(method, path, # rubocop:disable Metrics/ParameterLists + object_name, + api_base, api_key, + headers, params, &read_body_chunk_block) raise ArgumentError, "method should be a symbol" \ unless method.is_a?(Symbol) @@ -477,6 +485,7 @@ def self.maybe_gc_connection_managers context.api_version = headers["Stripe-Version"] context.body = body_log context.idempotency_key = headers["Idempotency-Key"] + context.object_name = object_name context.method = method context.path = path context.query = query @@ -646,6 +655,7 @@ def self.maybe_gc_connection_managers return unless Instrumentation.any_subscribers?(:request_begin) event = Instrumentation::RequestBeginEvent.new( + object_name: context.object_name, method: context.method, path: context.path, user_data: {} @@ -963,6 +973,7 @@ def self.maybe_gc_connection_managers # that we can log certain information. It's useful because it means that we # don't have to pass around as many parameters. class RequestLogContext + attr_accessor :object_name attr_accessor :body attr_accessor :account attr_accessor :api_key diff --git a/test/stripe/instrumentation_test.rb b/test/stripe/instrumentation_test.rb index b121b329c..02d4c75c2 100644 --- a/test/stripe/instrumentation_test.rb +++ b/test/stripe/instrumentation_test.rb @@ -47,6 +47,7 @@ class InstrumentationTest < Test::Unit::TestCase context "RequestEventBegin" do should "return a frozen object" do event = Stripe::Instrumentation::RequestBeginEvent.new( + object_name: "customer", method: :get, path: "/v1/test", user_data: nil @@ -59,6 +60,7 @@ class InstrumentationTest < Test::Unit::TestCase context "RequestEventEnd" do should "return a frozen object" do mock_context = stub( + object_name: "customer", duration: 0.1, method: :get, path: "/v1/test", diff --git a/test/stripe/stripe_client_test.rb b/test/stripe/stripe_client_test.rb index 0ec61155b..b3aa5b370 100644 --- a/test/stripe/stripe_client_test.rb +++ b/test/stripe/stripe_client_test.rb @@ -462,6 +462,7 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new client.send(request_method, :post, "/v1/account", + object_name: "account", headers: { "Stripe-Account" => "bar" }, &@read_body_chunk_block) end @@ -473,6 +474,7 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new client.send(request_method, :post, "/v1/account", + object_name: "account", headers: { stripe_account: "bar" }, &@read_body_chunk_block) end @@ -588,6 +590,7 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new client.send(request_method, :post, "/v1/account", + object_name: "account", headers: { "Idempotency-Key" => "abc", "Stripe-Account" => "acct_123", @@ -641,6 +644,7 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new assert_raises Stripe::APIError do client.send(request_method, :post, "/v1/account", + object_name: "account", &@read_body_chunk_block) end end @@ -698,6 +702,7 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new client.send(request_method, :post, "/v1/account", + object_name: "account", &@read_body_chunk_block) ensure Stripe.stripe_account = old @@ -712,6 +717,7 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new client.send(request_method, :post, "/v1/account", + object_name: "account", headers: { stripe_account: stripe_account }, &@read_body_chunk_block) end @@ -724,6 +730,7 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new client.send(request_method, :post, "/v1/account", + object_name: "account", &@read_body_chunk_block) end end @@ -761,6 +768,7 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new client.send(request_method, :post, "/v1/account", + object_name: "account", &@read_body_chunk_block) ensure Stripe.app_info = old @@ -777,6 +785,7 @@ class StripeClientTest < Test::Unit::TestCase e = assert_raises Stripe::APIError do client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end assert_equal 'Invalid response object from API: "" (HTTP response code was 500)', e.message @@ -792,6 +801,7 @@ class StripeClientTest < Test::Unit::TestCase e = assert_raises Stripe::InvalidRequestError do client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end assert_equal("req_123", e.request_id) @@ -805,6 +815,7 @@ class StripeClientTest < Test::Unit::TestCase e = assert_raises Stripe::APIConnectionError do client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end assert_equal StripeClient::ERROR_MESSAGE_CONNECTION % Stripe.api_base + @@ -820,6 +831,7 @@ class StripeClientTest < Test::Unit::TestCase e = assert_raises Stripe::APIError do client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end assert_equal 'Invalid response object from API: "{\"bar\":\"foo\"}" (HTTP response code was 500)', e.message @@ -836,6 +848,7 @@ class StripeClientTest < Test::Unit::TestCase e = assert_raises Stripe::IdempotencyError do client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end assert_equal(400, e.http_status) @@ -850,6 +863,7 @@ class StripeClientTest < Test::Unit::TestCase e = assert_raises Stripe::InvalidRequestError do client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end assert_equal(400, e.http_status) @@ -864,6 +878,7 @@ class StripeClientTest < Test::Unit::TestCase e = assert_raises Stripe::AuthenticationError do client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end assert_equal(401, e.http_status) @@ -878,6 +893,7 @@ class StripeClientTest < Test::Unit::TestCase e = assert_raises Stripe::CardError do client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end assert_equal(402, e.http_status) @@ -894,6 +910,7 @@ class StripeClientTest < Test::Unit::TestCase e = assert_raises Stripe::PermissionError do client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end assert_equal(403, e.http_status) @@ -908,6 +925,7 @@ class StripeClientTest < Test::Unit::TestCase e = assert_raises Stripe::InvalidRequestError do client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end assert_equal(404, e.http_status) @@ -922,6 +940,7 @@ class StripeClientTest < Test::Unit::TestCase e = assert_raises Stripe::RateLimitError do client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end assert_equal(429, e.http_status) @@ -1013,6 +1032,7 @@ class StripeClientTest < Test::Unit::TestCase end.to_return(body: JSON.generate(object: "charge")) client = StripeClient.new client.send(request_method, :get, "/v1/charges/ch_123", + object_name: "charge", &@read_body_chunk_block) end @@ -1023,6 +1043,7 @@ class StripeClientTest < Test::Unit::TestCase .to_return(body: JSON.generate(object: "charge")) client = StripeClient.new client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end @@ -1033,6 +1054,7 @@ class StripeClientTest < Test::Unit::TestCase .to_return(body: JSON.generate(object: "charge")) client = StripeClient.new client.send(request_method, :delete, "/v1/charges/ch_123", + object_name: "charge", &@read_body_chunk_block) end @@ -1049,6 +1071,7 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new client.send(request_method, :post, "/v1/charges", + object_name: "charge", headers: { idempotency_key: "provided_key" }, &@read_body_chunk_block) end @@ -1067,6 +1090,7 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new err = assert_raises Stripe::APIConnectionError do client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end assert_match(/Request was retried 2 times/, err.message) @@ -1088,6 +1112,7 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end @@ -1103,6 +1128,7 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new assert_raises Stripe::APIConnectionError do client.send(request_method, :post, "/v1/charges", + object_name: "charge", &@read_body_chunk_block) end end @@ -1112,6 +1138,7 @@ class StripeClientTest < Test::Unit::TestCase should "allows empty strings in params" do client = StripeClient.new client.send(request_method, :get, "/v1/invoices/upcoming", + object_name: "invoice", params: { customer: "cus_123", coupon: "" }, &@read_body_chunk_block) assert_requested( @@ -1127,6 +1154,7 @@ class StripeClientTest < Test::Unit::TestCase should "filter nils in params" do client = StripeClient.new client.send(request_method, :get, "/v1/invoices/upcoming", + object_name: "invoice", params: { customer: "cus_123", coupon: nil }, &@read_body_chunk_block) assert_requested( @@ -1141,6 +1169,7 @@ class StripeClientTest < Test::Unit::TestCase should "merge query parameters in URL and params" do client = StripeClient.new client.send(request_method, :get, "/v1/invoices/upcoming?coupon=25OFF", + object_name: "invoice", params: { customer: "cus_123" }, &@read_body_chunk_block) assert_requested( @@ -1156,6 +1185,7 @@ class StripeClientTest < Test::Unit::TestCase should "prefer query parameters in params when specified in URL as well" do client = StripeClient.new client.send(request_method, :get, "/v1/invoices/upcoming?customer=cus_query", + object_name: "invoice", params: { customer: "cus_param" }, &@read_body_chunk_block) assert_requested( @@ -1178,7 +1208,7 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new e = assert_raises Stripe::APIError do - client.execute_request(:post, "/v1/charges") + client.execute_request(:post, "/v1/charges", object_name: "charge") end assert_equal 'Invalid response object from API: "" (HTTP response code was 200)', e.message end @@ -1189,7 +1219,7 @@ class StripeClientTest < Test::Unit::TestCase client = StripeClient.new e = assert_raises ArgumentError do - client.execute_request_stream(:post, "/v1/charges") + client.execute_request_stream(:post, "/v1/charges", object_name: "charge") end assert_equal "execute_request_stream requires a read_body_chunk_block", e.message end @@ -1202,7 +1232,7 @@ class StripeClientTest < Test::Unit::TestCase accumulated_body = +"" - resp, = client.execute_request_stream(:post, "/v1/charges") do |body_chunk| + resp, = client.execute_request_stream(:post, "/v1/charges", object_name: "charge") do |body_chunk| accumulated_body << body_chunk end