restify / clients

HttpClient, StringClient, and JsonClient extracted from restify
MIT License
57 stars 34 forks source link

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

Open DonutEspresso opened 6 years ago

DonutEspresso commented 6 years ago

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.

coveralls commented 6 years ago

Coverage Status

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

DonutEspresso commented 6 years ago

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?

hekike commented 6 years ago

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