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

Add support for JSON formatted logs #4044

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Add support for JSON formatted logs #4044

merged 1 commit into from
Aug 7, 2023

Conversation

chagui
Copy link
Contributor

@chagui chagui commented Jul 20, 2023

Description

This pull-request introduce a configuration option to set the log format to JSON. The option is available both via the config file and CLI arguments.

Solves #3133

How to use it

config file entry

[log]
  format = "json"

CLIs argument:

  • buildkitd --log-format json
  • buildctl --log-format json

Notes

Original work from @moeghanem and @benjamin010095. I forked #3824 and squashed the commits into a single one.
See also:

@chagui chagui changed the title Add support for JSON formatted logs (#3133) Add support for JSON formatted logs Jul 20, 2023
@chagui chagui changed the title Add support for JSON formatted logs Add support for JSON formatted logs (#3133) Jul 20, 2023
@chagui chagui force-pushed the master branch 2 times, most recently from 460b516 to 5623955 Compare July 20, 2023 14:48
@chagui chagui marked this pull request as draft July 20, 2023 14:48
@chagui chagui force-pushed the master branch 3 times, most recently from a7f6b61 to 1814f85 Compare July 20, 2023 15:35
@chagui chagui marked this pull request as ready for review July 20, 2023 15:50
Copy link
Member

@jedevc jedevc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for carrying this @chagui 🎉 Left a couple comments 👍

cmd/buildkitd/main.go Outdated Show resolved Hide resolved
cmd/buildkitd/main.go Outdated Show resolved Hide resolved
@jedevc jedevc requested a review from AkihiroSuda July 20, 2023 15:59
@moeghanem
Copy link

Thanks for carrying this @chagui 🎉 Left a couple comments 👍

Yes, thanks a lot @chagui. This completely slipped my mind 😅

@jedevc jedevc requested a review from crazy-max July 21, 2023 15:18
Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For buildkitd I wonder if we should have a LogConfig struct with Format attr instead of just LogFormat so we could extend log config in the future such as caller bool that would add file:line of the caller to log output. I was also thinking of a level one as well that would be overloaded by Debug if set.

In TOML config it would look like:

[log]
  level = "info"
  format = "text"
  caller = false

cmd/buildkitd/config/config.go Outdated Show resolved Hide resolved
@chagui
Copy link
Contributor Author

chagui commented Jul 22, 2023

I really like the idea, I will update the code accordingly.

@chagui chagui force-pushed the master branch 2 times, most recently from 6fe07a0 to a878aa8 Compare July 22, 2023 21:20
@@ -27,6 +27,11 @@ type Config struct {
DNS *DNSConfig `toml:"dns"`

History *HistoryConfig `toml:"history"`

// LogFormat is the format of the logs. It can be "json" or "text".
Log struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid nested struct and create a dedicated type LogConfig like HistoryConfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
Out of curiosity what is the rational for avoiding nested struct?
I saw the Workers nested struct and thought to do the same because Log would be a simple one.

docs/buildkitd.toml.md Outdated Show resolved Hide resolved
docs/buildkitd.toml.md Outdated Show resolved Hide resolved
@jedevc
Copy link
Member

jedevc commented Jul 26, 2023

PTAL @crazy-max

@jedevc jedevc linked an issue Jul 27, 2023 that may be closed by this pull request
@crazy-max crazy-max changed the title Add support for JSON formatted logs (#3133) Add support for JSON formatted logs Aug 7, 2023
@jedevc jedevc merged commit 8952b70 into moby:master Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for JSON formatted logs
4 participants