OTLP trace gRPC exporter

[paivagustavo]

Quick explanation:

This introduces otlptrace package, defining a trace Client interface and trace Exporter struct that implements the otel sdk exporter interface. This replaces the ProtocolDriver interface and describe an interface specific for the trace exporter.

The otlptracegrpc adds a client that implements the otlptrace.Client. Most of this was part of the exporters/otlp/otlpgrpc/driver.go.

The otlp/grpc folder contains the grpc connection logic that will be used by both the metrics and traces Client. This still needs to be a module to not add the grpc dependency on the otlp package. This is a copy from the existing code in otlpgrpc/connection.go

The otlptracetest containts the integration test for the otlp trace Exporter, to test it with both grpc and http clients. This is a copy from the existing code exporters/otlp/internal/otlptest.

Tests were mainly copied from the existing tests and removed the metrics part of them.

How users will interact with it

The sugar functions to create and install an pipeline was added directly to the otlptracegrpc pacakge. This means that the users only needs to interact with it to create/install a trace pipeline to the sdk.

exp, err := otlptracegrpc.NewExporter(ctx, opts...)

or

exp, tp, err := otlptracegrpc.InstallNewPipeline(ctx, opts...)

What comes next?

This is part of the #1827 todo list:

• add gRPC otlp trace exporter
• add gRPC otlp metrics exporter
• add HTTP otlp trace exporter
• add HTTP otlp metrics exporter
• refactor out the duplicated code (transform, connection, otlpconfig)
• update proto-go dependencies
• update examples and remove old otlp gRPC/HTTP exporter
• update docs

[codecov]

Codecov Report

Merging #1922 (7b17a42) into main (5a8f7ff) will decrease coverage by 2.9%.
The diff coverage is 47.9%.

@@           Coverage Diff           @@
##            main   #1922     +/-   ##
=======================================
- Coverage   79.3%   76.3%   -3.0%     
=======================================
  Files        141     157     +16     
  Lines       7553    8358    +805     
=======================================
+ Hits        5992    6380    +388     
- Misses      1313    1723    +410     
- Partials     248     255      +7     

Impacted Files	Coverage Δ
...rs/otlp/otlptrace/internal/otlptracetest/client.go	0.0% <0.0%> (ø)
...otlp/otlptrace/internal/otlptracetest/collector.go	0.0% <0.0%> (ø)
...ters/otlp/otlptrace/internal/otlptracetest/data.go	0.0% <0.0%> (ø)
.../otlp/otlptrace/internal/otlptracetest/otlptest.go	0.0% <0.0%> (ø)
exporters/otlp/otlptrace/otlptracegrpc/exporter.go	0.0% <0.0%> (ø)
...s/otlp/otlptrace/internal/connection/connection.go	2.3% <2.3%> (ø)
exporters/otlp/otlptrace/exporter.go	46.5% <46.5%> (ø)
exporters/otlp/otlptrace/otlptracegrpc/options.go	59.3% <59.3%> (ø)
...xporters/otlp/otlptrace/internal/otlpconfig/tls.go	66.6% <66.6%> (ø)
...ters/otlp/otlptrace/internal/otlpconfig/options.go	75.6% <75.6%> (ø)
... and 23 more

[MrAlias]

Overall I think the approach show in this PR should be adopted. I think this PR, though large, is properly scoped for the initial PR for a signal in the OTLP and should be marked ready for review so it can be thoroughly reviewed.

I'm assuming there would be follow on PRs to:

• add the HTTP driver for traces
• add a similarly scoped metrics client with gRPC driver
• add an HTTP metrics driver (?)

[MrAlias]

We should also include a PR to finish the changes that updates the README: https://github.com/open-telemetry/opentelemetry-go/blob/main/exporters/otlp/README.md

[pellared]

Can you please add #1827 to the PR description? I spend a few minutes to find it 😉

It would be good if you rebase your branch. It misses changes introduced here: d23cc61

First of all, I agree with @paivagustavo that these packages should be "self-contained" so that the user can just like this: exp, err := otlptracegrpc.NewExporter(ctx, opts...). It also decouples the API surface from the implementation details. It would make it possible in the future to not use otlptrace inside otlptracegrpc without making any breaking changes. I made a similar "wrapping" here: d23cc61

General design comments for the whole exporters/otlp directory:

I feel that the the exporters/otlp package and exporters/otlp/otlptrace have really nothing to do with the otlp itselt. It looks just like a helper package to create an exporter (it does not have to be an OTLP exporter). Probably it should not be located under otlp, but just somewhere under exporters and also rather as internal to reduce the API surface at this stage.

When I am looking at

opentelemetry-go/exporters/otlp/protocoldriver.go

Lines 25 to 52 in f06cace

	// ProtocolDriver is an interface used by OTLP exporter. It's
	// responsible for connecting to and disconnecting from the collector,
	// and for transforming traces and metrics into wire format and
	// transmitting them to the collector.
	type ProtocolDriver interface {
	// Start should establish connection(s) to endpoint(s). It is
	// called just once by the exporter, so the implementation
	// does not need to worry about idempotence and locking.
	Start(ctx context.Context) error
	// Stop should close the connections. The function is called
	// only once by the exporter, so the implementation does not
	// need to worry about idempotence, but it may be called
	// concurrently with ExportMetrics or ExportTraces, so proper
	// locking is required. The function serves as a
	// synchronization point - after the function returns, the
	// process of closing connections is assumed to be finished.
	Stop(ctx context.Context) error
	// ExportMetrics should transform the passed metrics to the
	// wire format and send it to the collector. May be called
	// concurrently with ExportTraces, so the manager needs to
	// take this into account by doing proper locking.
	ExportMetrics(ctx context.Context, cps metricsdk.CheckpointSet, selector metricsdk.ExportKindSelector) error
	// ExportTraces should transform the passed traces to the wire
	// format and send it to the collector. May be called
	// concurrently with ExportMetrics, so the manager needs to
	// take this into account by doing proper locking.
	ExportTraces(ctx context.Context, ss []tracesdk.ReadOnlySpan) error
	}

This interface feels very "god-ish" and does not look like related to OTLP and probably it should not be exported.

From a user perspective, I would like just to use otlphttp or otlpgrpc module. Initially, the modules could just implement the tracer exporter interface. Before the metrics API is stabilized I would prefer to use a module like x/exporters/otlphttp which would implement both traces and metrics exporter interfaces. As a user, if I am open to experimentation, I would simply use the module under x tree and I could use one exporter as both metrics and trace exporter.

When we (contributors) feel that the stuff under x is stabilized then we simply replace the stable otlphttp implementation with the code from x. Next, the package x can be still used (there is no need to remove it) and it can be then used e.g. to add a log exporter implementation.

I feel that it might be better to redesign the whole exporters/otlp directory even at the cost of making breaking changes.

[MadVikingGod]

I like where it's going. I think this would be an acceptable way forward.

I know with the shared otel/grpc and internal this might not work, but would it be easier to maintain and communicate to users if this was under exporters/trace/otel? I think we pontificated on it a bit in the meeting, so if it's not possible or it makes things messier let's not do it.

[Aneurysm9]

Before the metrics API is stabilized I would prefer to use a module like x/exporters/otlphttp which would implement both traces and metrics exporter interfaces. As a user, if I am open to experimentation, I would simply use the module under x tree and I could use one exporter as both metrics and trace exporter.

When we (contributors) feel that the stuff under x is stabilized then we simply replace the stable otlphttp implementation with the code from x. Next, the package x can be still used (there is no need to remove it) and it can be then used e.g. to add a log exporter implementation.

I really do not like the idea of using import path to indicate experimental status and requiring users to change all of their imports and references when stability is reached. Semver has a perfectly fine way of indicating that an API is experimental and unstable, major version 0. Using a different import path would seem to violate this tenet of the OTel versioning guidelines.

