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

Change gRPC client code to use grpc.NewClient #738

Open
nadiamoe opened this issue Jun 21, 2024 · 1 comment
Open

Change gRPC client code to use grpc.NewClient #738

nadiamoe opened this issue Jun 21, 2024 · 1 comment
Assignees

Comments

@nadiamoe
Copy link
Member

nadiamoe commented Jun 21, 2024

grpc.Dial and grpc.DialContext are deprecated, although they will be supported throughout v1.x.x.

We use this to dial the API gRPC server during the agent initialization. The suggested way to do this is to replace this:

return grpc.DialContext(ctx, addr, opts...)

With something such as:

return grpc.NewClient(addr, opts...)

Or:

con, err := grpc.NewClient(addr, opts...)
if err != nil {
    return nil, fmt.Errorf("building client: %w", err)
}
con.Connect() // Although this does not block waiting for conn, and therefore never returns error
return con, nil

But this modifies our current behavior where we block and force the connection with the API before returning from the dialAPIServer function. This seems to be an anti pattern and we should just expect the connection to be established during the first rpc call, and be able to handle connection errors as a result of that call, which I believe we already do.
So, we can probably remove the blocking behavior from main and just expect the first gRPC service call to connect, because the agent will try to fetch data from the API when starting, that will happen early when initializing also, but I'm keeping this PR open by now to review this better.

Originally posted by @ka3de in #671 (comment)

Introduced in: #706

@nadiamoe
Copy link
Member Author

This also applies to the proxy here: https://github.com/grafana/sm-proxy/pull/106

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants