[Trace SDK] Provide builders to avoid exposing SDK internals

[marcalff]

Greetings.

In an application, as a user of the trace sdk, I need to create a opentelemetry::exporter::otlp::OtlpGrpcExporter instance.

Per instructions in exporters/otlp/README.md,
the only way to do this seems to call new on the class itself, which imply that:

• the code #include <opentelemetry/exporters/otlp/otlp_grpc_exporter.h>
• the user code compiled in the application now depends on the exact memory layout of OtlpGrpcExporter.

This is problematic for two reasons:

1. In the specific case of OtlpGrpcExporter, the header file otlp_grpc_exporter.h contains:
   #include "opentelemetry/proto/collector/trace/v1/trace_service.grpc.pb.h"

To build my application, I now need to compile this header from protobuf, and I need to install all the dependencies that are generated as well. This is very inconvenient, but still possible. It will slow down adoption of opentelemetry-cpp in any case.

2. In general, the content of otlp_grpc_exporter.h is very dependent on the SDK implementation, and is likely to change, causing compatibility issues with the user code. Delivering fixes without breaking the application is now much more difficult, which is the rationale for filing this as a bug and not a feature request.

Now, taking a step back, for applications merely using the SDK, they do not need to actually know the details of an exporter implementation, they only needs an opentelemetry::sdk::trace::SpanExporter to build a TraceProvider.

Applications extending the SDK will still need to see more details, this is a different topic.

I would suggest to create Builder classes, like:

// In namespace opentelemetry::exporter::otlp::
class OtlpGrpcExporterBuilder {
public:
  static std::unique_ptr<opentelemetry::sdk::trace::SpanExporter>
  Build(opentelemetry::exporter::otlp::OtlpGrpcExporterOptions options);
};

And implement the builder in a .cc file (in the exporter library), not inline in the header, even for a one-liner.

Note how the builder returns opentelemetry::sdk::trace::SpanExporter, not opentelemetry::exporter::otlp::OtlpGrpcExporter.

This resolves:

• the header file dependencies, as file otlp_grpc_exporter.h is no longer needed (options are in a separate header)
• the binary layout dependency on the SDK implementation classes, as class OtlpGrpcExporter is never seen.

This pattern can be applied to every subclass of SpanExporter, SpanProcessor, etc, which are at the top level of the SDK.

This can be applied to Traces, Metrics, and Logs.

Regards.

[marcalff]

To clarify, even after including all the dependencies from
#include "opentelemetry/proto/collector/trace/v1/trace_service.grpc.pb.h"
and get a build going, the application build failed with failures similar to issue #880 and issue #883.

Limiting include pollution with dependencies that can be avoided sounds a better long term solution.
#883 was resolved with a work around in the example code, but the problem still exists.

[owent]

Agree, I think it's a good idea to implement a standalone factory class to create OTLP exporters, which only depend OTLP exporter options and SDK.This can assign to me if there is no one else working on this.

[lalitb]

Agree, this will definitely decrease the build time for the application - Thanks @owent for volunteering to take it. Assigning it to you.

[deadb0d4]

Hey @owent, since it is marked as a good-first-issue could I please try this one out? I am a bit confused what classes specificly require builder method (e.g. SpanExporter)? Any chance, there is a doc?

[owent]

Hey @owent, since it is marked as a good-first-issue could I please try this one out? I am a bit confused what classes specificly require builder method (e.g. SpanExporter)? Any chance, there is a doc?

Yes and thanks.I think we can use OtlpFactory, which keep the simular name with HttpClientFactory and PredicateFactory .

Could you please make the changes base on async-changes branch? We are working on merge this branch back into main. We can reduce the conflicts if we base on async-changes.

[lalitb]

@deadb0d4 - This is assigned to you now. Thanks for the initiative.

[lalitb]

Assigning it to @marcalff, he has plans to look into this once async changes are merged.