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
Add processed requests counter to workers #2061
Conversation
The changes are pretty basic for now. What do you think ? Should we count hijacked connections ? What about TCP connections (lopez mode) ? I see this feature as a HTTP requests counter so the code looks good as it is but there is probably some edge case that I forgot 😄 |
LGTM. I'm not really worried about hijacked or TCP requests for a first iteration. We can leave that for the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized - one thing we don't have here is a test that the counter actually increments! We should have tests for both single and cluster mode. Probably in the integration test files (test_integration_*.rb) would make sense.
Indeed, I've just tested the feature locally, I will add a spec.
…On Thu, Oct 31, 2019, 5:36 PM Nate Berkopec ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I just realized - one thing we don't have here is a test that the counter
actually increments! We should have tests for both single and cluster mode.
Probably in the integration test files (test_integration_*.rb) would make
sense.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2061?email_source=notifications&email_token=ABG633KYHDOI3QLRD4OS3VTQRMCPNA5CNFSM4JGP3WCKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJ5IYSY#pullrequestreview-310021195>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABG633KF5QLWHJEMK4Z5RULQRMCPNANCNFSM4JGP3WCA>
.
On Thu, Oct 31, 2019, 5:36 PM Nate Berkopec ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I just realized - one thing we don't have here is a test that the counter
actually increments! We should have tests for both single and cluster mode.
Probably in the integration test files (test_integration_*.rb) would make
sense.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2061?email_source=notifications&email_token=ABG633KYHDOI3QLRD4OS3VTQRMCPNA5CNFSM4JGP3WCKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJ5IYSY#pullrequestreview-310021195>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABG633KF5QLWHJEMK4Z5RULQRMCPNANCNFSM4JGP3WCA>
.
|
Those tests are failing on Windows. I think we should probably use something other than curl. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs windows tests passing
#2106 got there first. Thanks for starting this! |
Description
PR implementing feature #2057
Your checklist for this pull request
[changelog skip]
to all commit messages.[ci skip]
to the title of the PR.#issue
" to the PR description or my commit messages.