Skip to content
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.

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

capnslog: Set some defaults, propose a CoreOS formatter #28

Merged
merged 1 commit into from
Aug 10, 2015

Conversation

barakmich
Copy link
Contributor

Chasing #26.

This is a little bold, but somebody's got to take a position and go for consensus. Here's some thoughts on sane defaults and a real CoreOS formatter.

The notion is as follows: we have a common log format, for which I'm proposing this:

    INFO | 2015-06-11 15:17:16.3707 | dolly: Hello Dolly
 WARNING | 2015-06-11 15:17:16.3707 | dolly: Well hello, Dolly
   ERROR | 2015-06-11 15:17:16.3707 | main: It's so nice to have you back where you belong
    INFO | 2015-06-11 15:17:16.3708 | You're still glowin', you're still crowin', you're still lookin' strong
CRITICAL | 2015-06-11 15:17:16.3708 | main: Dolly'll never go away again

But wait, there's more! A built-in debug flag to the CoreOS formatter yields the following:

    INFO | 2015-06-11 15:21:46.0527 [proc.go:63] | dolly: Hello Dolly
 WARNING | 2015-06-11 15:21:46.0528 [proc.go:63] | dolly: Well hello, Dolly
   ERROR | 2015-06-11 15:21:46.0528 [proc.go:63] | main: It's so nice to have you back where you belong
    INFO | 2015-06-11 15:21:46.0528 [proc.go:63] | You're still glowin', you're still crowin', you're still lookin' strong
CRITICAL | 2015-06-11 15:21:46.0528 [proc.go:63] | main: Dolly'll never go away again

The notion being that a new standard --debug flag for CoreOS apps can (a) drop the log level to debug or trace and (b) start tracing code lines (which is expensive in normal operation).

I designed the format for greppability and visual cues, without using ANSI color.

@xiang90
Copy link

xiang90 commented Jun 11, 2015

@barakmich Maybe move the time to the first col?

@marineam
Copy link
Contributor

Oh my. whatever the default formatter is it is going to make its way into journal logs which already has timestamps and so on, making them heck to read (etcd has long had this problem). I'm fine with providing a verbose format for when running a tool outside of systemd/journal but otherwise it is a load of noise. :(

@philips
Copy link

philips commented Jun 11, 2015

Yes, please no timestamps, "levels", etc if we detect we are running under systemd. Ideally capnlog will detect these cases and write to the journal instead with the Send method: https://github.com/coreos/go-systemd/blob/master/journal/send.go#L61

@philips
Copy link

philips commented Jun 11, 2015

Just to illustrate here is what this would look like once it lands in the journal:

Jun 11 07:47:03 c1 unknown:     INFO | 2015-06-11 15:17:16.3707 | dolly: Hello Dolly
Jun 11 07:47:03 c1 unknown:  WARNING | 2015-06-11 15:17:16.3707 | dolly: Well hello, Dolly
Jun 11 07:47:03 c1 unknown:    ERROR | 2015-06-11 15:17:16.3707 | main: It's so nice to have you back where you belong
Jun 11 07:47:03 c1 unknown:     INFO | 2015-06-11 15:17:16.3708 | You're still glowin', you're still crowin', you're still lookin' strong
Jun 11 07:47:03 c1 unknown: CRITICAL | 2015-06-11 15:17:16.3708 | main: Dolly'll never go away again

@barakmich
Copy link
Contributor Author

That's fine -- I'm happy to write a syslog style formatter (in fact, I have already) -- if you actually switch it to running on syslog, then it will "Do The Right Thing"

This is for replacing the text output, which exists today in terms of log and os.Stderr.

