Skip to content

Added support for JSON based logging#11133

Merged
aanm merged 1 commit intocilium:masterfrom
mvisonneau:logging/json
May 14, 2020
Merged

Added support for JSON based logging#11133
aanm merged 1 commit intocilium:masterfrom
mvisonneau:logging/json

Conversation

@mvisonneau
Copy link
Copy Markdown
Member

@mvisonneau mvisonneau commented Apr 24, 2020

This PR enable support for getting JSON based logs out of the binaries.

It can be set using either

# ciliumd.yaml
log-opt:
  format: json

or --log-opt format=json

There is a slight issue related to some log lines that get written before the logger gets actually configured. I reckon it must also be the case when log-driver is set to syslog or the level is different than the default though.

Added support for logging in JSON format

@maintainer-s-little-helper
Copy link
Copy Markdown

Commit 3de339d71371ce8a7714b2f9c59686e67a5697ff does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper Bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 24, 2020
@maintainer-s-little-helper
Copy link
Copy Markdown

Please set the appropriate release note label.

@maintainer-s-little-helper
Copy link
Copy Markdown

Please set the appropriate release note label.

@coveralls
Copy link
Copy Markdown

coveralls commented Apr 24, 2020

Coverage Status

Coverage increased (+0.007%) to 44.459% when pulling b40fad1 on mvisonneau:logging/json into 9f32546 on cilium:master.

@tgraf tgraf added pending-review release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Apr 24, 2020
@maintainer-s-little-helper maintainer-s-little-helper Bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 4, 2020
@mvisonneau mvisonneau marked this pull request as ready for review May 4, 2020 16:56
@mvisonneau mvisonneau requested a review from a team May 4, 2020 16:56
@mvisonneau mvisonneau requested a review from a team as a code owner May 4, 2020 16:56
Copy link
Copy Markdown
Member

@joestringer joestringer 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 the contribution! I have a couple of minor comments below on the code.

Comment thread pkg/logging/logging.go Outdated
Comment thread pkg/logging/logging.go Outdated
Signed-off-by: Maxime VISONNEAU <maxime.visonneau@gmail.com>
@mvisonneau mvisonneau requested a review from a team May 5, 2020 13:08
Comment thread pkg/logging/logging.go
Comment on lines -243 to -247
switch os.Getenv("INITSYSTEM") {
case "SYSTEMD":
fileFormat.FullTimestamp = true
default:
fileFormat.TimestampFormat = time.RFC3339
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the rational behind this removal?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

my former comment on this got removed, my bad. I assumed this statement was not actually used given that the logrus.Formatter is configured with an hardcoded value for disabling the timestamp 🤔 DisableTimestamp = true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well that explains stuff... and I thought it was because of the detection of os.Getenv("INITSYSTEM"). @borkmann you usually run cilium-agent standalone. Did you ever miss the lack of timestamps?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nope, not yet so far. ;)

Comment thread pkg/logging/logging.go
Comment on lines -243 to -247
switch os.Getenv("INITSYSTEM") {
case "SYSTEMD":
fileFormat.FullTimestamp = true
default:
fileFormat.TimestampFormat = time.RFC3339
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well that explains stuff... and I thought it was because of the detection of os.Getenv("INITSYSTEM"). @borkmann you usually run cilium-agent standalone. Did you ever miss the lack of timestamps?

@aanm aanm requested a review from joestringer May 12, 2020 20:31
@aanm
Copy link
Copy Markdown
Member

aanm commented May 12, 2020

test-me-please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/minor This PR changes functionality that users may find relevant to operating Cilium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants