Add Stripe client telemetry to request headers #557
Conversation
lib/StripeResource.js
Outdated
| @@ -125,6 +125,8 @@ StripeResource.prototype = { | |||
| // lastResponse. | |||
| res.requestId = headers['request-id']; | |||
|
|
|||
| var requestDuration = Date.now() - req._requestStart; | |||
There was a problem hiding this comment.
could you make this variable include the units in the name? (e.g. suffix it with "ms")?
lib/stripe.js
Outdated
| this._enableTelemetry = enableTelemetry; | ||
| }, | ||
|
|
||
| telemetryEnabled: function() { |
There was a problem hiding this comment.
It looks like the convention in the rest of this file is to use get/set prefixes on the function names, so I'd change this to getTelemetryEnabled.
|
Made a couple small comments, but I'll defer to the lib's maintainers for the final approval. 👍 |
|
@brandur-stripe could you take a look at this one? |
There was a problem hiding this comment.
Left a couple comments, but largely LGTM! Thanks for getting all of these through, and thanks for the additional review @dcarney!
ptal @jameshageman-stripe
lib/StripeResource.js
Outdated
| 'request_id': res.requestId, | ||
| 'request_duration_ms': requestDurationMs, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Like for stripe-python, can we put these into a _addTelemetryHeader and _recordRequestMetrics or such?
There was a problem hiding this comment.
This unbounded buffer worries me a little bit, do you think that we could have a check against some arbitrarily large size (e.g. > 100) and if we get there, don't push on a new value and then use utils.emitWarning to indicate that we dropped a request result? (So that we can tell it's happening.)
lib/StripeResource.js
Outdated
| @@ -248,6 +258,13 @@ StripeResource.prototype = { | |||
| Object.assign(headers, options.headers); | |||
| } | |||
|
|
|||
| if (self._stripe.getTelemetryEnabled() && self._stripe._prevRequestMetrics.length > 0) { | |||
| var metrics = self._stripe._prevRequestMetrics.pop(); | |||
There was a problem hiding this comment.
Should we use shift instead of pop so that the buffer becomes FIFO?
jameshageman-stripe
force-pushed
the
jameshageman/add-client-telemetry
branch
from
1c78d2c
to
b36eedc
Compare
|
I've made the buffer both bounded and FIFO |
|
LGTM! Thanks. |
|
Released as 6.23.0. |
r? @brandur-stripe @ob-stripe
cc @stripe/api-libraries @dcarney @akropp-stripe
Replicates stripe/stripe-go#766, adding request duration metrics to subsequent requests in the
X-Stripe-Client-Telemetryheader.It can be enabled with:
Since node.js is concurrent but only runs on one thread, I've used a buffering approach similar to the go implementation but without explicit mutual exclusion. Right now the buffer is unbounded, so it's size is proportional to the maximum number of successful, concurrent requests.
Instead of mocking out the server, I spin up a real http server on a random port for each test, and add assertions on the server side. This may be unconventional, but it allows me to be absolutely sure that the metrics are reaching the server. I used a similar approach in the Go implementation.