chore: improve logging with additional test coverage #1937

annafang-google · 2023-09-01T15:34:18Z

Let proxy continue to run even instances specified are inactive.
Tested these scenarios:

inactive instances without TCP or Unix socket options
inactive instances with TCP port
inactive instances with Unix socket

enocom · 2023-09-01T15:41:48Z

internal/proxy/proxy.go

-				}
-			}
-			return nil, fmt.Errorf("[%v] Unable to mount socket: %v", inst.Name, err)
+			fmt.Errorf("[%v] Unable to mount socket: %v", inst.Name, err)


We need to either log this error, or return it. Right now, this is creating an error value and discarding it.

If there is only one instance, should we still fail the proxy?

Replaced it with logger.Errorf(). Thanks!
Considering the scenario where this one instance was accidentally stopped, having the proxy run will save a minor effort of restarting proxy after the instance is activated. It's not a huge saving, but the behaviour could be consistent.

internal/proxy/proxy.go

enocom · 2023-09-01T15:45:14Z

internal/proxy/proxy_test.go

@@ -306,6 +309,39 @@ func TestClientInitialization(t *testing.T) {
 				filepath.Join(testUnixSocketPathPg),
 			},
 		},
+		{
+			desc: "without TCP port or unix socket for non functional instance",
+			in: &proxy.Config{


The test below is build around wantTCPAddrs or wantUnixAddrs. So as written, this test will never fail. To ensure we have the behavior we want here, we should add a functioning socket with a non functioning one and ensure we get what we expect with wantTCPAddrs or wantUnixAddrs.

The fake dialer's EngineVersion() added a fakeserver case to mimic the failed instance case. EngineVersion() call will fail in the NewClient() call. What the test does is to make sure that NewClient() will proceed upon the failure of EngineVersion().

enocom · 2023-09-01T15:45:54Z

internal/proxy/proxy_test.go

+					},
+				},
+			},
+			wantUnixAddrs: []string{


Should the fake instance result in a Unix socket?

In the case of the auto-reconnecting the instance, it might be good to keep the socket?

enocom · 2023-09-01T19:44:01Z

A couple of high level thoughts:

It is possible to specify a port to allow the Proxy to run with inactive instances. In fact, we recommend production workloads do this anyway to speed up the initial startup (and avoid waiting on the metadata call)
It is not possible to do this with Unix sockets presently because we must know if an instance is postgres or not, and right now the Go Connector will fail if an instance isn't up (source). We could split that call into two parts with some work, but perhaps the value of this is less high.
It's probably best to just skip failed Unix sockets with an error message and call it a day given we already support this feature otherwise.

WDYT?

annafang-google · 2023-09-05T15:29:37Z

Makes sense. Updated the PR that fails the unix path if the instance is not active.

enocom · 2023-09-05T21:42:35Z

internal/proxy/proxy.go

@@ -747,6 +747,14 @@ type socketMount struct {
 	dialOpts []cloudsqlconn.DialOption
 }

+func getNetworkType(conf *Config, inst InstanceConnConfig) string {


Let's make this networkType. Cf. https://go.dev/doc/effective_go#Getters

Fixed. Thanks!

enocom · 2023-09-05T21:43:50Z

internal/proxy/proxy.go

 		}

 		l.Infof("[%s] Listening on %s", inst.Name, m.Addr())
 		mnts = append(mnts, m)
 	}
 	c.mnts = mnts
