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

logs: setting --log='file-defaults: {format: json} does not impact the format of cockroach-stderr.log #58968

Closed
thtruo opened this issue Jan 13, 2021 · 7 comments
Assignees
Labels
A-kv-server Relating to the KV-level RPC server A-logging In and around the logging infrastructure. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@thtruo
Copy link
Contributor

thtruo commented Jan 13, 2021

Observation
When I run a command like cockroach start-single-node --certs-dir=certs --log='file-defaults: {format: json}' to set the logging format to json, I notice that cockroach-stderr.log continues to be in crdb-v1 format.

Expectation
When I run a command like cockroach start-single-node --certs-dir=certs --log='file-defaults: {format: json}', I expect cockroach-stderr.log to be structured as json as well, not crdb-v1.

Repro steps

  • Run cockroach start-single-node --certs-dir=certs --log='file-defaults: {format: json}'
  • cd into cockroach-data/ and run tail cockroach.log | jq . which outputs JSON logs ✅
  • Run tail cockroach-pebble.log | jq . which outputs JSON logs ✅
  • Run tail cockroach-stderr.log | jq . which does NOT output JSON logs. These logs are in crdb-v1 format
@thtruo thtruo added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-kv-server Relating to the KV-level RPC server A-logging In and around the logging infrastructure. labels Jan 13, 2021
@thtruo
Copy link
Contributor Author

thtruo commented Jan 13, 2021

cc @piyush-singh

@knz
Copy link
Contributor

knz commented Jan 14, 2021

cockroach-stderr.log is not an output sink for log events; it's an output file for things that are unrelated to logging really.

It's an oddity due to some unfixable problems in the Go runtime system.

We need to find a way to drive the idea home that it's not a log file like the others. Meanwhile we can't drive attention away from this file entirely because it does contain useful details sometimes.

Maybe we could rename it?

@knz
Copy link
Contributor

knz commented Jan 14, 2021

In any case, the fact it's different must be called out in documentation properly. @thtruo how do you suggest we pick up the special semantics of this file effectively in explanations, etc?

@thtruo
Copy link
Contributor Author

thtruo commented Jan 15, 2021

Ack. Agreed that this should be called out in documentation cc @taroface FYI a good discussion topic

That's helpful context @knz. I see your perspective around the need to keep that file in some way. I don't have strong opinions around how we name it, but here's one idea. Perhaps removing the cockroach- prefix from cockroach-stderr.log can help illustrate that point for a user? My line of thinking is - since every other log file has that prefix, it implies they are similar in the sense they carry relevant log events. stderr.log on the other hand is basically the catchall for everything else and naming it differently could help drive that distinction home.

Whether we rename or not, at the very least, we'll have this explained in docs.

@thtruo
Copy link
Contributor Author

thtruo commented Jan 15, 2021

Nvm, my suggestion about removing the cockroach- prefix does not make sense. It was helpful for me to review that portion of your RFC again. I wonder if by default calling it cockroach-strayerr.log would make that clearer?

Looking at the cockroach debug check-log-config --log='file-defaults: {format: json}' command again makes it clear what's happening: format:json doesn't get applied anyway to things that get sent into the stderr log file.
Image 2021-01-15 at 10 32 01 AM

Sounds like this issue is not really a functional bug but an area for potential user confusion. I'm okay with closing this if you are cc @knz

@knz
Copy link
Contributor

knz commented Jan 15, 2021

I think at this point it is mostly a doc project and there's no clear change engineering-wise.

However if we need to make a callout about this in docs, it won't be sufficient to rely on the stream of past release notes. I would recommend creating a specific docs issue instead.

@knz knz changed the title logs: setting --log='file-defaults: {format: json} should result in a json structured cockroach-stderr.log logs: setting --log='file-defaults: {format: json} does not impact the format of cockroach-stderr.log Jan 15, 2021
@thtruo
Copy link
Contributor Author

thtruo commented Jan 15, 2021

Ack. Closing this. Tracking the relevant docs issue here cockroachdb/docs#9344

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-kv-server Relating to the KV-level RPC server A-logging In and around the logging infrastructure. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

No branches or pull requests

2 participants