Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

DonutEspresso
Copy link
Member

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
Copy link

coveralls commented Feb 22, 2018

Coverage Status

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

Copy link
Member

@retrohacker retrohacker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! This looks great!

@DonutEspresso
Copy link
Member Author

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?

}
cb();
}, before: function (opts, cb) {
beforeRan = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add more assertation around opts?
To be sure that this is the complete opts object, not just the subset of it.

@hekike
Copy link
Member

hekike commented Feb 22, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants