Confusing timeout setting options

[rakyll]

There are two settings types that allows users to set a timeout and these are not orthogonal.

TimeoutSettings.Timeout
RetrySettings.MaxElapsedTime

It's not clear which one should be taken into account if two conflicting settings are provided. Instead of providing two settings, is it possible to remove RetrySettings.MaxElapsedTime?

[pkositsyn]

From exporterhelper README:

max_elapsed_time (default = 120s): Is the maximum amount of time spent trying to send a batch; ignored if enabled is false

These are totally orthogonal settings. Max elapsed time includes time taken for retries and waiting for retries.

[pkositsyn]

Clearly, max_elapsed_time must be greater than timeout. Do you think that documentation isn't obvious on that?

[rakyll]

@pkositsyn I think what's not clear to users whether max_elapsed_time overrides timeout. If yes, why would we expect users to use a new setting (max_elapsed_time) rather than modifying the timeout?

[pkositsyn]

@rakyll I don't understand - these are two totally different things
If max_elapsed_time is less than timeout - that means just no retries and this should be replaced by disable of retries.
Otherwise the flow is the following: (1st try) timeout - wait interval time - (2nd try) timeout - wait interval time - ... - max elapsed time - the payload is lost.

[tigrannajaryan]

These are different settings. Need to clarify docs. Reduced priority since not a functionality bug and it works as expected.

[flands]

@rakyll I took a quick look at this to see what could be done to clarify the docs. Given timeout and max_elapsed_time are different things, it is unclear how to proceed. timeout refers to an individual attempt to export data (could be the original attempt or a retry attempt), while max_elapsed_time is checked on every retry -- if on a retry check this value is exceeded, retries will no longer be attempted. In theory, max_elapsed_time could be less than timeout and that could be a valid configuration -- for example, if the attempt to export fails and returns a retriable error immediately. Given the checks happen at different times and for different reasons, I could not think of a way to improve the docs. Do you have any suggestions to address this issue?

[pkositsyn]

@flands could you clarify, what difference is between "retries are disabled" and "max_elapsed_time is less than timeout"? In my opinion, they are interchangeable and actually it is reasonable to update the docs so that the first option of these two should be chosen

[rakyll]

@flands If you clarify the doc with what you said in this issue, that'd help. The current description doesn't capture:

• timeout is scoped to an individual request in a batch. It calls out the "individual" without mentioning it's an individual in a retry series.
• max_elapsed_time is the timeout for the entire export attempt including retries.
• And as @pkositsyn says, what happens if max_elapsed_time is less than timeout? Is it valid configuration?

[flands]

@rakyll @pkositsyn please see the PR and let me know what you think