-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Request context is passed to PushIfNotCancelled instead of VU context #1260
Comments
cuonglm
added a commit
that referenced
this issue
Dec 4, 2019
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. To fix it, simply wrapping the request context with the client timeout. Fixed #1260
cuonglm
added a commit
that referenced
this issue
Dec 5, 2019
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. To fix it, simply wrapping the request context with the client timeout. Fixed #1260
cuonglm
changed the title
Client.Timeout is not passed to request context
Request context is passed to PushIfNotCancelled instead of VU context
Dec 5, 2019
cuonglm
added a commit
that referenced
this issue
Dec 6, 2019
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
added a commit
that referenced
this issue
Dec 6, 2019
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
added a commit
that referenced
this issue
Dec 6, 2019
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
added a commit
that referenced
this issue
Dec 17, 2019
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
added a commit
that referenced
this issue
Dec 17, 2019
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
added a commit
that referenced
this issue
Dec 17, 2019
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
added a commit
that referenced
this issue
Dec 17, 2019
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
added a commit
that referenced
this issue
Dec 17, 2019
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This script fails with current go master:
There are two problems for current code base:
The text was updated successfully, but these errors were encountered: