-
Notifications
You must be signed in to change notification settings - Fork 123
Fix request dump debug log #259
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
Conversation
tobio
left a comment
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 know this is a super quick fix, but we could simplify the tf provider by using the go-elasticsearch Logger interface.
internal/clients/debug.go
Outdated
| func (d *debugTransport) Perform(r *http.Request) (*http.Response, error) { | ||
| ctx := r.Context() | ||
| reqData, err := httputil.DumpRequestOut(r, true) | ||
| reqData, err := httputil.DumpRequest(r, true) |
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.
From the docs
It should only be used by servers to debug client requests
I think that we should be either using the build in go-elasticsearch loggers or if using tflog.* was important implementing a custom logger
a9d58f0 to
178b497
Compare
|
@webfella are you able to take a look at my changes here? |
webfella
left a comment
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.
LGTM 🥟
Resolves #258
httputil.DumpRequestOutis actually executing dummy RondTrip and it causes following error since the schema and host is not set at the point.request dump error: &errors.errorString{s:"unsupported protocol scheme \"\""}This PR changes the method to use
httputil.DumpRequestwhich can work with an incomplete url.