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

Json output for logs #94

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

nlamirault
Copy link

Close #47

Signed-off-by: Nicolas Lamirault [email protected]

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Close aws#47

Signed-off-by: Nicolas Lamirault <[email protected]>
@jcogilvie
Copy link

Can the maintainers take a look at this please? I would greatly appreciate it.

@codecov-commenter
Copy link

Codecov Report

Merging #94 (8cbc600) into main (f043e56) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #94   +/-   ##
=======================================
  Coverage   56.09%   56.09%           
=======================================
  Files           7        7           
  Lines         574      574           
=======================================
  Hits          322      322           
  Misses        244      244           
  Partials        8        8           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

simonmarty
simonmarty previously approved these changes Aug 30, 2022
@simonmarty simonmarty enabled auto-merge (squash) August 30, 2022 22:54
@simonmarty
Copy link
Contributor

Similar logic in the Secrets Store CSI driver @joebaro

@simonmarty simonmarty added enhancement New feature or request and removed triage/needs-investigation labels Sep 19, 2022
@jcogilvie
Copy link

jcogilvie commented Oct 13, 2022

Bump? I would really love to have consistent, structured logging from this provider. I think to fix the build you may need to construct the logger as done here:

https://github.com/kubernetes/component-base/blob/master/logs/json/klog_test.go#L245

			logger, _ := NewJSONLogger(verbosityLevel, writer, nil, nil)
			klog.SetLogger(logger)

Signed-off-by: Nicolas Lamirault <[email protected]>
auto-merge was automatically disabled October 14, 2022 08:09

Head branch was pushed to by a user without write access

@doron-cohen
Copy link

@nlamirault @simonmarty This feature is important for us since our logging provider fails to parse the logs and marks all of them as errors. Please see if this can be pushed into the next version. It seems pretty done. Thanks.

@danielskowronski
Copy link

Bumping as well, lack of logging in this component makes it impossible to debug some issues (especially networking to things like STS) other than attaching debugger...

@jcogilvie
Copy link

Are we waiting on the maintainer or the author? If the branch just needs to be brought up to date I can probably take a look.

@simonmarty

@simonmarty
Copy link
Contributor

I was waiting for the author to address the comments I left @jcogilvie

@jnogol
Copy link

jnogol commented Nov 10, 2023

bump, this would be incredibly useful and would reduce clutter and spending in our logs.

@flavioelawi
Copy link

@simonmarty Anything we can do if @jcogilvie doesn't get back?

@jcogilvie
Copy link

I'm here but I'm not the author.

@flavioelawi
Copy link

I'm here but I'm not the author.

woopsie sorry I meant to tag @nlamirault :)

@prakashbalaji
Copy link

prakashbalaji commented Jun 6, 2024

We were continuously getting the logs as error in our datadog console. Once we switched the csi driver to json log, the errors went off and the reporting is way cleaner. Hope this PR gets some attention and we have ability to switch to json logs. I don't see review comments in the PR too, do we know what we are waiting for?

Screenshot 2024-06-06 at 2 35 24 PM

@simonmarty

* main: (41 commits)
  Increment helm chart version number
  Update image tag (aws#355)
  Update to Go 1.21 (aws#353)
  Bump golang.org/x/net from 0.20.0 to 0.23.0 (aws#341)
  Helm: fix environment variables location (aws#332)
  Update the version of the image (aws#331)
  Fix support for non-default kubelet root directory (aws#330)
  Increase the version to 0.3.8
  Make throttling params QPS and Burst configurable (aws#323)
  Bump google.golang.org/protobuf from 1.32.0 to 1.33.0 (aws#325)
  Add support for non-default kubelet root directory (aws#322)
  Allow users to configure the underlying AWS SDK to enable FIPS endpoint (aws#324)
  STS VPC endpoint for private clusters (aws#319)
  Update to secrets-store.csi.x-k8s.io/v1 (aws#317)
  Update CODEOWNERS
  Update helm chart to version 0.3.6 (aws#309)
  Move to Go 1.20 (aws#303)
  Version bump (aws#297)
  Update dependencies (aws#296)
  Clarify requirement for EC2 on EKS (aws#275)
  ...

Signed-off-by: Nicolas Lamirault <[email protected]>
@nlamirault nlamirault requested a review from a team as a code owner June 10, 2024 08:13
@prakashbalaji
Copy link

@nlamirault - Any updates on this PR since there was some activity 2 weeks ago?

Signed-off-by: Nicolas Lamirault <[email protected]>
@nlamirault
Copy link
Author

@simonmarty something is missing ?

)

// Main entry point for the Secret Store CSI driver AWS provider. This main
// rountine starts up the gRPC server that will listen for incoming mount
// requests.
func main() {
klog.InitFlags(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

We're concerned about these init flags conflicting with future changes to our repository. If they are not necessary, can you remove them?

Copy link
Author

Choose a reason for hiding this comment

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

but there is in the klog documentation:
Use klog.InitFlags(nil) explicitly for initializing global flags as we no longer use init() method to register the flags

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I'm wrong but we dont use klog.init() anywhere in the first place (i.e. we don't initialize global flags for klog).

)

// Main entry point for the Secret Store CSI driver AWS provider. This main
// rountine starts up the gRPC server that will listen for incoming mount
// requests.
func main() {
klog.InitFlags(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I'm wrong but we dont use klog.init() anywhere in the first place (i.e. we don't initialize global flags for klog).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to configure json logging
10 participants