feat: add support for structured logs by enocom · Pull Request #1246 · GoogleCloudPlatform/cloud-sql-proxy

enocom · 2022-06-22T21:32:03Z

No description provided.

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.

hessjcg

LGTM

kurtisvg · 2022-06-23T15:10:41Z

cmd/root.go


+// SetLogger overrides the default logger.
+func (c *Command) SetLogger(l log.Logger) {
+	c.conf.Logger = l


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)

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.

kurtisvg · 2022-06-23T15:11:17Z

cmd/root.go

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


Seems like we could still declare these inline during initialization on L101

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.

kurtisvg · 2022-06-23T15:12:50Z

internal/log/log.go

+	// Infof is for reporting informational messages.
+	Infof(format string, args ...interface{})
+	// Errorf is for reporitng errors.
+	Errorf(format string, args ...interface{})


How about DEBUG, WARNING, or FATAL log levels? Where did we land on adding those?

We decided to introduce a debug flag that would enable additional debug logging. Fatal and Warning are out for now.

Thought I had an issue for this already filed, but didn't. So I created one here: #1255.

kurtisvg · 2022-06-23T15:15:25Z

internal/proxy/proxy.go

 	// mnts is a list of all mounted sockets for this client
 	mnts []*socketMount
+
+	logger log.Logger


Shouldn't this say a reference to cmd? What if cmd's logger changes?

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.

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.

enocom · 2022-07-01T20:21:20Z

Looks like we lost some logging around which authentication method is in use. I'll restore it here.

cmd/root.go

kurtisvg · 2022-07-07T02:37:36Z

internal/proxy/proxy.go

 	// mnts is a list of all mounted sockets for this client
 	mnts []*socketMount
+
+	logger log.Logger


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.

kurtisvg · 2022-07-07T02:38:05Z

internal/proxy/proxy.go


 // 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) {


Are we sure this shouldn't stay a ref to command?

kurtisvg · 2022-07-07T18:06:10Z

cloudsql/cloudsql.go

 }
+
+// Logger is the interface used throughout the project for logging.
+type Logger interface {


This looks like a public interface now right? Does this mean it's in line for a breaking change?

(if so we may want to consider adding all the interfaces we want now)

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.

kurtisvg · 2022-07-07T18:08:29Z

internal/proxy/proxy.go


 // 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) {


nit: I'm still at of a bit of a loss as to why this is preferable to just passing the cmd in

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.

This is a port of GoogleCloudPlatform/cloud-sql-proxy#1246

enocom added 2 commits June 22, 2022 12:02

feat: use custom logger with timestamps

f7d6b97

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.

feat: add support for structured logs

034f98d

enocom requested a review from a team June 22, 2022 21:32

hessjcg approved these changes Jun 22, 2022

View reviewed changes

kurtisvg suggested changes Jun 23, 2022

View reviewed changes

enocom added 3 commits July 1, 2022 13:40

Merge branch 'v2' into structured-logs

b54f9fa

Store logger on Command

2773a4e

Reduce the diff slightly

e9ecb39

enocom requested a review from kurtisvg July 1, 2022 20:19

Restore log message about auth methods

96521fd

kurtisvg suggested changes Jul 7, 2022

View reviewed changes

enocom added 3 commits July 7, 2022 11:18

Merge branch 'v2' into structured-logs

2fb102d

Make Logger and Dialer configurable on Command

1bcadcd

Remove SetLogger method

bee7829

kurtisvg approved these changes Jul 7, 2022

View reviewed changes

enocom added 5 commits July 7, 2022 12:33

Merge remote-tracking branch 'origin/v2' into structured-logs

c1e5977

Add Debugf to exported interface

d9801d5

Typo

030b942

Sort interface by increasing level

eec5ba0

Fix flaky test

32fb526

enocom merged commit ee913ce into v2 Jul 7, 2022

enocom deleted the structured-logs branch July 7, 2022 20:27

enocom added a commit to GoogleCloudPlatform/alloydb-auth-proxy that referenced this pull request Jul 12, 2022

feat: add support for structured logs

188f268

This is a port of GoogleCloudPlatform/cloud-sql-proxy#1246

enocom mentioned this pull request Jul 12, 2022

feat: add support for structured logs GoogleCloudPlatform/alloydb-auth-proxy#66

Merged

enocom added a commit to GoogleCloudPlatform/alloydb-auth-proxy that referenced this pull request Jul 12, 2022

feat: add support for structured logs (#66)

5a1e8e8

This is a port of GoogleCloudPlatform/cloud-sql-proxy#1246

enocom added a commit that referenced this pull request Jul 12, 2022

feat: add support for structured logs (#1246)

ed8391d

enocom added a commit that referenced this pull request Jul 27, 2022

feat: add support for structured logs (#1246)

f11355f

enocom added a commit that referenced this pull request Aug 23, 2022

feat: add support for structured logs (#1246)

9683928

enocom added a commit that referenced this pull request Aug 29, 2022

feat: add support for structured logs (#1246)

7c7b8e3

release-please bot mentioned this pull request Aug 29, 2022

chore(main): release 2.0.0-preview.1 #1334

Merged

Conversation

enocom commented Jun 22, 2022

Uh oh!

hessjcg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enocom Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enocom commented Jul 1, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

enocom Jul 1, 2022 •

edited

Loading