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: report many pipeline resets #282

Merged
merged 3 commits into from Sep 10, 2020
Merged

feat: report many pipeline resets #282

merged 3 commits into from Sep 10, 2020

Conversation

feugy
Copy link
Contributor

@feugy feugy commented Sep 10, 2020

This is a follow up of #281
Now, request iterator resets when some of its setupRequest() returning falsey value (previously it the pipeline was simply crashing).

But this may actually obfuscate bugs of the tested service: often setupRequest() fails as the previous response did not contained expected data.

This PR simply displays how many times the pipeline reseted, so users could decide whether it is intended or not.

┌─────────┬────────┬────────┬────────┬────────┬───────────┬───────────┬────────────┐
│ Stat    │ 2.5%   │ 50%    │ 97.5%  │ 99%    │ Avg       │ Stdev     │ Max        │
├─────────┼────────┼────────┼────────┼────────┼───────────┼───────────┼────────────┤
│ Latency │ 121 ms │ 624 ms │ 890 ms │ 914 ms │ 541.06 ms │ 237.32 ms │ 1002.16 ms │
└─────────┴────────┴────────┴────────┴────────┴───────────┴───────────┴────────────┘
┌───────────┬────────┬────────┬────────┬────────┬────────┬─────────┬────────┐
│ Stat      │ 1%     │ 2.5%   │ 50%    │ 97.5%  │ Avg    │ Stdev   │ Min    │
├───────────┼────────┼────────┼────────┼────────┼────────┼─────────┼────────┤
│ Req/Sec   │ 218    │ 218    │ 372    │ 479    │ 356.5  │ 70.29   │ 218    │
├───────────┼────────┼────────┼────────┼────────┼────────┼─────────┼────────┤
│ Bytes/Sec │ 190 kB │ 190 kB │ 372 kB │ 465 kB │ 369 kB │ 67.3 kB │ 190 kB │
└───────────┴────────┴────────┴────────┴────────┴────────┴─────────┴────────┘

Req/Bytes counts sampled once per second.

10k requests in 28.09s, 10.3 MB read
18 errors (0 timeouts)
request pipeline was resetted 4 times

Note: I know test/progressTracker.test.stub.js isn't part of the test suite, but as I've changed it to troubleshoot the feature, I though it would be fine to keep a new test in it.
The feature is properly covered in est/httpClient.test.js and test/run.test.js test files.

Edit: this PR actually unveiled some flaws in the previous implementation.
Reseting request pipeline the way I did is wrong, as it may leave currentRequest.requestBuffer to null.

This new version fixes the flaw, and also handle a specific case where the very first setupRequest() fails. In this situation, we can not reset pipeline (or it'll never end), and we must exit on an error.

lib/progressTracker.js Outdated Show resolved Hide resolved
Copy link
Owner

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit b44838f into mcollina:master Sep 10, 2020
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

2 participants