Stripe instrumentation

[hannahramadan]

Adds instrumentation for Stripe 5.38.0+. Makes use of Stripe's pub/sub instrumentation pattern.

Stripe users can attach custom attributes to Stripe events via the user_data hash created on a Stripe request event initialization. No attributes are collected by default. Two new config options allow customers to fine-tune reported attributes: stripe.user_data.include and stripe.user_data.exclude

closes #2057

[fallwith]

I think it would be better to move these 2 lines from 2 separate methods that need not know about each other out into their own method that deletes and returns the segment:

event.user_data.delete(:newrelic_segment)

segment = event.user_data[:newrelic_segment]

[hannahramadan]

Made some changes. Let me know if this works c59f04c

[fallwith]

Should the metric name elements be slash delimited?

Suggested change

	"Stripe#{event.path} #{event.method}"
	"Stripe#{event.path}/#{event.method}"

[hannahramadan]

I've updated this. c59f04c

[fallwith]

Seeing as the begin wraps the entire method body, there's no need for it, and you can simply remove the begin and end and outdent the ensure one level (to be on a peer level with def and rescue).

[fallwith]

It is possible to reach the ensure block with a nil segment, and calling #finish on nil will raise another error which won't be handled. We can use the safe navigation operator to proactively anticipate and defend against this possibility.

Suggested change

	segment.finish
	segment&.finish

[hannahramadan]

Great call c59f04c

[fallwith]

It doesn't really matter given that the tests don't operate on the url contained in the response, but from a "the specs should tell the story" perspective, perhaps you should store 'v1/customers' as a constant and use it both here and in the #detect calls that the tests perform.

[hannahramadan]

updated 63014dd

[fallwith]

Seeing as you use this wrapper multiple times, it'd be a good candidate for a test helper.

def with_stubbed_connection_manager(&block)
  Stripe::StripeClient.stub(:default_connection_manager, @connection) do
    @connection.stub(:execute_request, @response) do
      block.call
    end
  end
end

then for each test you could use it:

with_stubbed_connection_manager do
  in_transaction do |txn|
    Stripe::Customer.list({limit: 3})
    stripe_segment = txn.segments.detect { |s| s.name == 'Stripe/v1/customers get' }

    assert(stripe_segment)
  end
end

[hannahramadan]

Pattern into a method.. love and done! 63014dd

[fallwith]

I could see you doing something like:

stripe_segment = stripe_segment_from_txn(txn)

for this repeated pattern.

[hannahramadan]

Love this. Kayla had a similar suggestion to put repeated patterns into methods. I've updated 63014dd

[fallwith]

There's always a single "dummy" segment when you use in_transaction. You should be able to do:

refute stripe_segment

but you don't want the transaction's segment array itself to be empty, as it'll always have at least 1 entry.

[hannahramadan]

That makes sense. I've updated what we're looking for 63014dd

[kaylareopelle]

This is so cool, Hannah! Awesome work. I found a few small things for you to consider before approval.

[kaylareopelle]

Can metric names have spaces in them? (I have no idea, but it struck me as odd)

[hannahramadan]

Both you and James called this out. I've removed the space c59f04c

[kaylareopelle]

I think you want this to be stripe?

[hannahramadan]

Because pub/sub, I've removed this!

[kaylareopelle]

Does this need Gem::Version.new to compare the values?

[hannahramadan]

We were running fine without it, but I've definitely run into issues with this in the past. To be safe and match instrumentation, I've updated c997c51

[kaylareopelle]

Could you add the start a Stripe event comment here too? I loved seeing it on the later tests.

[kaylareopelle]

Alternatively, you could name a method start_stripe_event and have it call Stripe::Customer.list({limit: 3}), if the event is started the same way in all the tests.

[hannahramadan]

Great suggestion! 63014dd

[github-actions]

SimpleCov Report

	Coverage	Threshold
Line	94.29%	94%
Branch	85.83%	85%