Migrate from grpc.Dial to grpc.NewClient

[yurishkuro]

In 1.63.x grpc.Dial is being replaced with grpc.NewClient that does not attempt to make a connection. Need to make code fixes to use the new API.

This was originally breaking dependabot PR #5327, but gRPC later un-deprecated the Dial functions. Still, they will likely be deprecated in the future releases, so we need to migrate.

(Update) The only remaining place is

./cmd/jaeger/internal/integration/span_reader.go:45:	cc, err := grpc.DialContext(ctx, ports.PortToHostPort(port), opts...)

Validation:

• make test

[danish9039]

working on it

[tico88612]

It looks like this requires replacing grpc.Dial and grpc.DialContext with grpc.NewClient.

There are two problems encountered during the replacement process:

1. grpc.NewClient will ignore WithTimeout DialOption. (https://pkg.go.dev/google.golang.org/grpc#NewClient) How can I solve the problem about grpc.DialContext (or any possible solutions?)
   here is example
   

   jaeger/cmd/anonymizer/app/query/query.go

   Lines 41 to 44 in 8a4748c

   	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
   	defer cancel()
   	conn, err := grpc.DialContext(ctx, addr, grpc.WithTransportCredentials(insecure.NewCredentials()))

2. In cmd/agent/app/configmanager/grpc/manager_test.go Line 51 if I replace grpc.Dial with grpc.NewClient, should I change Line 58?

=== RUN   TestSamplingManager_GetSamplingStrategy_error
    manager_test.go:58: 
                Error Trace:    /Users/jerry/working/jaeger/cmd/agent/app/configmanager/grpc/manager_test.go:58
                Error:          "rpc error: code = Unavailable desc = name resolver error: produced zero addresses" does not contain "Error while diali
ng: dial tcp: address foo: missing port in address"
                Test:           TestSamplingManager_GetSamplingStrategy_error
--- ❌ FAIL: TestSamplingManager_GetSamplingStrategy_error (0.32s)

jaeger/cmd/agent/app/configmanager/grpc/manager_test.go

Lines 50 to 59 in 8a4748c

	func TestSamplingManager_GetSamplingStrategy_error(t *testing.T) {
	conn, err := grpc.Dial("foo", grpc.WithTransportCredentials(insecure.NewCredentials()))
	defer close(t, conn)
	require.NoError(t, err)
	manager := NewConfigManager(conn)
	resp, err := manager.GetSamplingStrategy(context.Background(), "any")
	require.Nil(t, resp)
	require.Error(t, err)
	assert.Contains(t, err.Error(), "Error while dialing: dial tcp: address foo: missing port in address")
	}

[yurishkuro]

Can you find docs, tickets, or changelog that explains why they replaced Dial with NewClient and what migration path is suggested?

[tico88612]

I have searched for the relevant documents as far as I can.

• 2018's issue: Consider deprecating Dial and DialContext and replacing with a NewClient grpc/grpc-go#1786
• grpc-go anti-pattern: https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md
• chore: expose NewClient method to end users grpc/grpc-go#7010

Unfortunately, I didn't find any information about the migration path, and other grpc-go users weren't notified either.

• No mention in the 1.63.0 release note Deprecation of DialContext and Dial is not mentioned in 1.63.0 release notes grpc/grpc-go#7090
• The relevant documents are still being updated. Update docs and examples and tests to use NewClient instead of Dial grpc/grpc-go#7049

[yurishkuro]

The existing docs do say that WithTimeout should not be used, so it's safe to refactor to use NewClient and drop the timeouts handling during that phase.

To your second question about the error string, my preference for testing errors like that is to wrap them in the business logic, e.g. return fmt.Errorf("our internal message/explanation: %w", err) and then the test should only match on our part of the error string, not on what comes from the library (which could change across versions, as happens this time)

[tico88612]

The current grpc-go version v1.63.x removes the deprecated tags from Dial and DialContext.

• Deprecation of DialContext and Dial is not mentioned in 1.63.0 release notes grpc/grpc-go#7090 (comment)
• v1.63.x: grpc: un-deprecate Dial and DialContext and cherry-pick #7101 grpc/grpc-go#7103

CI has been passed.

• build(deps): bump google.golang.org/grpc from 1.62.1 to 1.63.2 #5342

But grpc-go will add back the deprecated tag in v1.64, I will continue to study how to remove Dial and DialContext to NewClient and fix the related test cases.

[yurishkuro]

May be worth splitting your pr into pieces that are known to work ( verify via go test on individual packages)

[tico88612]

I can change the parts that don't affect the test first, and I'll mark TODO for the others that don't.

[yurishkuro]

Last one #5443