What's really terrible is that what's useful in systemd is not at all useful on the command line or through normal output -- which log assumes as well. :(

@barakmich
Copy link
Contributor Author

I might make another package/folder in this repo with coreos defaults -- but I want to create a great logging package here, and then we can deal with systemd :P

@xiang90
Copy link

xiang90 commented Jun 11, 2015

@marineam @philips I think the default should not be optimized for systemd. But I agree that we can/should probably detect it and use a better format (or we can have a format for the user to set if he/she runs under systemd)

@crawford
Copy link
Contributor

Why wouldn't we build this with systemd in mind? We use systemd! The whole point of this package is to make logging easier for us.

@yichengq
Copy link

Maybe we could have a format that is especially designed for running softwares through systemd in CoreOS, which helps reach the best UX.

@crawford IMO, this logging package is to uniform the logging of company-wide softwares, which do not always run in systemd.

@crawford
Copy link
Contributor

It's fine that we don't always run under CoreOS/systemd, but that is the default case. Why wouldn't our logging library reflect that fact?

@jonboulle
Copy link
Contributor

shameless +1

On Thu, Jun 11, 2015 at 3:04 PM, Alex Crawford [email protected]
wrote:

It's fine that we don't always run under CoreOS/systemd, but that is the
default case. Why wouldn't our logging library reflect that fact?


Reply to this email directly or view it on GitHub
#28 (comment).

@barakmich
Copy link
Contributor Author

Alright, let's make everyone happy:

If we're running under PID 1, we're pretty much running as a daemon or systemd unit. Let's speak natively to systemd, if we can (sysvinit will not see it as enabled and default to dev-mode):

Jun 12 15:01:52 titania example[10495]: Hello Dolly
Jun 12 15:01:52 titania example[10495]: Well hello, Dolly
Jun 12 15:01:52 titania example[10495]: It's so nice to have you back where you belong
Jun 12 15:01:52 titania example[10495]: You're still glowin', you're still crowin', you're still lookin' strong
Jun 12 15:01:52 titania example[10495]: Dolly'll never go away again
Jun 12 15:01:52 titania systemd[1]: pidecho.service: main process exited, code=exited, status=1/FAILURE

(You can't see the bold and the red, but yay, native log level translation)

But if you're running as a dev, you get the usual style above. (Or, incidentally, under docker, I would imagine which does its own process tree and log collection)

Win/win. What do you think?

(I'm not included in Semaphore though I have an account -- i added go-systemd as a dependency, and it needs to be fetched, to make the build green)

/cc @jonboulle @crawford @xiang90 @philips

@crawford
Copy link
Contributor

Sounds good.

@xiang90
Copy link

xiang90 commented Jun 12, 2015

@barakmich Sounds good to me

@yifan-gu
Copy link

yifan-gu commented Jul 2, 2015

Build fails?

@chancez
Copy link
Contributor

chancez commented Jul 6, 2015

@yifan-gu Needs dependencies which @barakmich can't add via go-get due to no permissions in semaphore.

@xiang90
Copy link

xiang90 commented Aug 6, 2015

@barakmich Any progress on this one?

@barakmich
Copy link
Contributor Author

Made changes. Still treats systemd correctly. Semaphore green. PTAL.

@chancez
Copy link
Contributor

chancez commented Aug 7, 2015

My only concern is we call one of these the CoreOS formatter yet the real CoreOS formatter is basically the journald one, so I don't see why the CoreOS one is labeled what it is. I understand it's probably what we want if running on our Mac laptops, just naming seems off.

Otherwise LGTM

@marineam
Copy link
Contributor

marineam commented Aug 7, 2015

Maybe PrettyFormatter?

@xiang90
Copy link

xiang90 commented Aug 10, 2015

Maybe PrettyFormatter?

Sounds good.

@philips
Copy link

philips commented Aug 10, 2015

+1 on renaming CoreOSFormatter to PrettyFormatter

@barakmich
Copy link
Contributor Author

Done

@xiang90
Copy link

xiang90 commented Aug 10, 2015

LGTM

barakmich added a commit that referenced this pull request Aug 10, 2015
capnslog: Set some defaults, propose a CoreOS formatter
@barakmich barakmich merged commit 6b7d4a6 into coreos:master Aug 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants