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

Record Metrics after `client.run` Resource has released #2283

Merged
merged 8 commits into from Nov 19, 2018

Conversation

@tbrown1979
Copy link
Contributor

@tbrown1979 tbrown1979 commented Nov 16, 2018

Fixes a bug with the Client Metrics code where we would decrement active requests and record total time before the Resource returned from the client.run was actually released. This means our metrics were not accurate.

The fix here is to perform both of these actions after the client.run has .released. To do so, both the recordTotalTime and incr/decr of the active requests have been turned into Resources of their own, that .release after the client.run. This results in the behavior we expect. A test has been added to demonstrate the bug, and to show that it is now fixed.

fixes #2282

tbrown1979 added 4 commits Nov 16, 2018
tbrown1979
…erly for activeRequests
tbrown1979
tbrown1979
tbrown1979
@tbrown1979 tbrown1979 changed the title Finalize ClientMetrics so they finish at the proper time Record Metrics after `client.run` Resource has released Nov 19, 2018
Copy link
Member

@rossabaker rossabaker left a comment

Is it possible to get a failing test to show the bug that is fixed? It may be a hassle.

tbrown1979 added 2 commits Nov 19, 2018
tbrown1979
tbrown1979
@tbrown1979
Copy link
Contributor Author

@tbrown1979 tbrown1979 commented Nov 19, 2018

@rossabaker Added a test that fails with the old code but passes with the new code. It's not very pretty, but it seems to do the job. If there's any improvements you see please let me know!

tbrown1979 added 2 commits Nov 19, 2018
tbrown1979
tbrown1979
@aeons aeons merged commit c71843e into http4s:master Nov 19, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.