Add request start and end time to request and response events #673
Conversation
|
Hi @Just-Sieb, thanks for the PR! This looks peaceful to me, but assigning to @rattrayalex-stripe for review. |
| expect(request.path).to.equal('/v1/charges'); | ||
| expect(request.request_start_time).to.be.within( | ||
| Date.now() - 30000, | ||
| Date.now() |
There was a problem hiding this comment.
Just a note that in general, I don't like tests that rely on a real clock and prefer using dependency injection or even mocks/stubs to make the test more deterministic... Not sure if we have an established pattern for doing this in stripe-node. We can probably keep this as-is and revisit if the test ever becomes flaky.
There was a problem hiding this comment.
I concur with you on your sentiments on tests like this. I went with this approach because I saw this pattern used for testing the elapsed time so I figured it was what Stripe expects/prefers. Perhaps just a type check of these values to ensure they are a number would be a better option?
There was a problem hiding this comment.
While I generally also dislike relying on the real clock, I don't think we have delightful mocking available at this time.
That said, we can probably set the starting bound to be a Date.now() set at the top of the test, which should be both more accurate and robust. Similarly, below, in an ideal world we'd check that response.request_end_time is greater than request.request_start_time (which would require adding an onRequest handler).
There was a problem hiding this comment.
Good suggestions @rattrayalex-stripe, I went ahead and added the request_start_time to the response event as well as that lets me more easily implement your request_end_time greater that request_start_time test as well as improve the elapsed test to not be vulnerable to a bizarre long request. Plus I suspect for some people's usage it will be beneficial for adding logging of the start and end time of any Stripe request. I also made the set lower bound at start of test change. This should be ready for you to review again. Thanks!
There was a problem hiding this comment.
Great thinking! Thanks!
There was a problem hiding this comment.
Few small changes requested in the tests, otherwise this looks good! Thanks for taking the time to share this with other users, @Just-Sieb !
Just-Sieb
force-pushed
the
jds/event-time
branch
from
00e4757
to
851f4ae
Compare
test/flows.spec.js
Outdated
| response.request_end_time - response.request_start_time | ||
| ); | ||
| expect(response.request_end_time).to.be.within( | ||
| lowerBoundStartTime, |
There was a problem hiding this comment.
sorry, tiny nit: probably a bit cleaner to use response.request_start_time here and then remove the expect below.
There was a problem hiding this comment.
I went ahead and updated. This should be good for you to merge now.
| @@ -379,7 +383,16 @@ describe('Flows', function() { | |||
| expect(response.path).to.equal('/v1/charges'); | |||
| expect(response.request_id).to.match(/req_[\w\d]/); | |||
| expect(response.status).to.equal(402); | |||
| expect(response.elapsed).to.be.within(50, 30000); | |||
| expect(response.elapsed).to.equal( | |||
There was a problem hiding this comment.
Left a tiny nit to clean up the test slightly, but not a requirement to enact it, so approving the PR.
I'll leave this un-merged for a few hours in case you feel like addressing it, and otherwise merge later this afternoon.
Thanks for the contribution, @Just-Sieb !
Just-Sieb
force-pushed
the
jds/event-time
branch
from
851f4ae
to
67b89a9
Compare
I am working on setting up distributed tracing for work and I am wanting to create spans on our Stripe calls. This change makes it easier to get accurate start and end times for the spans. Plus I think this addition should be beneficial for other people as well.