container logs should be escaped when printed #2751
Conversation
internal/dao/pod.go
Outdated
| @@ -380,8 +381,8 @@ func readLogs(ctx context.Context, wg *sync.WaitGroup, stream io.ReadCloser, out | |||
| r := bufio.NewReader(stream) | |||
| for { | |||
| var item *LogItem | |||
| if bytes, err := r.ReadBytes('\n'); err == nil { | |||
| item = opts.ToLogItem(bytes) | |||
| if line, err := r.ReadString('\n'); err == nil { | |||
There was a problem hiding this comment.
@wjiec Given we are dealing with logs, I think there will be additional allocations converting bytes->string->bytes. Perhaps we should regex directly the bytes vs hitting the extra allocations?
There was a problem hiding this comment.
@derailed I've thought about this, if regular expression matching is done directly on the bytes it would require copying the code out of the tview, which I think might be a bit repetitive, and could also leave a hidden problem if the tview updates this regular expression afterwards. I chose the safer out of the two possible methods.
There was a problem hiding this comment.
Thanks Jayson! I understand. We are however using a fork and we can control changes if they occurred but that would require a strategy change for region and color tags which is unlikely. On the other hand we're dealing with lots of gc pressure. I think we could perhaps intro a one liner tview.EscapeBytes in the fork?
There was a problem hiding this comment.
@derailed Thank you for your reply, indeed, it is the best way!
wjiec
force-pushed
the
bugfix/escape-log
branch
from
b5d6d4c
to
6ce7e76
Compare
|
@derailed Oops, I'm having problems with the implementation in |
* container logs should be escaped when printed * reduce additional memory allocation * upgrade deps and move escape to tview
this pr fixes #2746.