-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[chore] Source Timeout for Providers #33958
[chore] Source Timeout for Providers #33958
Conversation
// Make a different context for our provider calls, to differentiate between a provider timing out and the entire | ||
// context being cancelled | ||
var childCtx context.Context | ||
if p.timeout != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to do this here and not at the top level (when we build the source provider)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused - which part should we move where? Moving the creation of the child context into the Chain
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving the WithTimeout
up until we call Source
instead of here. It does mean adding it in more places, which is a bit more annoying. I'll leave what to do up to you
Also, I am now wondering if we want a separate timeout for this or if just the per-OTLP blob timeout added by the exporterhelper works . Probably we want a separate one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - I think its good to leave in here. I think its good for this logic to be right next to the call site and if we ever want to do something fancier like per source timeout it will be much easier to add it here.
I definitely think we should want a separate timeout for this! the per otlp blob timeout should be used just for the normal export case - not the occasions we're blocked on fetching a source.
Description:
Datadog exporter tests have an unnecessary 30 second wait while it waits for various metadata service calls to timeout (this is the default TCP timeout in the go network stack). Also had to modify the ec2 provider so that it respected the context. Most of the changes are just propagating this new timeout to the provider, and propagating this context around. I made the decision to differentiate the timeout context and the overall context for the chainProvider - these could be brought together but it looks like we had existing logic for a context timeout vs a chainProvider context cancellation so I wanted to respect that split.
Testing:
mostly stuck to a 31 second timeout on existing tests to not change behavior. Added a tighter timeout for some of the slower tests.
Added some tests for the timeout logic