[telemetrygen] Add timeout flag#47299
Conversation
bogdan-st
left a comment
There was a problem hiding this comment.
Looks good, small nits that I think should be addressed.
Also please add this new flag to the readme as well.
| fs.Float64Var(&c.Rate, "rate", c.Rate, "Approximately how many metrics/spans/logs per second each worker should generate. Zero means no throttling.") | ||
| fs.Var(&c.TotalDuration, "duration", "For how long to run the test. Use 'inf' for infinite duration.") | ||
| fs.DurationVar(&c.ReportingInterval, "interval", c.ReportingInterval, "Reporting interval") | ||
| fs.DurationVar(&c.Timeout, "timeout", c.Timeout, "Maximum time to wait for the test to complete. Zero means no timeout.") |
There was a problem hiding this comment.
I think this is confusing as the timeout is the wait time for one export event not the full "test"
| fs.Float64Var(&c.Rate, "rate", c.Rate, "Approximately how many metrics/spans/logs per second each worker should generate. Zero means no throttling.") | ||
| fs.Var(&c.TotalDuration, "duration", "For how long to run the test. Use 'inf' for infinite duration.") | ||
| fs.DurationVar(&c.ReportingInterval, "interval", c.ReportingInterval, "Reporting interval") | ||
| fs.DurationVar(&c.Timeout, "timeout", c.Timeout, "Maximum time to wait for the test to complete. Zero means no timeout.") |
There was a problem hiding this comment.
Also, zero means no timeout is misleading, zero would configure the SDK's default timeout which is 10s.
There was a problem hiding this comment.
Agree, will update the usage message.
Also, digging a bit into this it looks like the exporter is defaulting to a 5s timeout , while the Go SDK exporters default to 10s
| } | ||
| } | ||
|
|
||
| func TestHTTPExporterOptions_Timeout(t *testing.T) { |
There was a problem hiding this comment.
Would be worth testing the grpc exporter as well, the new timeout is wired for both but this is only testing http.
Add support timeout setting for telemetrygen open-telemetry#47203
922bfc7 to
25cc331
Compare
|
@bogdan-st do you want to give this another look before I merge? |
bogdan-st
left a comment
There was a problem hiding this comment.
Still would have liked a README line but that's not that important.
LGTM otherwise
|
@araiu Can you add a README line documenting this option? |
Definitely, missed this from @bogdan-st's previous comment. Also took the liberty to update the While we're at this, @bogdan-st do you see any other flags that might be worth getting surfaced instead of ? |
|
I think
Thanks!
I think that is a valuable conversation, I will merge this PR and whatever we decide can be included in a future PR |
|
Thank you for your contribution @araiu! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. If you are getting started contributing, you can also join the CNCF Slack channel #opentelemetry-new-contributors to ask for guidance and get help. |
Description
Add support timeout setting for telemetrygen
Link to tracking issue
Fixes #47203
Testing
Sending traces to a collector that has an artificial delay of
10sbefore answering