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

Added support for JSON based logging #11133

Merged
merged 1 commit into from May 14, 2020
Merged

Conversation

mvisonneau
Copy link
Contributor

@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

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

Please set the appropriate release note label.

@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@coveralls
Copy link

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
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.

pkg/logging/logging.go Outdated Show resolved Hide resolved
pkg/logging/logging.go Outdated Show resolved Hide resolved
Signed-off-by: Maxime VISONNEAU <maxime.visonneau@gmail.com>
Comment on lines -243 to -247
switch os.Getenv("INITSYSTEM") {
case "SYSTEMD":
fileFormat.FullTimestamp = true
default:
fileFormat.TimestampFormat = time.RFC3339
Copy link
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
Contributor 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
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
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 on lines -243 to -247
switch os.Getenv("INITSYSTEM") {
case "SYSTEMD":
fileFormat.FullTimestamp = true
default:
fileFormat.TimestampFormat = time.RFC3339
Copy link
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
Member

aanm commented May 12, 2020

test-me-please

@aanm aanm merged commit 454eae1 into cilium:master May 14, 2020
1.8.0 automation moved this from In progress to Merged May 14, 2020
@mvisonneau mvisonneau deleted the logging/json branch May 14, 2020 12:32
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
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

6 participants