Rework OTel logs handler to be spec compliant, refactor for clarity #751
Conversation
Previously we had code that suffered from two main problems: - It used the global "http.HandleFunc" method, instead of specific serverMux structs (this is a problem if multiple servers are running) - It did not respect context cancelation correctly, likely leading to Go routine leaks and other issues when the collector reloads
This was copied from the ListenAndServe method, but isn't needed because we know that an address is present.
The OTLP protocol spec requires that we return a response object (encoded based on request content type) that indicates how many of the submitted log records were rejected. Further it requires us to return a 4XX error if we could not process the request. Any error thats not 429 (rate limited) must not be retried by the client, per the spec. In passing, refactor the OTel logs handler to reduce duplication when dealing with very similar yet different cases (jsonlog as the main log record, vs wrapped in Kubernetes context).
keiko713
left a comment
There was a problem hiding this comment.
Some small comments but overall, looking good!
| lc := net.ListenConfig{} | ||
| l, err := lc.Listen(ctx, "tcp", s.Addr) | ||
| if err != nil { | ||
| logger.PrintError("Error starting HTTP server on %s: %v\n", addr, err) |
There was a problem hiding this comment.
One thing it's slightly missing from the previous version is, this error message is too generic and sometimes it'll be hard to tell which part didn't go well. I like that we have this util func so I think it's an acceptable tradeoff, also I can't come up with a better alternative without making things messy, but just figured to mention.
There was a problem hiding this comment.
Yeah, I thought about that, but short of adding a special field or error callback, this is hard to improve (because one of the two errors happens in the Go routine).
| } | ||
|
|
||
| if veryVerbose { | ||
| jsonData, err := json.Marshal(logsData) |
There was a problem hiding this comment.
Looks like you dropped the indent thingy (json.MarshalIndent(logsData, "", " ")), why? Maybe you thought that it'll be too many lines...?
There was a problem hiding this comment.
So I've actually found the pretty-printed version harder to work with in practice - to actually analyze it you'd typically want to copy it out, and the pretty printing makes that harder to do.
That said, I think I'll revert to pretty printing, since it seems that's what we're doing for the other --very-verbose cases, and its better to be consistent here. I can also see the pretty printing working better when we're on a customer call to review a problem.
|
|
||
| if veryVerbose { | ||
| prefixedLogger.PrintVerbose("OTel log server received JSON log data in the following format:\n") | ||
| prefixedLogger.PrintVerbose(string(b)) |
There was a problem hiding this comment.
A bit similar to above, this was previously pretty JSON but now it's a compact one. Is this intentional?
|
|
||
| var resp []byte | ||
| switch r.Header.Get("Content-Type") { | ||
| case "application/x-protobuf": |
There was a problem hiding this comment.
I think you'll also want to specify the content type for the write, which maybe you can do here by doing something like the following?:
w.Header().Set("Content-Type", "application/x-protobuf")
|
|
||
| if err != nil { | ||
| w.WriteHeader(http.StatusBadRequest) | ||
| w.Write([]byte(err.Error())) |
There was a problem hiding this comment.
According to the specification, we should technically:
- use Status as a reply
- we can omit code, but will need message
- then marshall Status with either
protoorprotojson, depending on which content type we received
Plus some minor prep work to help with that, see individual commits.
Follow-up from #747.