OpenTelemetry clients MUST NOT be designed in a manner that breaks existing users when a signal transitions from experimental to stable. This would punish users of the release candidate, and hinder adoption.

[pellared]

I really do not like the idea of using import path to indicate experimental status and requiring users to change all of their imports and references when stability is reached. Semver has a perfectly fine way of indicating that an API is experimental and unstable, major version 0.
Using a different import path would seem to violate this tenet of the OTel versioning guidelines.

OpenTelemetry clients MUST NOT be designed in a manner that breaks existing users when a signal transitions from experimental to stable. This would punish users of the release candidate, and hinder adoption.

I see your points. I will think about it a few more times (maybe I will even edit this comment if totally I change my mind 😄 ).

I see the x pattern as a way to experiment with new API for modules/packages that already reached v1+.

With my proposal I try to achieve the following:

1. Reduce the number of packages so that the users not get lost in the large API/package surface.
2. Still have a way for the maintainers to experiment with a new unstable API.

What other solution do we have? Create a new package each time we want to create a new unstable API? Then the discovery of packages that contain new features would be harder.

Loose thoughts (it is chaotic)

I am aware that the semantic import versioning has its drawbacks: https://peter.bourgon.org/blog/2020/09/14/siv-is-unsound.html
However, one of its main reasons was to reduce the possibility of the diamond dependency problem: https://research.swtch.com/vgo

"Requiring users to change all of their imports" is a general property of Go modules. The users would need to change it anyway if we ever create a new major version.

Having a different import path for the "stable" and "unstable" packages reduces the possibility that a user would have the diamond problem. If done properly, the diamond problem could only occur for packages that are using unstable imports and the stable one should always work if the "stable" packages follow SemVer.

A broken build is more punishing than just changing the import path. Still, I suggested keeping the source code under x. If the unstable API did not have any breaking change then the import path does not have to be changed by any user.

[paivagustavo]

Can you please add #1827 to the PR description? I spend a few minutes to find it 😉

Sorry abou that, I've added it to the description along with a todo list for closing that issue.

It would be good if you rebase your branch. It misses changes introduced here: d23cc61

Thanks for catching that, rebased.

About the experimental package, I would say that we keep away of that. The metrics is not experimental, it's just unstable and users that are currently using it should be aware of it since it is a 0.x version. This pattern may work for some of the go language packages since they could drop any of them at any time, and that does not match with our current problem about stability of the metrics exporter.

I'm going to mark this PR as ready to review, and will start to get the others PRs moving as well.

[pellared]

@paivagustavo

Thanks for the comment and description. It is very clear to me and I like the plan 👍

Initially, I thought that we may want to consider implementing both metrics and traces exporter in one struct. However, I think the way you outlined is better.

[Aneurysm9]

I think this needs more godoc and test coverage before I can approve it, but I think this is definitely on the right track. Let's discuss at the SIG meeting tomorrow how we can sequence the steps you've outlined to get the trace exporters in shape ASAP to enable an RC and then we can worry about metrics.

[paivagustavo]

I've copied the transform and otlpconfig packages to the otlptracepackage. This completely removes the dependency of the otlp package from the otlptrace, and as a consequence the metrics sdk as well.

This does duplicate a bunch of code, that could be refactored in the future once the old otlp drivers are deleted.

Sorry for this big PR.

[MrAlias]

🥇

[MrAlias]

For the test failures, TraceStateFromKeyValues was moved to the oteltest package for testing purposes. I think the only place it is being used here is in the test packages so this should be a simple fix of changing the package it is imported from.

[pellared]

I am still not sure about the design of otlptrace module.

I feel that exporting a common exporter struct is a smell:

