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.

Sign up for GitHub

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

Jump to bottom

WIP - first pass at percentiles #55

Merged
merged 1 commit into from
May 3, 2017
Merged

WIP - first pass at percentiles #55

merged 1 commit into from
May 3, 2017

Conversation

lholmquist
Copy link
Member

@lholmquist lholmquist commented Apr 11, 2017 •
edited
Loading

connects to #38

@ghost ghost assigned lholmquist Apr 11, 2017
@ghost ghost added the in progress label Apr 11, 2017
@coveralls
Copy link

coveralls commented Apr 11, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.06%) to 99.257% when pulling c8491a5 on lholmquist:rolling-percentiles-38 into f54822e on bucharest-gold:master.

@coveralls
Copy link

coveralls commented Apr 12, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.06%) to 99.257% when pulling 4c25510 on lholmquist:rolling-percentiles-38 into f54822e on bucharest-gold:master.

@coveralls
Copy link

coveralls commented Apr 17, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-1.0%) to 98.201% when pulling b6dd2c1 on lholmquist:rolling-percentiles-38 into f54822e on bucharest-gold:master.

@lholmquist
Copy link
Member Author

@lance @helio-frota mind just taking a quick look at this if you guys have a chance, just to see if the code looks legit

@coveralls
Copy link

coveralls commented Apr 17, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-1.0%) to 98.201% when pulling e28e516 on lholmquist:rolling-percentiles-38 into f54822e on bucharest-gold:master.

@coveralls
Copy link

coveralls commented Apr 17, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-1.0%) to 98.201% when pulling e28e516 on lholmquist:rolling-percentiles-38 into f54822e on bucharest-gold:master.

lance
lance requested changes Apr 17, 2017
View reviewed changes
Copy link
Member

@lance lance left a comment

Choose a reason for hiding this comment

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

This looks good except for a few nits. I think there should also be some tests. :)

lib/circuit.js Outdated
@@ -275,8 +282,9 @@ class CircuitBreaker extends EventEmitter {
* Emitted when the circuit breaker action takes longer than `options.timeout`
* @event CircuitBreaker#timeout
*/
this.emit('timeout', error);
resolve(fallback(this, error, args) || fail(this, error, args));
const latencyEndTime = Date.now() - latencyStartTime;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: How about just naming this latency, since it's not really the end time, but the actual difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, that makes sense

lib/circuit.js Outdated
.catch((error) =>
handleError(error, this, timeout, args, resolve, reject));
.catch((error) => {
const latencyEndTime = Date.now() - latencyStartTime;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Same as comment above re: the name.

lib/circuit.js Outdated
if (fb) resolve(fb);
else reject(error);
}

function fallback (circuit, err, args) {
function fallback (circuit, err, args, latencyEndTime) {
Copy link
Member

Choose a reason for hiding this comment

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

Is latencyEndTime used here? I don't really follow.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, doesn't look like it is, i think i put it in there because i had a thought about storing the latency during the fallback event, but i don't think we need to do that, i'll remove it for now

lib/status.js
});

if (this.rollingPercentilesEnabled) {
acc.latencyTimes.push.apply(acc.latencyTimes, val.latencyTimes || []);
Copy link
Member

Choose a reason for hiding this comment

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

Why not just call push()? What's the need for apply?

Copy link
Member Author

@lholmquist lholmquist Apr 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

we need to concatenate the values in that latencyTimes so if we just did push, then we would get something like this: [ [1], [2], [3], [4] ] instead of [1,2,3,4] . would be nice if we could use the spread syntax here, but i don't think it goes back to node 4

@lholmquist lholmquist mentioned this pull request Apr 17, 2017
Merged
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 98.201% when pulling 22f226b on lholmquist:rolling-percentiles-38 into f54822e on bucharest-gold:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 98.201% when pulling 22f226b on lholmquist:rolling-percentiles-38 into f54822e on bucharest-gold:master.

@lance
Copy link
Member

lance commented Apr 17, 2017 via email

@coveralls
Copy link

coveralls commented Apr 18, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-1.005%) to 98.175% when pulling e4f6a8d on lholmquist:rolling-percentiles-38 into 3f5827a on bucharest-gold:master.

@coveralls
Copy link

coveralls commented Apr 18, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.09%) to 99.27% when pulling 088ea05 on lholmquist:rolling-percentiles-38 into 3f5827a on bucharest-gold:master.

@coveralls
Copy link

coveralls commented Apr 24, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.09%) to 99.275% when pulling 3130e7c on lholmquist:rolling-percentiles-38 into 4d89ae4 on bucharest-gold:master.

@coveralls
Copy link

coveralls commented Apr 24, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.09%) to 99.275% when pulling e61c7d7 on lholmquist:rolling-percentiles-38 into 4d89ae4 on bucharest-gold:master.

@lholmquist
feat: addition of rolling percentile latency's. GH-ISSUE nodeshift#38
ce7b50d 
a new option has been added: rollingPercentilesEnabled, which defaults to true.  If this is false, all latency's and means are -1
@coveralls
Copy link

coveralls commented Apr 24, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.09%) to 99.275% when pulling ce7b50d on lholmquist:rolling-percentiles-38 into 4d89ae4 on bucharest-gold:master.

lance
lance approved these changes May 3, 2017
View reviewed changes
Copy link
Member

@lance lance left a comment

Choose a reason for hiding this comment

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

I'll fix those wording issues in the merge

test/test.js
breaker.fire(1);
});

test('Circuit Breaker timeout event emits latency', (t) => {
Copy link
Member

Choose a reason for hiding this comment

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

This should say "Breaker failure event". And the one below should say "Breaker timeout event" :-)

@lance lance merged commit ce7b50d into nodeshift:master May 3, 2017
@ghost ghost removed the in progress label May 3, 2017
@lholmquist lholmquist mentioned this pull request Nov 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lance lance lance approved these changes

Assignees

@lholmquist lholmquist

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lholmquist @coveralls @lance @balunasj