-
Notifications
You must be signed in to change notification settings - Fork 5.7k
feat(outputs.opentelemetry): Support http protocol #17997
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks so much for the pull request! |
srebhan
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.
Thanks @minuk-dev for your contribution! I do have some initial comments below. Let's first get the code structure right and then work on the details...
srebhan
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.
Thanks @minuk-dev for the update! Here is round two of my comments... ;-)
| const ( | ||
| maxHTTPResponseReadBytes = 64 * 1024 // 64 KB | ||
| ) |
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.
Don't declare this const but just use the value!
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 fixed it. But, why?
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.
Because it is used only in one spot and you can interpret the value from the assignment there. Having this indirection makes reviews harder e.g. if someone changes the value here the consequences in the code are not clear. Use constants if they add value e.g. if you check a numeric return code it's better to write
const errnoUnauthorized = 5
...
if errno == errnoUnauthorized {...}instead of
if errno == 5 {...}or if the value might be changed later and is used in multiple places.
srebhan
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.
Thanks for the update @minuk-dev! Some more updates...
| } else { | ||
| h.httpClient.Transport = &http.Transport{ | ||
| TLSClientConfig: &ntls.Config{ | ||
| InsecureSkipVerify: 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.
Hmm we can't enforce this insecure setting! If at all, this should be a user setting.
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 need to find other output plugins how to handle it.
Please wait for a while.
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.
Sorry. I fixed it. I made a mistake.
| defer func() { | ||
| //nolint:errcheck // cannot fail with io.Discard | ||
| // 64KB is a not specific limit. But, opentelemetry-collector also uses 64KB for safety. | ||
| io.CopyN(io.Discard, httpResponse.Body, 64*1024) | ||
| _ = httpResponse.Body.Close() | ||
| }() |
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.
Why do you need this complexity? Wouldn't
| defer func() { | |
| //nolint:errcheck // cannot fail with io.Discard | |
| // 64KB is a not specific limit. But, opentelemetry-collector also uses 64KB for safety. | |
| io.CopyN(io.Discard, httpResponse.Body, 64*1024) | |
| _ = httpResponse.Body.Close() | |
| }() | |
| if _, err := io.Copy(io.Discard, httpResponse.Body) { | |
| return pmetricotlp.ExportResponse{}, err | |
| } |
be good enough?
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.
If the server sends very big response unexpectedly, two codes work differently.
So, I think we should use io.CopyN() instead of io.Copy().
Do you think this defensive code is not needed?
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.
Citing the documentation for http.Response:
// The http Client and Transport guarantee that Body is always
// non-nil, even on responses without a body or responses with
// a zero-length body. It is the caller's responsibility to
// close Body. The default HTTP client's Transport may not
// reuse HTTP/1.x "keep-alive" TCP connections if the Body is
// not read to completion and closed.
So either you have to read the whole body if you care for reusing persistent TCP connections or you don't care and then you can just close the body.
Remember that the body is streamed from the connection so if the server sends a "very big response" it will be read and discarded i.e. you read the response from a chunk of memory or a network buffer in the worst case (also memory) and do nothing with it... Which negative effect to you expect from this?
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 not sure your suggestion is really better. But, I'll change it.
Below is the detailed reason.
Sample code: https://go.dev/play/p/T3vZ1MH-eeK
I use io.CopyN here deliberately as a defensive choice.
This client does not consume the response body, so the goal is to avoid unbounded reads
rather than to maximize connection reuse.
With io.Copy, the client will read until EOF. If the server returns a very large body
or a chunked response that does not terminate properly (e.g. a misbehaving server or
proxy issue), the client would keep reading and consume CPU and network resources.
By limiting the read with io.CopyN, we cap how much data we are willing to consume and
stop early in those cases, even though it means the connection will not be reused.
In this context, protecting client resources is preferred over keep-alive reuse.
You can find many usecases in github.
https://github.com/search?q=io.CopyN%28io.Discard%2C+res.Body&type=code
| func readResponseBody(resp *http.Response) ([]byte, error) { | ||
| if resp.ContentLength == 0 { | ||
| return nil, nil | ||
| } | ||
|
|
||
| maxRead := resp.ContentLength | ||
|
|
||
| // if maxRead == -1, the ContentLength header has not been sent, so read up to | ||
| // the maximum permitted body size. If it is larger than the permitted body | ||
| // size, still try to read from the body in case the value is an error. If the | ||
| // body is larger than the maximum size, proto unmarshaling will likely fail. | ||
| // 64KB is a not specific limit. But, opentelemetry-collector also uses 64KB for safety. | ||
| if maxRead == -1 || maxRead > 64*1024 { | ||
| maxRead = 64 * 1024 | ||
| } | ||
| protoBytes := make([]byte, maxRead) | ||
| n, err := io.ReadFull(resp.Body, protoBytes) | ||
|
|
||
| // No bytes read and an EOF error indicates there is no body to read. | ||
| if n == 0 && (err == nil || errors.Is(err, io.EOF)) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| // io.ReadFull will return io.ErrorUnexpectedEOF if the Content-Length header | ||
| // wasn't set, since we will try to read past the length of the body. If this | ||
| // is the case, the body will still have the full message in it, so we want to | ||
| // ignore the error and parse the message. | ||
| if err != nil && !errors.Is(err, io.ErrUnexpectedEOF) { | ||
| return nil, err | ||
| } | ||
|
|
||
| return protoBytes[:n], nil | ||
| } |
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 really think you should use io.LimitedReader instead.
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 think they are different. But, I understand you and I'm not sure about it.
I check it.
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 not sure I can follow... The body is a ReadCloser so I would do (in the caller side of the code)
// Make sure we close the body
defer httpResponse.Body.Close()
// Read the body up to the maximum length
r := io.LimitReader(httpResponse.Body, maxBodyLength)
body, err := io.ReadAll(r)
if err != nil {
return pmetricotlp.ExportResponse{}, fmt.Errorf("reading response body failed: '%w'", err)
}
// Discard the rest of the body
n, err := io.Copy(io.Discard, httpResponse.Body)
if err != nil {
return pmetricotlp.ExportResponse{}, fmt.Errorf("discarding remaining response body failed: '%w'", err)
}
if n > 0 {
return pmetricotlp.ExportResponse{}, fmt.Errorf("response body %d byte larger than supported maximum", n)
}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 think we don't need to use resp.ContentLength in your version.
We don't have to discard the rest of the body.
So, I changed it like this: 17a6922.
srebhan
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.
Thanks @minuk-dev! Please see my comment and also my responses above.
| exportResponse := pmetricotlp.NewExportResponse() | ||
| switch httpResponse.Header.Get("Content-Type") { | ||
| case "application/x-protobuf": | ||
| err = exportResponse.UnmarshalProto(responseBytes) | ||
| if err != nil { | ||
| return pmetricotlp.ExportResponse{}, err | ||
| } | ||
| case "application/json": | ||
| err = exportResponse.UnmarshalJSON(responseBytes) | ||
| if err != nil { | ||
| return pmetricotlp.ExportResponse{}, err | ||
| } | ||
| } |
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 think this must be
| exportResponse := pmetricotlp.NewExportResponse() | |
| switch httpResponse.Header.Get("Content-Type") { | |
| case "application/x-protobuf": | |
| err = exportResponse.UnmarshalProto(responseBytes) | |
| if err != nil { | |
| return pmetricotlp.ExportResponse{}, err | |
| } | |
| case "application/json": | |
| err = exportResponse.UnmarshalJSON(responseBytes) | |
| if err != nil { | |
| return pmetricotlp.ExportResponse{}, err | |
| } | |
| } | |
| exportResponse := pmetricotlp.NewExportResponse() | |
| switch httpResponse.Header.Get("Content-Type") { | |
| case "protobuf": | |
| if err := exportResponse.UnmarshalProto(responseBytes); err != nil { | |
| return pmetricotlp.ExportResponse{}, err | |
| } | |
| case "json": | |
| if err := exportResponse.UnmarshalJSON(responseBytes); err != nil { | |
| return pmetricotlp.ExportResponse{}, err | |
| } | |
| } |
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.
The contentType is from the http response's header.
So, we cannot use it like that.
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
Summary
### Question- I checked other plugins' directories, but is it a coding convention to keep everything within a single file? There's too much logic, so I want to split the file.Solved: #17997 (comment)
Local Test
otel.yaml
docker run --rm -it -v "$(pwd)/otel.yaml":/etc/otel/config.yaml -p 4318:4318 --name otel-collector otel/opentelemetry-collector:latest --config /etc/otel/config.yamlChecklist
Related issues