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

Implementation #1

Merged
merged 8 commits into from Nov 18, 2015

Conversation

@arb
Copy link
Contributor

arb commented Nov 17, 2015

This is the initial implementation of this object. I copied all the tests over from Good, added some others and cleaned up the general implementation. This will serve as version 1 of this object.

@arb arb modified the milestone: 1.0.0 Nov 17, 2015
@arb arb added the feature label Nov 17, 2015
@arb arb mentioned this pull request Nov 17, 2015
@arb arb force-pushed the implementation branch to 2eb3f34 Nov 17, 2015
README.md Outdated
Starts an Oppsy object collecting network and server information.
- `interval` - number of seconds to wait between each data sampling.

#### opps.stop()

This comment has been minimized.

Copy link
@cjihrig

return (callback) => {

process.nextTick(() => {

This comment has been minimized.

Copy link
@cjihrig

cjihrig Nov 17, 2015

Since you're targeting new versions of Node, you can do process.nextTick(callback, null, predicate());

@cjihrig

This comment has been minimized.

Copy link

cjihrig commented Nov 17, 2015

LGTM with a couple comments. You may or may not want to make this a plugin. Your call.

language: node_js

node_js:
- 4.0

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 17, 2015

Member

Quotes on all versions

- 4
- 5

sudo: false

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 17, 2015

Member

I think this is the default now

This comment has been minimized.

Copy link
@arb

arb Nov 17, 2015

Author Contributor

I just copied the travis file used in hapi core.

This comment has been minimized.

sockets: this._networkMonitor.sockets
};
}
start(interval) {

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 17, 2015

Member

I find line breaks between functions more readable

}
start(interval) {

Hoek.assert(interval <= 2147483647, 'interval must be less than 2147483648');

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 17, 2015

Member

Can't you extend big-time to deal with that ? (although admittedly stats that are emitted that often are useless :))

This comment has been minimized.

Copy link
@arb

arb Nov 17, 2015

Author Contributor

I ❤️ big-time.

const Items = require('items');

class Network {
constructor(server/*, httpAgents, httpsAgents */) {

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 17, 2015

Member

Why not declare those arguments ? It's not like it's dynamic afterwards.

this._httpAgents = [].concat(arguments[1] || Http.globalAgent);
this._httpAgents = [].concat(arguments[2] || Https.globalAgent);

this._server.on('request-internal', this._onRequest.bind(this));

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 17, 2015

Member

Minor but I think you should bind once.

This comment has been minimized.

Copy link
@arb

arb Nov 17, 2015

Author Contributor

You mean use once instead of on? Yeah that's probably a good idea.

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 17, 2015

Member

Oh nevermind, I'm tired, I thought you bound twice the same method... 😭

const port = request.connection.info.port;
const statusCode = request.response && request.response.statusCode;

this._responseTimes[port] = this._responseTimes[port] || { count: 0, total: 0, max: 0 };

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 17, 2015

Member

This looks like a lot of repeating of that thing

const overview = {};
for (let i = 0; i < ports.length; ++i) {
const port = ports[i];
const count = Hoek.reach(this, '_responseTimes.' + port + '.count', { default: 1 });

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 17, 2015

Member

Template string ?

internals.delay = (callback) => {

const bench = new Hoek.Bench();
setImmediate(() => {

This comment has been minimized.

Copy link
@Marsup

Marsup Nov 17, 2015

Member

Better in one line no ?

arb added a commit that referenced this pull request Nov 18, 2015
Implementation
@arb arb merged commit e2c3c20 into master Nov 18, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@arb arb deleted the implementation branch Nov 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.