grafana / synthetic-monitoring-agent

Synthetic Monitoring Agent
https://grafana.com/docs/grafana-cloud/how-do-i/synthetic-monitoring/
Apache License 2.0
155 stars 20 forks source link

Change gRPC client code to use `grpc.NewClient` #738

Open roobre opened 1 week ago

roobre commented 1 week ago

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: https://github.com/grafana/synthetic-monitoring-agent/blob/83f8603977c4785e611fa381a14eea933b80826b/cmd/synthetic-monitoring-agent/grpc.go#L43 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 https://github.com/grafana/synthetic-monitoring-agent/issues/671#issuecomment-2039564231

Introduced in: https://github.com/grafana/synthetic-monitoring-agent/pull/706

roobre commented 1 week ago

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