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

Support duration format for Timeout config field #104

Closed
ezsilmar opened this issue Jun 25, 2019 · 3 comments · Fixed by #115
Closed

Support duration format for Timeout config field #104

ezsilmar opened this issue Jun 25, 2019 · 3 comments · Fixed by #115
Assignees
Labels
CLI Issues for ghz CLI enhancement New feature or request

Comments

@ezsilmar
Copy link
Contributor

ezsilmar commented Jun 25, 2019

Hello,

Is your feature request related to a problem? Please describe.
There's a Timeout option in ghz config file which expects uint as a value. The value is interpreted as a number of seconds. It means that we can't specify a timeout which is less than a second or is not an integer (f.e. 1500 ms or 0.5). The same also applies to DialTimeout and KeepaliveTime parameters.

The ability to specify precise timeout is extremely important. Imagine you have an SLA for the response time of your service, f.e. 100 ms. In production, the request will be cancelled on the client side if the server fails to answer in time. Now it's possible to emulate this behavior using ghz only with 1-second precision.

Describe the solution you'd like
It would be great if Timeout (as well as other time options) will accept duration string like 500ms. This will also make all time-related options behave in the same predictable way. To support backward compatibility we can interpret the value without unit as seconds (maybe it already works like this?)

Describe alternatives you've considered
We could treat the uint as the number of milliseconds but this would be a breaking change.

Additional context
I also found a little bug related to this issue in UnmarshalJSON function from config.go.
If X option is specified but Z is not, the X one will be ignored because of the error check. It'd be better to check if the value is specified before trying to parse it, and if it's specified but invalid throw the error.

@bojand bojand self-assigned this Jun 26, 2019
@bojand bojand added CLI Issues for ghz CLI enhancement New feature or request labels Jun 26, 2019
@ezsilmar
Copy link
Contributor Author

ezsilmar commented Jul 1, 2019

A simple fix we use for now: criteo-forks@a92edf8

I'm not sure I'll have time to make it a proper contribution though :(

@bojand
Copy link
Owner

bojand commented Jul 12, 2019

Thank you for reporting this. I understand the issue and I agree this could be improved. Just wanted to say I will try and take a deeper look at the solution and address this soon when I get some more free time.

bojand added a commit that referenced this issue Jul 29, 2019
bojand added a commit that referenced this issue Jul 29, 2019
Change uint duration values to Duration type. Fixes #104.
@bojand
Copy link
Owner

bojand commented Jul 29, 2019

Hello this should be working now in 0.40.0. All duration values originally configured as ints have been changed to the duration type. If there are any problems please create a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI Issues for ghz CLI enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants