-
Notifications
You must be signed in to change notification settings - Fork 411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Trace SDK] Provide builders to avoid exposing SDK internals #1393
Comments
To clarify, even after including all the dependencies from Limiting include pollution with dependencies that can be avoided sounds a better long term solution. |
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. |
Agree, this will definitely decrease the build time for the application - Thanks @owent for volunteering to take it. Assigning it to you. |
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 Could you please make the changes base on async-changes branch? We are working on merge this branch back into |
@deadb0d4 - This is assigned to you now. Thanks for the initiative. |
Assigning it to @marcalff, he has plans to look into this once async changes are merged. |
[Trace SDK] Implement builders (open-telemetry#1393)
Fixed bazel build, clang-format.
Format cleanup.
Use std::unique_ptr instead of nostd::unique_ptr
Continued, implemented: - JaegerExporterFactory, - InMemorySpanExporterFactory, - RandomIdGeneratorFactory.
Added doc. Makefile format.
Misc cleanup.
Implemented code review comments: - reworded docs/cpp-sdk-factory-design.md, - re organized includes, - added TracerContextFactory. Also: - renamed Build() methods to Create(), for consistency with HttpClientFactory::Create(), - added doxygen comments, - added CHANGELOG entry.
Implement code review comments: - Factory methods should return a unique_ptr. - Used TracerContextFactory in examples.
Implement code review comments: - avoid copy of resource parameters (used const reference) - added unit tests to verify build sanity - moved JsonBytesMappingKind and HttpRequestContentType to their own header, to avoid the dependency on protobuf from otlp_http_client.h
Fixed makefiles.
Unit test cleanup
CMake cleanup. Reverted the renaming of library opentelemetry_exporter_in_memory to opentelemetry_exporter_memory_trace. The name opentelemetry_exporter_in_memory is used externally, in cmake/opentelemetry-cpp-config.cmake.in
Format makefile.
Fixed grammar in documentation.
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:
#include <opentelemetry/exporters/otlp/otlp_grpc_exporter.h>
OtlpGrpcExporter
.This is problematic for two reasons:
OtlpGrpcExporter
, the header fileotlp_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.
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:
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
, notopentelemetry::exporter::otlp::OtlpGrpcExporter
.This resolves:
otlp_grpc_exporter.h
is no longer needed (options are in a separate header)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.
The text was updated successfully, but these errors were encountered: