Use log timestamps from docker logs. #401
Conversation
|
Hi @localghost |
router/pump.go
Outdated
| @@ -235,6 +235,7 @@ func (p *LogsPump) pumpLogs(event *docker.APIEvents, backlog bool, inactivityTim | |||
| Since: sinceTime.Unix(), | |||
| InactivityTimeout: inactivityTimeout, | |||
| RawTerminal: rawTerminal, | |||
| Timestamps: true, | |||
There was a problem hiding this comment.
set this to true only if this feature is enabled
router/pump.go
Outdated
| @@ -361,10 +362,15 @@ func newContainerPump(container *docker.Container, stdout, stderr io.Reader) *co | |||
| } | |||
| return | |||
| } | |||
| logMessage, logTime, err := parseLogLine(line) | |||
There was a problem hiding this comment.
parseLogLine should only be called if this feature is enabled. Also, if we get an error, perhaps we should fall back to default behaviour of using Time.Now()
router/pump.go
Outdated
| if err != nil { | ||
| return "", time.Time{}, err | ||
| } | ||
| return logEntry[1], logTime, nil |
There was a problem hiding this comment.
there is no guarantee that logEntry[1] exists. You should rework this function to check the length of logEntry
|
@gbolo |
There was a problem hiding this comment.
almost good for for merge. I like that the default behaviour remains during errors. Just review my comment.
@michaelshobbs how does the PR look to you?
| if len(logEntry) == 2 { | ||
| return logEntry[1], logTime | ||
| } | ||
| return "", logTime |
There was a problem hiding this comment.
why do we want to return an empty log line? Shouldn't this also be: return line, time.Now()
There was a problem hiding this comment.
if line has no white spaces - if it had the function would finish at len(logEntry) == 2 - and logEntry[0] is a valid time string - otherwise the function would finish at err != nil then it means that an empty message was logged
|
Hi @localghost I agree with @michaelshobbs. This shouldn't be too hard. Just make a test case that focuses on testing the function |
|
@gbolo, I know, I just don't have time currently. I will update this pr in a few days. Thanks. |
|
@gbolo @michaelshobbs |
|
something changed with the upstream golint repo. i've had to modify other projects to get around this. please fix this in logspout by changing this line: Line 41 in 89c2a08 to |
|
@gbolo @michaelshobbs |
|
It looks like you changed back it to time.Now() . I am searching for a way to getting miliseconds of the dates. Since they are all 0, observing exceptions is hard. Is there a way to do it in updated version? |
|
I have the same problem as @yunusemrecatalcam. What is the workaround for seeing real timestamps with at least millisecond resolution? |
The rationale for this PR is that not preserving original timestamps by logspout makes investigation of issues across multiple services hard.