-
Notifications
You must be signed in to change notification settings - Fork 349
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
feat: add support for structured logs #1246
Conversation
This commit introduces a custom logger that includes a timestamp. It distinguishes between info and error log lines using stdout and stderr. For convenience, it is possible to override the logger which is useful for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cmd/root.go
Outdated
@@ -78,6 +80,11 @@ type Command struct { | |||
httpPort string | |||
} | |||
|
|||
// SetLogger overrides the default logger. | |||
func (c *Command) SetLogger(l log.Logger) { | |||
c.conf.Logger = l |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a field on Command instead of in conf?
Conf might dictate how it's set up, but seems to make more sense as a higher level field (c.Logger rather than calling c.conf.Logger)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's better. It also means we have to explicitly pass the logger as a parameter to the proxy.NewClient method, but that also seems better.
cmd/root.go
Outdated
cmd.Version = versionString | ||
|
||
cmd.Use = "cloud_sql_proxy instance_connection_name..." | ||
cmd.Short = "cloud_sql_proxy provides a secure way to authorize connections to Cloud SQL." | ||
cmd.Long = `The Cloud SQL Auth proxy provides IAM-based authorization and encryption when | ||
connecting to Cloud SQL instances. It listens on a local port and forwards connections | ||
to your instance's IP address, providing a secure connection without having to manage | ||
any client SSL certificates.`, | ||
Args: func(cmd *cobra.Command, args []string) error { | ||
err := parseConfig(cmd, c.conf, args) | ||
if err != nil { | ||
return err | ||
} | ||
// The arguments are parsed. Usage is no longer needed. | ||
cmd.SilenceUsage = true | ||
return nil | ||
}, | ||
RunE: func(*cobra.Command, []string) error { | ||
return runSignalWrapper(c) | ||
}, | ||
any client SSL certificates.` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we could still declare these inline during initialization on L101
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry -- trying to make the relationship between cobra.Command and our command more clear here. This probably didn't help. I've cleaned it up.
internal/log/log.go
Outdated
// Infof is for reporting informational messages. | ||
Infof(format string, args ...interface{}) | ||
// Errorf is for reporitng errors. | ||
Errorf(format string, args ...interface{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about DEBUG, WARNING, or FATAL log levels? Where did we land on adding those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided to introduce a debug flag that would enable additional debug logging. Fatal and Warning are out for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought I had an issue for this already filed, but didn't. So I created one here: #1255.
internal/proxy/proxy.go
Outdated
dialer cloudsql.Dialer | ||
|
||
// mnts is a list of all mounted sockets for this client | ||
mnts []*socketMount | ||
|
||
logger log.Logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this say a reference to cmd? What if cmd's logger changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think our contract here is that prior to calling Execute
, the logger can change, but after, the command has started and the configuration is set. Side note: Hot reloading will probably create a new command (saving some state from the previous command) and if the logger changes, it will still be done before the next call to Execute
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll want hotloading to preserve the existing connections, so my 2c is that the cmd will need to be updated and adapted instead. But we can handle that in a different PR if need be.
Looks like we lost some logging around which authentication method is in use. I'll restore it here. |
internal/proxy/proxy.go
Outdated
dialer cloudsql.Dialer | ||
|
||
// mnts is a list of all mounted sockets for this client | ||
mnts []*socketMount | ||
|
||
logger log.Logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll want hotloading to preserve the existing connections, so my 2c is that the cmd will need to be updated and adapted instead. But we can handle that in a different PR if need be.
internal/proxy/proxy.go
Outdated
} | ||
|
||
// NewClient completes the initial setup required to get the proxy to a "steady" state. | ||
func NewClient(ctx context.Context, cmd *cobra.Command, conf *Config) (*Client, error) { | ||
func NewClient(ctx context.Context, l log.Logger, conf *Config) (*Client, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure this shouldn't stay a ref to command?
@@ -32,3 +32,11 @@ type Dialer interface { | |||
|
|||
io.Closer | |||
} | |||
|
|||
// Logger is the interface used throughout the project for logging. | |||
type Logger interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a public interface now right? Does this mean it's in line for a breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(if so we may want to consider adding all the interfaces we want now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is public. I've added Debugf
to the interface to support #1255. Since this interface is meant for google-internal usage only, I think we could technically break it by adding additional methods if necessary. But between info, error, and debug, we have everything we need now.
} | ||
|
||
// NewClient completes the initial setup required to get the proxy to a "steady" state. | ||
func NewClient(ctx context.Context, cmd *cobra.Command, conf *Config) (*Client, error) { | ||
func NewClient(ctx context.Context, d cloudsql.Dialer, l cloudsql.Logger, conf *Config) (*Client, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'm still at of a bit of a loss as to why this is preferable to just passing the cmd in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm passing in only what is used at the moment. Also, avoiding cmd
prevents a reference cycle in the design. Nonetheless, if we want to change this, we're well within our rights to do so since this is an internal
package.
No description provided.