-
Notifications
You must be signed in to change notification settings - Fork 435
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
profiler: Implement WithUploadTimeout #852
Conversation
cb33583
to
344e01c
Compare
344e01c
to
0b2e7a0
Compare
Also add support for the DD_PROFILING_UPLOAD_TIMEOUT used by other DD profilers such as Java. The timeout is implemented using a request context, which allows removing the timeout option from the defaultClient and WithUDS option. defaultHTTPTimeout is promoted to DefaultUploadTimeout to be consistent with the other default options. The order of preference is: 1. DefaultUploadTimeout 2. DD_PROFILING_UPLOAD_TIMEOUT 3. WithUploadTimeout() Implements PROF-2810
0b2e7a0
to
73b1bbf
Compare
…filer-upload-timeout
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.
This is an objectively better approach than the existing pattern. Until recently there was no specified upload timeout; the state of code prior to this pull request was modeled on what dd-trace-go/ddtrace
is doing.
dd-trace-go/ddtrace/tracer/option.go
Line 360 in 4451a5c
Timeout: defaultHTTPTimeout, dd-trace-go/ddtrace/tracer/option.go
Line 379 in 4451a5c
Timeout: defaultHTTPTimeout, dd-trace-go/ddtrace/tracer/transport.go
Line 45 in 4451a5c
Timeout: defaultHTTPTimeout,
However, I do think there should be a default timeout, which this pull request effectively removes.
We might also add a test for this in upload_test.go
.
@gbbr thoughts on this approach and updating the trace client to use a similar pattern?
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.
Nevermind, I can't read. I do see the DefaultUploadTimeout
I guess users can currently change the default client to a custom There was no need to have this until now with a specific function, but we can if we have to. |
@gbbr that makes sense. Profiler has a |
…filer-upload-timeout
b3cf200
to
3db6059
Compare
Also add support for the DD_PROFILING_UPLOAD_TIMEOUT used by other
DD profilers such as Java.
The timeout is implemented using a request context, which allows
removing the timeout option from the defaultClient and WithUDS option.
defaultHTTPTimeout is promoted to DefaultUploadTimeout to be consistent
with the other default options.
The order of preference is:
Implements PROF-2810