Docker time stamps as metric time stamps #7434
Conversation
| @@ -311,6 +316,37 @@ func (d *DockerLogs) tailContainerLogs( | |||
| } | |||
| } | |||
|
|
|||
| func parseLogEntry(rawMessage []byte, acc *telegraf.Accumulator, containerID *string) (time.Time, []byte) { | |||
| var message []byte | |||
| switch length := len(rawMessage); { | |||
There was a problem hiding this comment.
In this function use bytes.SplitN(line, []byte(" "), 2). if there aren't 2 splits return an error, otherwise you are good to parse and return (time, message, err)
There was a problem hiding this comment.
Looks pretty good, but I think theres is one edge case here, when the message is empty, I mean that it is only timestamp without any trailing whitespace, it is not an error. So in this case we need to return ts, empty message, and nil error. The caller function will make a decision either to emit empty message as a telegraf metric or not.
Moreover, I think that we should emit empty messages, as that is what container produced, but right now, this is thrown away. What is your opinion?
There was a problem hiding this comment.
Let's emit the empty string as the message.
There was a problem hiding this comment.
Updated as discussed
| message, err = r.ReadString('\n') | ||
| message, err = r.ReadBytes('\n') | ||
| //Parsing timestamp and message | ||
| messageTs, message = parseLogEntry(message, &acc, &containerID) |
There was a problem hiding this comment.
Change this function to support returning an error, if an error is returned you can acc.AddError(err) and skip processing the line.
| @@ -52,7 +52,12 @@ type Response struct { | |||
| func (r *Response) Close() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func getFirst(p ...interface{}) interface{} { | |||
There was a problem hiding this comment.
Define this like:
func MustParse(layout, value string) time.Time {
tm, err := time.Parse(layout, value)
if err != nil {
panic(err)
}
return tm
}
danielnelson
added
the
feat
|
Opened a PR against your fork, can you take a look: devopsext#1 |
Change error flow
| } | ||
|
|
||
| return timeStamp, message, nil | ||
| return ts, message, nil |
There was a problem hiding this comment.
@danielnelson , This cause copying the content of message, initially the function was returning []byte that is cheaper from this perspective (especially respecting the possible long lines of output from multiple containers). Moreover, later, the message is trimmed with TrimRightFunc(message, unicode.IsSpace), that again involve building new string. Should we revert back this to using []bytes until calling acc.AddFields? What do you think?
There was a problem hiding this comment.
Let's move TrimRightFunc into this function and apply the bytes version of it before converting the message to a string. The conversion/copy must happen at some point, its still best to do it here, but we just want to do it once.
There was a problem hiding this comment.
Adjusted as discussed
Required for all PRs:
This PR comes from #7068, the first part - Use docker timestamps as metric timestamp.
Logs are streamed with timestamps, time stamps parsed from log message and added as metric timestamp. No custom read logic implemented, as stdcopy.StdCopy appears to remove headers from messages while demultiplexing that is a perfect fit.
Readme is not updated as nothing was changed in the plugin configuration interface.