Handle the case where grpc client connections are shared #298
Conversation
dashpole
force-pushed
the
fix_reused_client
branch
from
e637a80
to
d57978a
Compare
| @@ -439,6 +441,12 @@ func (e *Exporter) StopMetricsExporter() { | |||
| func (e *Exporter) Close() error { | |||
| tErr := e.traceExporter.close() | |||
| mErr := e.statsExporter.close() | |||
| // If the trace and stats exporter share client connections, | |||
There was a problem hiding this comment.
This looks like a reasonable local fix, but should the client connection be tracked better? E.g. should there be a reference count?
There was a problem hiding this comment.
In practice, this case shouldn't ever really happen, since you normally don't want to dial the same endpoint for metrics and traces. It just happens in unit tests for the collector, from what I can tell. If we did something like that, we would basically just want to only close one connection if the user provides the WithGRPCConn option, since they connections are different otherwise.
But it would have been a bit ugly to check and track the args that are currently just passed through to the monitoring and trace clients. The other option would've been to add a "metrics-only" exporter constructor function, but that also seemed like too much for fixing unit tests.
The other option would be for whoever creates the connection initially to be responsible for closing it. But in most cases, users don't create the connection themselves, and will still need to invoke Shutdown (in the collector) for other reasons.
The test fails without the change to the Close function.
dashpole
force-pushed
the
fix_reused_client
branch
from
d57978a
to
9101a97
Compare
|
Added a test |
Follow-up to #295. If using an Insecure client with the OpenTelemetry collector, it will use the
WithGRPCConnoption. This grpc connection is then used for both the trace and stats clients. When closing the exporter, the secondClose()call fails witherror(s) closing trace client (<nil>), or metrics client (rpc error: code = Canceled desc = grpc: the client connection is closing), which indicates the connection is already closed. Handle this case by ignoring that error.This is required to fix unit test errors in open-telemetry/opentelemetry-collector-contrib#5990.
The go.sum changes must be left-over from a previous PR that didn't tidy?