Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve request events instrumentation #1162

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 45 additions & 7 deletions lib/stripe/instrumentation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,26 +33,64 @@ class RequestEndEvent
attr_reader :num_retries
attr_reader :path
attr_reader :request_id
attr_reader :response_header
attr_reader :response_body
attr_reader :request_header
attr_reader :request_body

# Arbitrary user-provided data in the form of a Ruby hash that's passed
# from subscribers on `request_begin` to subscribers on `request_end`.
# `request_begin` subscribers can set keys which will then be available
# in `request_end`.
attr_reader :user_data

def initialize(duration:, http_status:, method:, num_retries:, path:,
request_id:, user_data: nil)
@duration = duration
@http_status = http_status
@method = method
def initialize(request_context:, response_context:,
num_retries:, user_data: nil)
@duration = request_context.duration
@http_status = response_context.http_status
@method = request_context.method
@num_retries = num_retries
@path = path
@request_id = request_id
@path = request_context.path
@request_id = request_context.request_id
@user_data = user_data
@response_header = response_context.header
@response_body = response_context.body
@request_header = request_context.header
@request_body = request_context.body
freeze
end
end

class RequestContext
attr_reader :duration
attr_reader :method
attr_reader :path
attr_reader :request_id
attr_reader :body
attr_reader :header

def initialize(duration:, context:, header:)
@duration = duration
@method = context.method
@path = context.path
@request_id = context.request_id
@body = context.body
@header = header
end
end

class ResponseContext
attr_reader :http_status
attr_reader :body
attr_reader :header

def initialize(http_status:, response:)
@http_status = http_status
@header = response ? response.to_hash : nil
@body = response ? response.body : nil
end
end

# This class was renamed for consistency. This alias is here for backwards
# compatibility.
RequestEvent = RequestEndEvent
Expand Down
42 changes: 25 additions & 17 deletions lib/stripe/stripe_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -494,15 +494,16 @@ def self.maybe_gc_connection_managers
end
end

http_resp = execute_request_with_rescues(method, api_base, context) do
self.class
.default_connection_manager(config)
.execute_request(method, url,
body: body,
headers: headers,
query: query,
&response_block)
end
http_resp =
execute_request_with_rescues(method, api_base, headers, context) do
self.class
.default_connection_manager(config)
.execute_request(method, url,
body: body,
headers: headers,
query: query,
&response_block)
end

[http_resp, api_key]
end
Expand Down Expand Up @@ -564,7 +565,7 @@ def self.maybe_gc_connection_managers
http_status >= 400
end

private def execute_request_with_rescues(method, api_base, context)
private def execute_request_with_rescues(method, api_base, headers, context)
num_retries = 0

begin
Expand All @@ -587,7 +588,7 @@ def self.maybe_gc_connection_managers

log_response(context, request_start, http_status, resp.body, resp)
notify_request_end(context, request_duration, http_status,
num_retries, user_data)
num_retries, user_data, resp, headers)

if config.enable_telemetry? && context.request_id
request_duration_ms = (request_duration * 1000).to_i
Expand All @@ -614,7 +615,7 @@ def self.maybe_gc_connection_managers
log_response_error(error_context, request_start, e)
end
notify_request_end(context, request_duration, http_status, num_retries,
user_data)
user_data, resp, headers)

if self.class.should_retry?(e,
method: method,
Expand Down Expand Up @@ -657,17 +658,24 @@ def self.maybe_gc_connection_managers
end

private def notify_request_end(context, duration, http_status, num_retries,
user_data)
user_data, resp, headers)
return if !Instrumentation.any_subscribers?(:request_end) &&
!Instrumentation.any_subscribers?(:request)