-	return c, nil
+	if len(mnts) > 0 {


Let's swap this with the error below, so the error path is indented.

https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

Fixed. Thanks!

enocom · 2023-09-05T21:46:37Z

internal/proxy/proxy_test.go

+		},
+	}
+
+	unixTcs := []struct {


Shall we move this one test case into a dedicated test below? e.g., TestProxyInitializationWithFailedUnixSocket?

Good idea to make the test more explicit! Fixed

enocom · 2023-09-05T21:52:24Z

internal/proxy/proxy.go

@@ -801,6 +807,8 @@ func (c *Client) newSocketMount(ctx context.Context, conf *Config, pc *portConfi

 		address, err = newUnixSocketMount(inst, conf.UnixSocket, strings.HasPrefix(version, "POSTGRES"))
 		if err != nil {
+			c.logger.Errorf("could not mount unix socket %q for %q: %v",


Let's consolidate all logging up above in NewClient.

I see that we're not doing that consistently above. We should confirm that we're not logging this error twice, though.

As discussed offline, removed the NewClient logs and kept logs at the newSocketMount level.

internal/proxy/proxy.go

internal/proxy/proxy_test.go

Replaced fmt.Errorf with logger.Errorf for instance failure Fail the inactive instance with unix path Fixed lint error Added inactive instance test with unix path Replace getNetworkType with networkType Fixed the error path indentation Make the unix path failure test a seperate func Fail the proxy binary if the tcp socket argument is wrong

enocom

Key point: let's add support here for skipped over failed Unix sockets. Otherwise, we should require users specify a port for TCP sockets to avoid adding any additional complexity here.

internal/proxy/proxy.go

enocom · 2023-09-13T21:30:50Z

internal/proxy/proxy.go

+			switch err.(type) {
+			// this is not the inactive instance case. Fail the proxy
+			// so that the user could fix the proxy command error.
+			case *net.OpError:


In what cases does this error get returned? Could we add a comment clarifying that?

Since this isn't Unix related, I think we shouldn't add it.

Added the comments. This is to handle the case where listen call could fail.

enocom · 2023-09-13T21:33:51Z

internal/proxy/proxy.go

@@ -801,6 +807,8 @@ func (c *Client) newSocketMount(ctx context.Context, conf *Config, pc *portConfi

 		address, err = newUnixSocketMount(inst, conf.UnixSocket, strings.HasPrefix(version, "POSTGRES"))
 		if err != nil {
+			c.logger.Errorf("could not mount unix socket %q for %q: %v",


I see that we're not doing that consistently above. We should confirm that we're not logging this error twice, though.

internal/proxy/proxy.go

…port config

enocom · 2023-09-19T21:56:58Z

internal/proxy/proxy_test.go

+	for _, tc := range tcs {
+		t.Run(tc.desc, func(t *testing.T) {
+			_, err := proxy.NewClient(ctx, &fakeDialer{}, testLogger, tc.in)
+			if tc.wantSuccess != (err == nil) {


Could this be:

if tc.wantSuccess && err != nil { // ... }

For the proposed change, if tc.wantSuccess is false (we want NewClient() to fail), the branch will not be executed regardless what the err is. This will miss reporting the failing case when err is nil and we want NewClient() to fail.

enocom · 2023-09-19T21:57:50Z

internal/proxy/proxy.go

-				mErr := m.Close()
-				if mErr != nil {
-					l.Errorf("failed to close mount: %v", mErr)
+			switch err.(type) {


Instead of switching on error type, could we just continue if the socket type is a Unix socket (like what we had before)?

It will miss the case when the type is TCP socket and there is no active instance. Correct me if I'm wrong, my understanding from the discussion offline:

Inactive instance with unix socket should fail

Inactive instance with TCP socket with no port number configured should fail.

annafang-google · 2023-09-22T20:46:22Z

As discussed offline, keep the logic to fail the proxy if the proxy is unable to connect to inactive instances. Retain the logging to keep it consistent.
The recommended way to keep the proxy running is to specify the port number in the configuration for TCP case. The unix socket case however, because the proxy has no way to identify if the db is postgres (for inactive instance), the code will fail the proxy in this case.

annafang-google requested a review from a team as a code owner September 1, 2023 15:34

blunderbuss-gcf bot assigned jackwotherspoon Sep 1, 2023

enocom requested changes Sep 1, 2023

View reviewed changes

annafang-google force-pushed the non-fatal branch from 6b2cd86 to fd02e36 Compare September 1, 2023 18:16

annafang-google force-pushed the non-fatal branch 3 times, most recently from cff7808 to db06ac9 Compare September 5, 2023 15:24

enocom reviewed Sep 5, 2023

View reviewed changes

annafang-google force-pushed the non-fatal branch 3 times, most recently from 722c84a to 30238d6 Compare September 6, 2023 14:45

annafang-google force-pushed the non-fatal branch from 30238d6 to a77165f Compare September 7, 2023 02:26

enocom reviewed Sep 13, 2023

View reviewed changes

enocom self-requested a review September 13, 2023 21:37

annafang-google added 2 commits September 19, 2023 01:40

Make consistent error logging and fail the inactive instance without …

2c7bb3f

…port config

Added an example of the case where listen call fails

31e096c

enocom assigned enocom and unassigned jackwotherspoon Sep 19, 2023

enocom reviewed Sep 19, 2023

View reviewed changes

Revert the logic change for failing the inactive instances

0d28012

enocom changed the title ~~fix: let proxy continue to run with inactive instance(s) (#1854)~~ chore: improve logging with additional test coverage Sep 22, 2023

enocom self-requested a review September 22, 2023 21:06

enocom approved these changes Sep 22, 2023

View reviewed changes

annafang-google merged commit d88b907 into main Sep 25, 2023
12 checks passed

annafang-google deleted the non-fatal branch September 25, 2023 00:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: improve logging with additional test coverage #1937

chore: improve logging with additional test coverage #1937

annafang-google commented Sep 1, 2023

enocom Sep 1, 2023

enocom Sep 1, 2023

annafang-google Sep 1, 2023

enocom Sep 1, 2023

annafang-google Sep 1, 2023

enocom Sep 1, 2023

annafang-google Sep 1, 2023

enocom commented Sep 1, 2023

annafang-google commented Sep 5, 2023

enocom Sep 5, 2023

annafang-google Sep 6, 2023

enocom Sep 5, 2023

annafang-google Sep 6, 2023

enocom Sep 5, 2023

annafang-google Sep 6, 2023

enocom Sep 5, 2023

enocom Sep 13, 2023

annafang-google Sep 20, 2023

enocom left a comment

enocom Sep 13, 2023

enocom Sep 13, 2023

annafang-google Sep 19, 2023

enocom Sep 13, 2023

enocom Sep 19, 2023

annafang-google Sep 20, 2023

enocom Sep 19, 2023

annafang-google Sep 20, 2023

annafang-google commented Sep 22, 2023 •

edited

Loading

chore: improve logging with additional test coverage #1937

chore: improve logging with additional test coverage #1937

Conversation

annafang-google commented Sep 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enocom commented Sep 1, 2023

annafang-google commented Sep 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enocom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annafang-google commented Sep 22, 2023 • edited Loading

annafang-google commented Sep 22, 2023 •

edited

Loading