Use log timestamps from docker logs.#401
Use log timestamps from docker logs.#401localghost wants to merge 8 commits intogliderlabs:masterfrom
Conversation
|
Hi @localghost |
gbolo
left a comment
gbolo
left a comment
There was a problem hiding this comment.
Please re-work this PR. Also add any new feature descriptions to README.md. Also add your change to CHANGELOG.md.
Thanks for your contribution!
router/pump.go
Outdated
| 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
| } | ||
| 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 |
gbolo
left a comment
gbolo
left a comment
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
michaelshobbs
left a comment
michaelshobbs
left a comment
There was a problem hiding this comment.
looks reasonable. needs tests, however
|
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? |
5 participants
The rationale for this PR is that not preserving original timestamps by logspout makes investigation of issues across multiple services hard.