event = Instrumentation::RequestEndEvent.new(
request_context = Stripe::Instrumentation::RequestContext.new(
duration: duration,
context: context,
header: headers
)
response_context = Stripe::Instrumentation::ResponseContext.new(
http_status: http_status,
method: context.method,
response: resp
)

anniel-stripe marked this conversation as resolved.
Show resolved Hide resolved
event = Instrumentation::RequestEndEvent.new(
request_context: request_context,
response_context: response_context,
num_retries: num_retries,
path: context.path,
request_id: context.request_id,
user_data: user_data || {}
)
Stripe::Instrumentation.notify(:request_end, event)
Expand Down
22 changes: 19 additions & 3 deletions test/stripe/instrumentation_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,29 @@ class InstrumentationTest < Test::Unit::TestCase

context "RequestEventEnd" do
should "return a frozen object" do
event = Stripe::Instrumentation::RequestEndEvent.new(
mock_context = stub(
duration: 0.1,
http_status: 200,
method: :get,
num_retries: 0,
path: "/v1/test",
request_id: "req_123",
body: ""
)

request_context = Stripe::Instrumentation::RequestContext.new(
duration: 0.1,
context: mock_context,
header: nil
)

response_context = Stripe::Instrumentation::ResponseContext.new(
http_status: 200,
response: nil
)

event = Stripe::Instrumentation::RequestEndEvent.new(
num_retries: 0,
request_context: request_context,
response_context: response_context,
user_data: nil
)

Expand Down
20 changes: 15 additions & 5 deletions test/stripe/stripe_client_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1443,19 +1443,25 @@ class StripeClientTest < Test::Unit::TestCase
should "notify a subscriber of a successful HTTP request" do
events = []
Stripe::Instrumentation.subscribe(:request_end, :test) { |event| events << event }
stub_response_body = JSON.generate(object: "charge")

stub_request(:get, "#{Stripe.api_base}/v1/charges")
.to_return(body: JSON.generate(object: "charge"), headers: { "Request-ID": "req_123" })
Stripe::Charge.list
stub_request(:post, "#{Stripe.api_base}/v1/charges")
.with(body: { "amount" => "50", "currency" => "usd", "card" => "sc_token" })
.to_return(body: stub_response_body, headers: { "Request-ID": "req_123" })

Stripe::Charge.create(amount: 50, currency: "usd", card: "sc_token")

assert_equal(1, events.size)
event = events.first
assert_equal(:get, event.method)
assert_equal(:post, event.method)
assert_equal("/v1/charges", event.path)
assert_equal(200, event.http_status)
assert(event.duration.positive?)
assert_equal(0, event.num_retries)
assert_equal("req_123", event.request_id)
assert_equal(stub_response_body, event.response_body)
assert_equal("amount=50&currency=usd&card=sc_token", event.request_body)
anniel-stripe marked this conversation as resolved.
Show resolved Hide resolved
assert(event.request_header)
end

should "notify a subscriber of a StripeError" do
Expand Down Expand Up @@ -1484,6 +1490,7 @@ class StripeClientTest < Test::Unit::TestCase
assert_equal(500, event.http_status)
assert(event.duration.positive?)
assert_equal(0, event.num_retries)
assert_equal(JSON.generate(error: error), event.response_body)
end

should "notify a subscriber of a network error" do
Expand Down Expand Up @@ -1526,8 +1533,9 @@ class StripeClientTest < Test::Unit::TestCase
events = []
Stripe::Instrumentation.subscribe(:request, :test) { |event| events << event }

stub_response_body = JSON.generate(object: "charge")
stub_request(:get, "#{Stripe.api_base}/v1/charges")
.to_return(body: JSON.generate(object: "charge"))
.to_return(body: stub_response_body)
Stripe::Charge.list

assert_equal(1, events.size)
Expand All @@ -1537,6 +1545,8 @@ class StripeClientTest < Test::Unit::TestCase
assert_equal(200, event.http_status)
assert(event.duration.positive?)
assert_equal(0, event.num_retries)
assert_equal(stub_response_body, event.response_body)
assert(event.request_header)
end
end
end
Expand Down