Add Stripe instrumentation #2557
Conversation
agrobbin
force-pushed
the
stripe-instrumentation
branch
6 times, most recently
from
9cfac1f to
7737a98
Compare
| end | ||
|
|
||
| def tag_span(span, event) | ||
| span.resource = event.object_name if event.respond_to?(:object_name) && event.object_name |
There was a problem hiding this comment.
This particular resource configuration is dependent upon stripe/stripe-ruby#1168, which exposes object_name. I'm happy to remove this while that PR is considered if it's preferred!
There was a problem hiding this comment.
I think it would be great if we could derive resource from Stripe's api resource (with your Stripe's PR).
However, I feel like having stripe.customer instead of customer would be an improvement (currently, without an object name, it is stripe.request). What do you think?
There was a problem hiding this comment.
Yep that makes a lot of sense, @TonyCTHsu. I've updated this to be "stripe.#{event.object_name}".
I have not yet heard back from Stripe on that PR, so I'm curious what you suggest doing while we wait. I'm happy to remove this to unblock the rest of this instrumentation. Then I can open a new PR here once stripe/stripe-ruby#1168 (hopefully) gets merged.
There was a problem hiding this comment.
I think we can merge this with a code comment referencing the Stripe's PR. What bothers me more would be a missing spec to test this condition.
There was a problem hiding this comment.
@TonyCTHsu makes sense! I've just pushed up a test that I think best exercises this. I can keep an eye on stripe/stripe-ruby#1168, and update as appropriate based on what happens with that.
agrobbin
force-pushed
the
stripe-instrumentation
branch
3 times, most recently
from
c0aef06 to
56c4455
Compare
|
👋 @agrobbin , Thanks for your awesome contribution! I was not particularly familiar with this gem, but at first glance, it seems fine! Let me spend some time to dig around with the gem to provide proper review. One thing I would suggest is that, since it is supporting
|
agrobbin
force-pushed
the
stripe-instrumentation
branch
from
56c4455 to
41e9d45
Compare
|
@TonyCTHsu I just updated this to test against various |
| tag_span(span, event) | ||
|
|
||
| # Finish the span | ||
| span.finish |
There was a problem hiding this comment.
I think span.finish needs to be in an ensure block. If tag_span raises an error, the span will go unfinished and will not be visible.
There was a problem hiding this comment.
tag_span has a rescue StandardError @delner, but I see the pattern you're referring to elsewhere in the library, so I've updated this to match!
agrobbin
force-pushed
the
stripe-instrumentation
branch
3 times, most recently
from
be53688 to
bd2a75d
Compare
Codecov Report
@@ Coverage Diff @@
## master #2557 +/- ##
========================================
Coverage 98.03% 98.04%
========================================
Files 1127 1135 +8
Lines 60859 61066 +207
Branches 2772 2776 +4
========================================
+ Hits 59661 59870 +209
+ Misses 1198 1196 -2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| end | ||
|
|
||
| def tag_span(span, event) | ||
| span.resource = event.object_name if event.respond_to?(:object_name) && event.object_name |
There was a problem hiding this comment.
I think it would be great if we could derive resource from Stripe's api resource (with your Stripe's PR).
However, I feel like having stripe.customer instead of customer would be an improvement (currently, without an object name, it is stripe.request). What do you think?
agrobbin
force-pushed
the
stripe-instrumentation
branch
2 times, most recently
from
7615e60 to
5be411f
Compare
agrobbin
force-pushed
the
stripe-instrumentation
branch
from
5be411f to
1a6263b
Compare
What does this PR do?
This adds instrumentation of the stripe-ruby gem.
Motivation
While using the Stripe gem, I thought it would be really helpful to have spans show up in DataDog APM whenever we interact with Stripe!