sdjournal: add GetEntry method to retrieve all fields #164

glerchundi · 2016-05-26T21:33:03Z

Implements the same semantics as the JOURNAL_FOREACH_DATA_RETVAL macro in journal-internal.h and produces a *JournalEntry with every field with its corresponding data in a map[string]string. Also it includes address fields like realtime (__REALTIME_TIMESTAMP), monotonic (__MONOTONIC_TIMESTAMP and cursor (__CURSOR).

It mimics more or less journalctl -o json.

Fixes #142
Supersedes #136

wrouesnel · 2016-05-26T22:44:16Z

Looks good to me : should it support JsonEncode to produce a matching output?

glerchundi · 2016-05-30T20:57:49Z

@wrouesnel sounds reasonable but I would vote not to do so. I think json encoding is out of this package scope (although in my use-case would be a really helpful shortcut...)

WDYT on this @lucab @jonboulle?

lucab · 2016-05-30T21:51:50Z

Personally, I also think json encoding is out of scope here and easily doable in the consuming application. (I just had a quick glance at code, haven't reviewed yet)

glerchundi · 2016-05-30T22:05:33Z

(I just had a quick glance at code, haven't reviewed yet)

Ok, thanks!

lucab · 2016-06-03T09:12:28Z

sdjournal/journal_test.go

+	if got != want {
+		t.Fatalf("Bad result for entry.Fields[\"MESSAGE\"]: got %s, want %s", got, want)
+	}
+}


This test is fragile, it will easily pick up messages from other tests and get confused. I think you should instead filter on something a bit more specific (for example introducing a custom field key with a random entry, and matching on that).

Maybe filter by _PID (though beware concurrent tests).

lucab · 2016-06-03T09:34:22Z

Some remarks inline. I think the test definitely needs to be reworked, I already saw it failing several times here (it is a pity this can't be run on Travis).

lucab · 2016-06-03T14:21:53Z

@ wrouesnel @wolfeidau: as original supporters of this, do you have any other concerns with it? Does it cover your usecases (minus the json part)?

wrouesnel · 2016-06-03T14:23:07Z

None from me!

glerchundi · 2016-06-04T09:26:52Z

@lucab PTAL, i fixed your comments and reworked the test using a custom field with a timestamp as a random string, WDYT?

lucab · 2016-06-04T20:18:39Z

@glerchundi Thanks, I think it is much better now. I'll take another pass at this, just to double-check the C-casting part. In the meanwhile, would you mind word-wrapping the commit message as described in the contributing page and add a "fixes" line for #142?

glerchundi · 2016-06-05T08:33:13Z

@lucab done, I referenced the issue in the commit, PTAL whenever you have time, thanks :)

glerchundi · 2016-06-05T08:45:19Z

I reread every comments and splitted this PR in two commits, one for the implementation itself and the other for facilitating access to the fields themselves.

lucab · 2016-06-07T10:57:09Z

I think you just hit an unrelated bug here, as most of the time the test fails for me with:

--- FAIL: TestJournalGetEntry (0.00s)
        journal_test.go:188: Error reading to journal: EOF

Hardcoding the match filter works, though, so I'm wondering if the bug lies somewhere else in the match subsystem. Is the test working fine on your system? Which systemd version are you running?

glerchundi · 2016-06-07T14:11:23Z

@lucab wow, that's weird, I run the test against two environments, Debian and Ubuntu (both inside virtual machines):

glerchundi@debian:~/.go/src/github.com/coreos/go-systemd$ ./test 
Running tests...
ok      github.com/coreos/go-systemd/activation 6.788s  coverage: 0.0% of statements
?       github.com/coreos/go-systemd/journal    [no test files]
ok      github.com/coreos/go-systemd/login1 0.005s  coverage: 41.9% of statements
ok      github.com/coreos/go-systemd/machine1   0.006s  coverage: 65.0% of statements
ok      github.com/coreos/go-systemd/unit   0.003s  coverage: 93.7% of statements
ok      github.com/coreos/go-systemd/sdjournal  6.948s  coverage: 45.2% of statements
Checking gofmt...
Checking govet...
Success

glerchundi@debian:~/.go/src/github.com/coreos/go-systemd$ uname -a
Linux debian 3.16.0-4-amd64 #1 SMP Debian 3.16.7-ckt25-2 (2016-04-08) x86_64 GNU/Linux

glerchundi@debian:~/.go/src/github.com/coreos/go-systemd$ cat /etc/os-release 
PRETTY_NAME="Debian GNU/Linux 8 (jessie)"
NAME="Debian GNU/Linux"
VERSION_ID="8"
VERSION="8 (jessie)"
ID=debian
HOME_URL="http://www.debian.org/"
SUPPORT_URL="http://www.debian.org/support"
BUG_REPORT_URL="https://bugs.debian.org/"

glerchundi@debian:~/.go/src/github.com/coreos/go-systemd$ systemctl --version
systemd 215
+PAM +AUDIT +SELINUX +IMA +SYSVINIT +LIBCRYPTSETUP +GCRYPT +ACL +XZ -SECCOMP -APPARMOR

glerchundi@ubuntu:~/.go/src/github.com/coreos/go-systemd$ ./test
Running tests...
ok      github.com/coreos/go-systemd/activation 5.222s  coverage: 0.0% of statements
?       github.com/coreos/go-systemd/journal    [no test files]
ok      github.com/coreos/go-systemd/login1 0.005s  coverage: 41.9% of statements
ok      github.com/coreos/go-systemd/machine1   0.009s  coverage: 65.0% of statements
ok      github.com/coreos/go-systemd/unit   0.006s  coverage: 93.7% of statements
ok      github.com/coreos/go-systemd/sdjournal  6.490s  coverage: 45.0% of statements
Checking gofmt...
Checking govet...
Success

glerchundi@ubuntu:~/.go/src/github.com/coreos/go-systemd$ uname -a
Linux ubuntu 4.2.0-30-generic #36-Ubuntu SMP Fri Feb 26 00:58:07 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux

glerchundi@ubuntu:~/.go/src/github.com/coreos/go-systemd$ cat /etc/os-release 
NAME="Ubuntu"
VERSION="15.10 (Wily Werewolf)"
ID=ubuntu
ID_LIKE=debian
PRETTY_NAME="Ubuntu 15.10"
VERSION_ID="15.10"
HOME_URL="http://www.ubuntu.com/"
SUPPORT_URL="http://help.ubuntu.com/"
BUG_REPORT_URL="http://bugs.launchpad.net/ubuntu/"

glerchundi@ubuntu:~/.go/src/github.com/coreos/go-systemd$ systemctl --version
systemd 225
+PAM +AUDIT +SELINUX +IMA +APPARMOR +SMACK +SYSVINIT +UTMP +LIBCRYPTSETUP +GCRYPT +GNUTLS +ACL +XZ -LZ4 +SECCOMP +BLKID -ELFUTILS +KMOD -IDN

And it worked on both...., which systemd version are you using?

glerchundi · 2016-06-07T15:09:45Z

@lucab can you try if doing some retries, in this case 5 (I made this in the same way your reading follower does), works on your machine? Maybe journald is not emitting the result that fast to the sd_* subsystem and we're facing a race condition here.

lucab · 2016-06-07T15:31:11Z

@glerchundi yes I think I was hitting the R/W race you describe, just retrying one more time seems to reliably pass the test. I have no better proposals here, so I'll merge it as is.
LGTM

glerchundi · 2016-06-07T15:32:17Z

@lucab ok, wait a minute I slightly changed just for your use case, i'll rever to the last time you reviewed.

lucab · 2016-06-07T15:37:06Z

@glerchundi ah, no hurry then. I actually think you can leave the retry there, as the test is inherently racy.

glerchundi · 2016-06-07T15:40:30Z

Ah, I reverted already because I'm quite sure once the EOF (r==0) is emitted next wait will fail so if you don't care I think it's better to keep it as simple as possible and think in a better solution, WDYT?

…elds Implements the same semantics as the JOURNAL_FOREACH_DATA_RETVAL macro in journal-internal.h and produces a *JournalEntry with every field with its corresponding data in a map[string]string. Also it includes address fields like realtime (__REALTIME_TIMESTAMP), monotonic (__MONOTONIC_TIMESTAMP and cursor (__CURSOR). It mimics more or less `journalctl -o json`. Fixes coreos#142 Supersedes coreos#136

As GetEntry outputs every field in a string keyed map, it eases access to a concrete field without hardcoding it. Ref: https://www.freedesktop.org/software/systemd/man/systemd.journal-fields.html

glerchundi · 2016-06-07T15:48:04Z

@lucab should be mergeable now, recleaned commits, squashed, rebased and that git hell.

lucab · 2016-06-07T15:55:36Z

@glerchundi sorry for the confusion 😄. Yes, it is ok to land this as is and then explore other issues/races separately. I just double-checked the commits in this branch and they seem ok now. May I proceed?

glerchundi · 2016-06-07T15:58:36Z

Yes please!

glerchundi · 2016-06-07T16:01:53Z

🎉 🎉

Thanks for your time @lucab

glerchundi mentioned this pull request May 26, 2016

Add journal.GetDataMap to retreive the full key-value map of an entry. #136

Closed

glerchundi force-pushed the add-getjournalentry-function branch from 0cfaefa to eacd850 Compare May 26, 2016 21:38

lucab added kind/enhancement needs/review component/journal labels May 30, 2016

lucab reviewed Jun 3, 2016
View reviewed changes

lucab self-assigned this Jun 3, 2016

lucab added needs/rework and removed needs/review labels Jun 3, 2016

lucab mentioned this pull request Jun 3, 2016

sdjournal: Enumerate the fields #142

Closed

glerchundi force-pushed the add-getjournalentry-function branch from eacd850 to 975031c Compare June 4, 2016 09:22

glerchundi force-pushed the add-getjournalentry-function branch from 975031c to 559bbee Compare June 5, 2016 08:24

glerchundi force-pushed the add-getjournalentry-function branch from 559bbee to 8bea57f Compare June 5, 2016 08:37

glerchundi force-pushed the add-getjournalentry-function branch from c8324a7 to 3b78080 Compare June 5, 2016 08:46

lucab added needs/review and removed needs/rework labels Jun 7, 2016

glerchundi force-pushed the add-getjournalentry-function branch 3 times, most recently from 8562d27 to ed33ef2 Compare June 7, 2016 15:08

lucab added reviewed/lgtm and removed needs/review labels Jun 7, 2016

glerchundi force-pushed the add-getjournalentry-function branch from ed33ef2 to a443091 Compare June 7, 2016 15:33

glerchundi force-pushed the add-getjournalentry-function branch from a443091 to 896af86 Compare June 7, 2016 15:41

glerchundi added 2 commits June 7, 2016 17:43

sdjournal: export all user, trusted and address journal fields

7fd54aa

As GetEntry outputs every field in a string keyed map, it eases access to a concrete field without hardcoding it. Ref: https://www.freedesktop.org/software/systemd/man/systemd.journal-fields.html

glerchundi force-pushed the add-getjournalentry-function branch from 896af86 to 7fd54aa Compare June 7, 2016 15:45

lucab changed the title ~~sdjournal: add GetEntry method to retrieve all fields plus address fields~~ sdjournal: add GetEntry method to retrieve all fields Jun 7, 2016

lucab merged commit ff577b2 into coreos:master Jun 7, 2016

glerchundi deleted the add-getjournalentry-function branch June 7, 2016 16:05

lucab mentioned this pull request Jun 20, 2016

sdjournal: possible race in Match/Send/Wait/Next flow #180

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdjournal: add GetEntry method to retrieve all fields #164

sdjournal: add GetEntry method to retrieve all fields #164

glerchundi commented May 26, 2016 •

edited by lucab

wrouesnel commented May 26, 2016

glerchundi commented May 30, 2016

lucab commented May 30, 2016

glerchundi commented May 30, 2016

lucab Jun 3, 2016

tv42 Jun 3, 2016 •

edited

lucab commented Jun 3, 2016

lucab commented Jun 3, 2016

wrouesnel commented Jun 3, 2016

glerchundi commented Jun 4, 2016

lucab commented Jun 4, 2016

glerchundi commented Jun 5, 2016

glerchundi commented Jun 5, 2016

lucab commented Jun 7, 2016

glerchundi commented Jun 7, 2016

glerchundi commented Jun 7, 2016

lucab commented Jun 7, 2016

glerchundi commented Jun 7, 2016

lucab commented Jun 7, 2016 •

edited

glerchundi commented Jun 7, 2016

glerchundi commented Jun 7, 2016

lucab commented Jun 7, 2016

glerchundi commented Jun 7, 2016

glerchundi commented Jun 7, 2016

sdjournal: add GetEntry method to retrieve all fields #164

sdjournal: add GetEntry method to retrieve all fields #164

Conversation

glerchundi commented May 26, 2016 • edited by lucab

wrouesnel commented May 26, 2016

glerchundi commented May 30, 2016

lucab commented May 30, 2016

glerchundi commented May 30, 2016

lucab Jun 3, 2016

Choose a reason for hiding this comment

tv42 Jun 3, 2016 • edited

Choose a reason for hiding this comment

lucab commented Jun 3, 2016

lucab commented Jun 3, 2016

wrouesnel commented Jun 3, 2016

glerchundi commented Jun 4, 2016

lucab commented Jun 4, 2016

glerchundi commented Jun 5, 2016

glerchundi commented Jun 5, 2016

lucab commented Jun 7, 2016

glerchundi commented Jun 7, 2016

glerchundi commented Jun 7, 2016

lucab commented Jun 7, 2016

glerchundi commented Jun 7, 2016

lucab commented Jun 7, 2016 • edited

glerchundi commented Jun 7, 2016

glerchundi commented Jun 7, 2016

lucab commented Jun 7, 2016

glerchundi commented Jun 7, 2016

glerchundi commented Jun 7, 2016

glerchundi commented May 26, 2016 •

edited by lucab

tv42 Jun 3, 2016 •

edited

lucab commented Jun 7, 2016 •

edited