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

suppress logging from repo server #206

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

krancour
Copy link
Member

@alexmt you might be the best person to review this.

This is a prereq for akuity/kargo#951

@krancour krancour self-assigned this Oct 23, 2023
@krancour krancour requested a review from a team as a code owner October 23, 2023 20:48
@netlify
Copy link

netlify bot commented Oct 23, 2023

Deploy Preview for docs-kargo-render-akuity-io ready!

Name Link
🔨 Latest commit 1730c4f
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-render-akuity-io/deploys/6536fd5a94b5ed0008c5d684
😎 Deploy Preview https://deploy-preview-206.bookkeeper.akuity.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2c78951) 38.28% compared to head (1730c4f) 38.28%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #206   +/-   ##
=======================================
  Coverage   38.28%   38.28%           
=======================================
  Files          19       19           
  Lines        1157     1157           
=======================================
  Hits          443      443           
  Misses        697      697           
  Partials       17       17           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

In addition to surpressing logging as a default, which I think we should do, another common technique that I think we should do is logging to stderr instead of stdout. The parseable output would continue going to stdout. In fact, this is what kubectl does.

So you could do:

kubectl get pods -o json -v=6

The pod json would be to stdout, but the log messages would go to stderr:

I1023 15:37:26.181218   12126 loader.go:374] Config loaded from file:  /Users/jesse/.kube/config
I1023 15:37:26.937120   12126 round_trippers.go:553] GET https://mycluster.com/api/v1/namespaces/default/pods?limit=500 200 OK in 749 milliseconds

I'll approve this PR for the log level reduction. Feel free to create a different issue or PR for switching to stderr for log output.

@krancour
Copy link
Member Author

another common technique that I think we should do is logging to stderr instead of stdout. The parseable output would continue going to stdout

I had that thought as well. Since you like it also, yes... I'll open an issue for it.

@krancour krancour enabled auto-merge (squash) October 23, 2023 23:11
@krancour krancour merged commit a767b23 into akuity:main Oct 23, 2023
10 checks passed
@krancour krancour deleted the krancour/limit-logging branch February 2, 2024 16:21
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.

2 participants