Fix wrong context is used to decide pushing metrics or not. #1261
+77
−28
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #1261 +/- ##
==========================================
+ Coverage 75.42% 75.42% +<.01%
==========================================
Files 150 150
Lines 10883 10884 +1
==========================================
+ Hits 8208 8209 +1
Misses 2209 2209
Partials 466 466
Continue to review full report at Codecov.
|
cuonglm
force-pushed
the
fix/http-request-timeout
branch
from
8804297 to
db4b2f9
Compare
cuonglm
changed the title
cuonglm
force-pushed
the
fix/http-request-timeout
branch
from
db4b2f9 to
4169f7a
Compare
cuonglm
changed the title
cuonglm
force-pushed
the
fix/http-request-timeout
branch
from
4169f7a to
4722cb3
Compare
na--
previously approved these changes
Dec 6, 2019
Made it as v0.27.0 |
Test for error message contains string instead of euqal.
Go 1.13 and below has a bug that client timeout will not be propagated to http request context. That bug was fixed already and will be present in go1.14 (golang/go#31657). Currently, we set Timeout in our Client, but do not use it else where. It will not affect the request context for now, but does when go1.14 release. To make it compatible and works correctly with other go versions, we set the request context explicitly with the timeout. Update #1260
cuonglm
force-pushed
the
fix/http-request-timeout
branch
from
4722cb3 to
d9c2ec1
Compare
golangcibot
reviewed
Dec 17, 2019
The purpose of PushIfNotCancelled is pushing the sample if the passing context is done. But its name will make confusion, because a timeout context also causes the context cancelled. So changing the name to PushIfNotDone to make it clearer. While at it, also change the select/ctx.Done pattern to ctx.Err variant. It saves us some lines of code and maybe slightly better performance.
PushIfNotDone is intended to use with VU context instead of request context. This is a bug in current code base, but it is not triggered due to the bug in Go net/http. A request context can be in cancelled state due to be cancelled or timeouted. PushIfNotDone won't see the difference and will skip pushing metrics for timeouted requests. Fixing it by passing VU context to PushIfNotDone instead of request ctx. Fixes #1260
cuonglm
force-pushed
the
fix/http-request-timeout
branch
from
d9c2ec1 to
64f0614
Compare
na--
approved these changes
Dec 17, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Go 1.13 and below has a bug that client timeout will not be propagated
to http request context. That bug was fixed already and will be present
in go1.14 (golang/go#31657).
With current go master, our TestSetupTeardownThresholds fails. The
reason is that we only push sample if the request context not done. When
the bug in Go stdlib was not fixed, our own http Client.Timeout is not
passed to request context, so when client makes request, it does not
aware about the request context, and always push the sample.
Now when Client.Timeout is passed to request context, as long as the
client finish the request, the request context is consider done, and we
do not push the sample anymore.
Further more, we are using the wrong context for PushIfNotCancelled.
It's intended to use with VU context instead of the request context.
In summary, to fix it:
Fixed #1260