Split connection management away from exporter

[krnowak]

This commits adds an ConnectionManager interface, which the exporter
will use to connect to the collector and send both metrics and traces
to it. That way, the Exporter type is free from dealing with any
connection/protocol details, as this business is taken over by the
implementations of the ConnectionManager interface.

The gRPC code from the exporter is moved into the implementation of
ConnectionManager. Currently it only maintains a single connection,
just as the Exporter used to do.

With the split, most of the Exporter options became actually gRPC
connection manager's options. Currently the only option that remained
to be Exporter's is about setting the export kind selector.

[codecov]

Codecov Report

Merging #1369 (1123f58) into master (add9d93) will increase coverage by 0.2%.
The diff coverage is 88.0%.

@@           Coverage Diff            @@
##           master   #1369     +/-   ##
========================================
+ Coverage    78.1%   78.3%   +0.2%     
========================================
  Files         123     125      +2     
  Lines        6252    6259      +7     
========================================
+ Hits         4885    4904     +19     
+ Misses       1119    1111      -8     
+ Partials      248     244      -4     

Impacted Files	Coverage Δ
exporters/otlp/options.go	100.0% <ø> (+44.4%)	⬆️
exporters/otlp/otlp.go	90.9% <75.0%> (+7.3%)	⬆️
exporters/otlp/grpcoptions.go	87.5% <87.5%> (ø)
exporters/otlp/grpcdriver.go	90.1% <90.1%> (ø)
exporters/otlp/grpcconnection.go	85.9% <100.0%> (ø)
sdk/trace/batch_span_processor.go	78.4% <0.0%> (-2.0%)	⬇️
exporters/otlp/internal/transform/metric.go	80.6% <0.0%> (+0.5%)	⬆️

[MrAlias]

I like the overall design of adding the ConnectionManager interface. I'm interested to know what you think about leaving the transform functionality in the exporter though and keeping the new ConnectionManager interface focused on transport.

[krnowak]

@dashpole: Thanks for the review. I'll reply to both your issues here.

NewGRPCSingleConnectionManager implies there is something other than "Single" that we would want to add in the future. Would we want to make the "other" thing we want to add an option instead of a new function, and shorten this to NewGRPCConnectionManager? That would be fine so long as the options would still apply, which I think they should.

I think it is slightly easier to use if this takes ...options instead of a configuration since it doesn't require defining the configuration in a separate statement.

I.E.

func NewGRPCSingleConnectionManager(opts ...GRPCConnectionOption) ConnectionManager

The follow-up PR would introduce the NewGRPCMultiConnectionManager function, hence the Single in the name. The new function would create a connection manager that maintains separate connections for tracing and metrics. Both connections could be configured separately, which leads to a problem with the opts ...GRPCConnectionOption part: I don't see an elegant solution to configuring traces and metrics connection separately with the opts ...GRPCConnectionOption interface, other than duplicating all the options into WithTracesFoo and WithMetricsFoo. So for NewGRPCMultiConnectionManager I went with:

type MultiConnectionConfig struct {
	Traces GRPCConnectionConfig
	Metrics GRPCConnectionConfig
}

func NewGRPCMultiConnectionManager(cfg MultiConnectionConfig) ConnectionManager { … }

So for NewGRPCSingleConnectionManager I went with taking GRPCConnectionConfig as a parameter, instead of opts ...GRPCConnectionConfig for consistency. This is rather inconsistent with the rest of the project, which is something that #536 seeks to resolve.

[dashpole]

This is rather inconsistent with the rest of the project, which is something that #536 seeks to resolve.

Got it. Thanks for linking the issue.

The follow-up PR would introduce the NewGRPCMultiConnectionManager function

That makes sense. This is part of #1121, right?

If there are eventually going to be multiple types of exporters (not just grpc), it might make sense to make a more generic single vs multi connection manager that isn't grpc-specific. For example (with the current naming):

type MultiConnectionConfig struct {
	Traces ConnectionManager
	Metrics ConnectionManager
}

func NewMultiConnectionManager(cfg MultiConnectionConfig) ConnectionManager { … }

[krnowak]

This is rather inconsistent with the rest of the project, which is something that #536 seeks to resolve.

Got it. Thanks for linking the issue.

The follow-up PR would introduce the NewGRPCMultiConnectionManager function

That makes sense. This is part of #1121, right?

Yes.

If there are eventually going to be multiple types of exporters (not just grpc), it might make sense to make a more generic single vs multi connection manager that isn't grpc-specific. For example (with the current naming):

type MultiConnectionConfig struct {
	Traces ConnectionManager
	Metrics ConnectionManager
}

func NewMultiConnectionManager(cfg MultiConnectionConfig) ConnectionManager { … }

I got something like that already: krnowak@f8c2ec8, but maybe your variant with the config is actually better, because you spell you explicitly which manager goes for tracing and which for metrics. So thanks for the idea and we'll see how it fares during the review when it appears in another PR.

[krnowak]

Updated.

[krnowak]

Updated. Added some tests to increase the coverage a bit (increasing coverage for error conditions is rather annoying). And fixed one or two races.

[krnowak]

Before merging it, I'd like to discuss one thing at the Golang SIG meeting first.

[seanschade]

Just added a recommendation for a NewGRPCConnectionConfig function.

[MrAlias]

From the SIG meeting, we are going to not add a single/multiple gRPC connection driver. Instead going to progress to just using the driver interface to support multiple protocols for now.

[jmacd]

This looks good to me, independent of the single- vs multi-driver question, simply for taking us one step closer to an OTLP/HTTP exporter. 😀

[krnowak]

Rebased and updated. Simplified the gRPC driver a bit.

[krnowak]

Updated to fix a conflict in changelog. Updated some tests to be a bit less flaky.

[MrAlias]

Awesome work 👍