Add time stamps to log entries and use text payload #535
Conversation
|
/lgtm /dogscience |
|
This PR is to address the issue #528 It is worth noting that the beginning of each log file contains config information and does not have timestamps. For actual log entries, timestamps are included. Let me know if additional info should be recorded as well, or if different format is preferred. |
|
the date is probably unnecessary (time is enough) ? |
|
/lgtm cancel //PR changed after LGTM, removing LGTM. @chxchx @ldemailly |
chxchx
changed the title
|
Since stackdriver aggregates logs from many pods, you should add pod IP into prefix as well. |
|
ps: please avoid force pushing, it's not great to have what I approved or commented on just disapear |
|
PTAL Turns out istio-ingress is not a good example to give, since the timestamps and severity etc are included in the payload directly, which is not the case for other logs on mixer, discovery, prometheus, etc. The following is an example from mixer.log. Also, when each log entry was recorded to stackdriver, the pod IP was not included with the log, so when I fetch from stackdriver there is no way for me to access such information. If the IP is super important we might want to put it in log payload when the log is generated, but that would be out of the scope of this PR. |
|
PTAL |
tests/e2e/framework/testInfo.go
Outdated
| @@ -186,7 +186,8 @@ func (t testInfo) FetchAndSaveClusterLogs(namespace string) error { | |||
| for _, logEntry := range entries { | |||
| timestamp := logEntry.GetTimestamp() | |||
| fmtTime := time.Unix(timestamp.Seconds, 0) | |||
| log := fmt.Sprintf("[%s] %s", fmtTime, logEntry.GetTextPayload()) | |||
| log := fmt.Sprintf("[%d:%d:%d] %s", | |||
There was a problem hiding this comment.
don't you need %02d here ? what happens when the h/m/s are < 10 ?
tests/e2e/framework/testInfo.go
Outdated
| log := fmt.Sprintf("[%02d:%02d:%02d] %s", | ||
| fmtTime.Hour(), fmtTime.Minute(), fmtTime.Second(), logEntry.GetTextPayload()) | ||
| if log[len(log)-1:] != "\n" { | ||
| log += "\n" |
There was a problem hiding this comment.
is it really the case that sometime there is already a \n and some times there isn't ? it may be quite expensive and maybe should be split
into (assuming f is buffered and not taking a lock each time)
f.WriteString(fmt.Sprintf("[%02d:%02d:%02d] ",fmtTime.Hour(), fmtTime.Minute(), fmtTime.Second())
txt := logEntry.GetTextPayload()
f.WriteString(txt))
if len(txt) == 0 || log[len(log)-1] != '\n' {
f.WriteByte('\n')
}
but I would check logEntry.GetTextPayload() ? and try to avoid the if (maybe always add the newline
There was a problem hiding this comment.
Yeah I was amazed by the heterogeneity in logs. The payload is formatted different and some has newline and the end while some don't. I do see the point of taking advantage of the file write buffer since concatenation on string essentially claims additional memory and copy everything over. I did take a look on the WriteString() source code and see no locks. I assume copy-on-write until we close the file to commit our writes.
|
@chxchx should we merge this ? (/why not?) |
|
@ldemailly Yes. It is ready. |
|
I meant why didn't you click :-) now it needs rebase again |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chxchx, ldemailly, sebastienvas The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
/test all |
|
/retest |
|
/test all |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
/test all |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
* Access text payload and remove extra newline * Pretty Print Timestamps * Timestamp excludes the date * Zero pads single digit * No concat but write to file buffer * handle err from closing file Former-commit-id: 7f3fd71
* Access text payload and remove extra newline * Pretty Print Timestamps * Timestamp excludes the date * Zero pads single digit * No concat but write to file buffer * handle err from closing file Former-commit-id: 7f3fd71
* Access text payload and remove extra newline * Pretty Print Timestamps * Timestamp excludes the date * Zero pads single digit * No concat but write to file buffer * handle err from closing file Former-commit-id: 7f3fd71
This will help with issues where things are not reproducible
Co-authored-by: maistra-bot <null>
Release note: