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

Stdout and Stderr Loggers #150

Merged
merged 6 commits into from
Sep 21, 2018
Merged

Stdout and Stderr Loggers #150

merged 6 commits into from
Sep 21, 2018

Conversation

krak3n
Copy link
Contributor

@krak3n krak3n commented Mar 2, 2018

Related issues: #106 #138 #149

We noticed that debug messages from our Cloud SQL proxy would be considered as errors in stack driver since all log messages are sent to stderr as is defined by the default logger in the go log package: https://golang.org/src/log/log.go?#L73

This Pull requests sets up two new loggers, one for verbose / info messages which log to stdout and one for error messages which log to stderr.

I believe this should fix the problems as described in #138

@googlebot
Copy link

Thanks for your pull request. t looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
  • Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
  • The email used to register you as an authorized contributor must also be attached to your GitHub account.

@krak3n
Copy link
Contributor Author

krak3n commented Mar 2, 2018

I've signed it!

@googlebot
Copy link

CLAs look good, thanks!

@Carrotman42
Copy link
Contributor

People likely rely on the current implementation. In order to not break people, please flag off this feature so that the default functionality is unchanged. If this is really what most people want, we can consider changing the default in a later release.

@krak3n
Copy link
Contributor Author

krak3n commented Mar 2, 2018

@Carrotman42 I have add a flag with a default false value.


// LogDebugToStdout updates Verbose and Info logging to use stdout instead of stderr.
func LogDebugToStdout() {
logger := log.New(os.Stdout, "", log.LstdFlags)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is best way to do this 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty torn on this one too. I don't like that cloud_sql_proxy.go now has two different styles of overriding the logging style.

IMO it would probably be better if there was a "LogVerboseToNowhere" method in this package, and have line 394 of cloud_sql_proxy.go call that instead. Would you mind making that one last change?

@d-winter
Copy link

d-winter commented Apr 5, 2018

Any idea when this will get merged?

@Carrotman42
Copy link
Contributor

Carrotman42 commented Apr 5, 2018 via email

@chrissound
Copy link

+1 anxiously waiting for this..

@@ -119,6 +120,9 @@ General:
WARNING: this option disables ALL logging output (including connection
errors), which will likely make debugging difficult. The -quiet flag takes
precedence over the -verbose flag.
-log_debug_stdout
When explicitly set to true, verbose and info log messages will be directed
stdout as a pose to the default stderr.
Copy link
Contributor

Choose a reason for hiding this comment

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

'to stdout as opposed to the default stderr'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed a fix

@kYann
Copy link

kYann commented Apr 17, 2018

👍 Need this too !

version = flag.Bool("version", false, "Print the version of the proxy and exit")
verbose = flag.Bool("verbose", true, "If false, verbose output such as information about when connections are created/closed without error are suppressed")
quiet = flag.Bool("quiet", false, "Disable log messages")
logDebugStdout = flag.Bool("log_debug_stdout", false, "If true, log messages that are not errors will outout to stdout instead of stderr")
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to not catch this earlier, but it says "outout" instead of output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to turn spell check on. Pushed a fix 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, and sorry for the nitpicking ;)

@ricardocabral
Copy link

Do you know any workarounds?

adding "2>&1" as a last argument to the deployment command didn't help.

@hfwang
Copy link
Contributor

hfwang commented Apr 21, 2018

@krak3n I think we're good to go. Would you mind doing one last "little change" so that changing logging settings is a bit more consistent?

Otherwise it looks good to me, so if you don't want to make that change, I can do it myself.

@krak3n
Copy link
Contributor Author

krak3n commented Apr 21, 2018 via email


// Verbosef is called to write verbose logs, such as when a new connection is
// established correctly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix these comments you deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Carrotman42 sorry this took so long, I've pushed a fix 👍

@gytisgreitai
Copy link

@krak3n are you planning to proceed with this?

@krak3n
Copy link
Contributor Author

krak3n commented Jun 18, 2018

@gytisgreitai yes I do, I'll try and get the fixes requested in ASAP, life / work is currently very busy 😬

@kurtisvg kurtisvg added Status: Changes Requested type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Jul 16, 2018
@hfwang
Copy link
Contributor

hfwang commented Sep 21, 2018

LGTM. Sorry for the delay, I didn't realize there had been updates pushed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants