clients icon indicating copy to clipboard operation
clients copied to clipboard

feat: add support for before/after hooks to support tracing outbound requests

Open DonutEspresso opened this issue 7 years ago • 3 comments

Carries #95 over the finish line - would like to get input from the peanut gallery. This should dove tail nicely with our metrics needs as well.

DonutEspresso avatar Feb 22 '18 02:02 DonutEspresso

Coverage Status

Coverage increased (+0.3%) to 87.5% when pulling 5ce8ce8a1eacb7d94882a3744a150d6a4fc77dc4 on req-tracing into 471ac194f791fea0e1acd9442a9005ae6948eadf on master.

coveralls avatar Feb 22 '18 02:02 coveralls

Hey @joshwilsdon, let's continue the convo here. I rebased against master so this should be in a good place now.

For context, we're looking at building a custom REST client for internal use and we need to do some pretty low level metrics and error logging. That's not currently possible without doing something intrusive like extending directly from JSONClient and overriding existing class methods. These life cycle hooks are a good alternative.

The one thing I would want to add in this PR is the ability for after to mutate the return values (specifically, err, but open to having after's next be a pass through to the final callback). For example, if using Cueball, this after hook would allow one to create an error chain or MultiError combining ConnectionPool errors with the likely last cause of failure via pool.getLastError().

Since before has the ability to mutate the outgoing request, it seems to make sense to make after symmetrical. If not, after might be more suitable as an event emitted on the client. Thoughts?

DonutEspresso avatar Feb 22 '18 02:02 DonutEspresso

Would be nice to create an OpenTracing example one day, with the new timings object we could have the full Request and TCP connection lifecycle like here: https://github.com/RisingStack/opentracing-auto/blob/master/src/instrumentation/httpClient.js#L36 just without monkey patching.

HTTP Timings

hekike avatar Feb 22 '18 17:02 hekike