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

Restored wreck logging. Closes #469. #514

Merged
merged 1 commit into from Dec 1, 2016
Merged

Restored wreck logging. Closes #469. #514

merged 1 commit into from Dec 1, 2016

Conversation

@arb
Copy link
Contributor

arb commented Sep 14, 2016

Cleaned up event handling logic and just added a boolean to control whether events are reported on not. It was much easier and required far less code to keep a handler to all the added event listeners.

@@ -28,7 +28,8 @@
"async": "1.5.x",
"code": "3.x.x",
"hapi": "15.x.x",
"lab": "11.x.x"
"lab": "11.x.x",
"wreck": "9.x.x"

This comment has been minimized.

Copy link
@vdeturckheim
@arb arb added this to the 7.1.0 milestone Sep 16, 2016
@arb arb force-pushed the arb:wreck-logging branch from 472a6dc to 044d019 Sep 16, 2016
@arb arb added the feature label Oct 31, 2016
@kevinmstephens

This comment has been minimized.

Copy link
Contributor

kevinmstephens commented Nov 14, 2016

@Marsup Just a friendly request to complete the review if you can fit it in. Waiting on this release to hit NPM. Cheers.

}

// Events can not be any of ['log', 'request-error', 'ops', 'request', 'response', 'tail']
for (let i = 0; i < this.settings.extensions.length; ++i) {
const event = this.settings.extensions[i];
const handler = this._extensionHandler.bind(this, event);
const self = this;

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 14, 2016

Member

Any reason for this not to be outside the loop ?


this._state.extensions[event] = handler;
this._server.on(this.settings.extensions[i], handler);
const args = Array(arguments.length);

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 14, 2016

Member

Not sure it's as efficient but with node >= 4 I think we can safely go with Array.from(arguments).

timestamp: Date.now(),
payload: args
};
self.push(() => Object.assign({}, payload));

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 14, 2016

Member

This is not a full clone, is that still ok ?

This comment has been minimized.

Copy link
@arb

arb Nov 21, 2016

Author Contributor

Yes. It's as close as I'm willing to do for something that gets called so often.


if (this.settings.wreck) {
const wreck = Symbol.for('wreck');
process[wreck].on('response', this._wreckHandler);

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 14, 2016

Member

So only tracking the responses, not the requests ?

This comment has been minimized.

Copy link
@arb

arb Nov 21, 2016

Author Contributor

Yes. This is just restoring what was already there. I wasn't looking to add more than that.

this.pid = process.pid;

if (error) {
this.error = {

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 14, 2016

Member

Why not the full error ?

This comment has been minimized.

Copy link
@arb

arb Nov 21, 2016

Author Contributor

Again, I'm just porting what was already there.

url: uri.href,
protocol: uri.protocol,
host: uri.host,
headers: Hoek.reach(request, '_headers')

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 14, 2016

Member

Why not request._headers ? That's possible there's no request ?

},
"scripts": {
"test": "lab -m 5000 -t 100 -v -La code",
"test-cov-html": "lab -m 5000 -r html -o coverage.html -La code"

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 14, 2016

Member

Mistake ?

This comment has been minimized.

Copy link
@arb

arb Nov 21, 2016

Author Contributor

Nope. Never use any of those.


return event.event === 'wreck';
});
expect(result).to.be.an.instanceof(Utils.WreckResponse);

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 14, 2016

Member

Shouldn't you check what's in it as well ?

API.md Outdated
@@ -192,6 +193,9 @@ Event object associated with the `responseEvent` event option into Good. `reques
- `route` - route path used by request. Maps to `request.route.path`.
- `log` - maps to `request.getLog()` of the hapi request object.
- `config` - plugin-specific config object combining `request.route.settings.plugins.good` and `request.plugins.good`. Request-level overrides route-level. Reporters could use `config` for additional filtering logic.
- `headers` - the request headers if `includes.request` includes "headers"
- `requestPayload` - the request payload if `includes.request` includes "payload"

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 14, 2016

Member

Those things don't appear in code anywhere, should it ?

This comment has been minimized.

@arb arb force-pushed the arb:wreck-logging branch from 044d019 to 7c47e74 Nov 21, 2016
@arb arb dismissed Marsup’s stale review Dec 1, 2016

Because we talked on Gitter and he was OK with this moving forward.

Cleaned up event handling logic and just added a boolean to control whether events are reported on not. It was much easier and required far less code to keep a handler to all the added event listeners.
@arb arb force-pushed the arb:wreck-logging branch from 7c47e74 to 617f0ff Dec 1, 2016
@arb arb merged commit 45606c7 into hapijs:master Dec 1, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arb arb deleted the arb:wreck-logging branch Dec 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.