Skip to content

journal: fix parsing of the stdout stream #1460

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
evverx wants to merge 1 commit into from
Closed

journal: fix parsing of the stdout stream #1460

evverx wants to merge 1 commit into from

Conversation

@evverx
Copy link
Contributor

@evverx evverx commented Oct 5, 2015

There are two problems:

  • '\0'-bytes
$ printf '\1\2\3\0\3\2\1' | systemd-cat
$ journalctl -e -b _TRANSPORT=stdout
...
Oct 05 08:38:56 jessie-64 cat[25834]: [3B blob data]

$ journalctl -b _PID=25834 -o json-pretty
{
       ...
        "MESSAGE" : [ 1, 2, 3 ],
        ...
}
  • trailing spaces
$ printf "hola!   " | systemd-cat
$ journalctl -e -b _TRANSPORT=stdout
...
Oct 05 08:42:29 jessie-64 cat[25883]: hola!

$ journalctl -b _PID=25883 -o json-pretty
{
        ...
        "MESSAGE" : "hola!",
        ...
}

@zonque zonque added the journal label Oct 5, 2015
@poettering
Copy link
Member

poettering commented Oct 6, 2015

Hmm, I am not convinced this would be a good idea... We actually had this discussion before. I don't think we should guarantee bit-exact storing away of log messages when using stdout or stderr. Making sure leading whitespace is retained is important, so that indenting stays around, but trailing whitespace and empty lines I think we should suppress in order to keep logs minimal.

I mean, we generally reserve the right to suppress messages due to ratelimiting and such, hence I think the best we can do to not even get into the ratelimiting case is suppress as much "noise" as we can, and that includes whitespace at unnecessary places...

I figure the 0-byte issue does matter though, we should retain that like any other non-whitespace character

@poettering poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Oct 6, 2015
@evverx
Copy link
Contributor Author

evverx commented Oct 6, 2015

Making sure leading whitespace is retained is important, so that indenting stays around

I agree

but trailing whitespace and empty lines I think we should suppress in order to keep logs minimal.

I can write empty lines right now:)

$ yes '<1>' | head -5 | systemd-cat

$ journalctl -b _TRANSPORT=stdout PRIORITY=1 -e -o json-pretty
...
{
        ...
        "MESSAGE" : "",
}

Ok. I'll remove trailing spaces and empty lines.

@evverx
journal: fix parsing of the stdout stream
74a2ca6 
* save leading whitespaces
* remove trailing whitespaces
* remove empty lines
* retain '\0' like any other non-whitespace character

See systemd#1460 (comment)
@evverx evverx force-pushed the prevent-data-loss-in-stdout-transport branch from 42025d8 to 74a2ca6 Compare October 7, 2015 00:16
@evverx evverx changed the title journal: prevent data loss in the stdout stream journal: fix parsing of the stdout stream Oct 7, 2015
@evverx
Copy link
Contributor Author

evverx commented Oct 7, 2015

@poettering , fixed

@rumpelsepp rumpelsepp mentioned this pull request Oct 7, 2015
Merged
@evverx
Copy link
Contributor Author

evverx commented Oct 7, 2015

@rumpelsepp, yeah, the stacktrace of go application looks fine:)

Oct 07 10:26:30 jessie-64 go-run-panic[3284]: panic: AAA!
Oct 07 10:26:30 jessie-64 go-run-panic[3284]: goroutine 16 [running]:
Oct 07 10:26:30 jessie-64 go-run-panic[3284]: runtime.panic(0x425900, 0xc208000010)
Oct 07 10:26:30 jessie-64 go-run-panic[3284]:         /usr/lib/go/src/pkg/runtime/panic.c:279 +0xf5
Oct 07 10:26:30 jessie-64 go-run-panic[3284]: main.main()
Oct 07 10:26:30 jessie-64 go-run-panic[3284]:         /root/hw.go:4 +0x61
Oct 07 10:26:30 jessie-64 go-run-panic[3284]: goroutine 17 [runnable]:
Oct 07 10:26:30 jessie-64 go-run-panic[3284]: runtime.MHeap_Scavenger()
Oct 07 10:26:30 jessie-64 go-run-panic[3284]:         /usr/lib/go/src/pkg/runtime/mheap.c:507
Oct 07 10:26:30 jessie-64 go-run-panic[3284]: runtime.goexit()
Oct 07 10:26:30 jessie-64 go-run-panic[3284]:         /usr/lib/go/src/pkg/runtime/proc.c:1445
Oct 07 10:26:30 jessie-64 go-run-panic[3284]: goroutine 18 [runnable]:
Oct 07 10:26:30 jessie-64 go-run-panic[3284]: bgsweep()
Oct 07 10:26:30 jessie-64 go-run-panic[3284]:         /usr/lib/go/src/pkg/runtime/mgc0.c:1976
Oct 07 10:26:30 jessie-64 go-run-panic[3284]: runtime.goexit()
Oct 07 10:26:30 jessie-64 go-run-panic[3284]:         /usr/lib/go/src/pkg/runtime/proc.c:1445
Oct 07 10:26:30 jessie-64 go-run-panic[3284]: exit status 2

@rumpelsepp
Copy link

rumpelsepp commented Oct 7, 2015

Nice one!

@rumpelsepp rumpelsepp mentioned this pull request Oct 7, 2015
Closed
@AudriusButkevicius
Copy link

AudriusButkevicius commented Oct 7, 2015

:shipit:

@evverx evverx mentioned this pull request Nov 16, 2015
Closed
@poettering
Copy link
Member

poettering commented Nov 27, 2015

#1937 has been merged now, hence this one can be closed I figure.

@poettering poettering closed this Nov 27, 2015
@bobrik bobrik mentioned this pull request Jun 1, 2016
Closed
@evverx evverx mentioned this pull request Dec 10, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

journal reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@evverx @poettering @rumpelsepp @AudriusButkevicius @zonque