func NewExporter(ctx context.Context, opts ...Option) (*otlptrace.Exporter, error) {

Let me ask a question from the API client-side. Should the user ever need to use the otlptrace module directly? Do we want to allow the clients to use otlptrace module to create their own OTLP exporter implementation? Is it its purpose? If not then read the rest 😉

The otlptrace looks for me more like a helper module which only provides a common exporter implementation. I would rather make the package internal and wrap the type.

If we go further with the current design, then the otlptracehttp would have the same coupling to otlptrace. It will be hard to make a gRPC/HTTP specific change in the otlptrace.Exporter without without breaking Liskov Substitution Principle. Also this may cause to introduce API changes in both gRPC and HTTP modules when we would like to add something only for one module.

At this point, I would suggest removing otlptrace module and moving the exporter struct to otlptracegrpc. However, I am not sure if we want to return an interface or a concrete struct. I think that struct should be OK for this use case. We can introduce the otlptrace module together with otlptracehttp in next PR.

I also put a little comment here for visibility: #1827 (comment)

[paivagustavo]

Should the user ever need to use the otlptrace module directly?

They way it is designed, users and vendors may implement their own client. One of the reasons for such custom client would be #1819 (comment).

It will be hard to make a gRPC/HTTP specific change in the otlptrace.Exporter without without breaking Liskov Substitution Principle.

The Exporter is only supposed to implement the tracesdk.SpanExporter interface and nothing else. Any protocol-specific code should live in the specific client package.

I understand your reasoning about the existence of a otlptrace package. It is indeed a "helper" package, that it depends on others packages to work. I like the idea of keeping that "public",i.e. not internal, and letting users extend if needed.

We can go either direction, and it is easy to move this package to an internal folder if there is a consensus about that, we should probably discuss more on this week SIG meeting.

[pellared]

I understand your reasoning about the existence of a otlptrace package. It is indeed a "helper" package, that it depends on others packages to work. I like the idea of keeping that "public",i.e. not internal, and letting users extend if needed.

We can go either direction, and it is easy to move this package to an internal folder if there is a consensus about that, we should probably discuss more on this week SIG meeting.

Even if we want to have otlptrace public I would consider wrapping its types. Because it may be a coincidental cohesion. Sorry for my annoyance, I just try to be very careful when reviewing the public API surface.

[Aneurysm9]

It will be hard to make a gRPC/HTTP specific change in the otlptrace.Exporter without without breaking Liskov Substitution Principle.

Go doesn't have subclassing, so I'm not sure that the LSP is applicable. If you want to think in terms of classical inheritance, I would view otlptrace as providing an abstract class that needs to be reified by including a client implementation. That implementation can come from otlptracegrpc or otlptracehttp or some other user-provided package, but since we use composition and not inheritance the relationship is inverted and the otlptrace.Exporter has its missing implementation provided by composing a otlptrace.Client implementation rather than having other packages inherit common functionality from it.

[pellared]

Go doesn't have subclassing, so I'm not sure that the LSP is applicable. If you want to think in terms of classical inheritance, I would view otlptrace as providing an abstract class that needs to be reified by including a client implementation. That implementation can come from otlptracegrpc or otlptracehttp or some other user-provided package, but since we use composition and not inheritance the relationship is inverted and the otlptrace.Exporter has its missing implementation provided by composing a otlptrace.Client implementation rather than having other packages inherit common functionality from it.

func NewExporter(ctx context.Context, opts ...Option) (*otlptrace.Exporter, error) {

My concern is that this function returns a concrete struct from another package. Not an "abstract class" or interface.

I am not against using otlptrace.Exporter for sake of easier implementation. I think it should not be used as a "returned value" in the function signature. It tightly couples the NewExporter functions in otlptracegrpc and otlptracegrpc to otlptrace.Exporter.

[pellared]

Writing from mobile I remember that some Options where wrapped in otlp folder to mitigate similar issue.

Edit:
I mean here we are wrapping internal options.

I believe this design makes it easier to maintain backwards compatibility.

[MadVikingGod]

Not a blocker because this is internal.

I noticed that the grpc connection struct was moved into the otlptrace package. Is the plan to have this mostly copied when we make the otlpmetric? If we move it back to otlp, or have copies I think either will work.

[pellared]

During SIG meeting @Aneurysm9 has explained why we are OK using the same exporter struct for both modules.