Fix trace sdk builder 1393

CHANGELOG.md

@@ -15,6 +15,8 @@ Increment the:
 
 ## [Unreleased]
 
+* [TRACE SDK] Add trace sdk builders (#1393) [#1471](https://github.com/open-telemetry/opentelemetry-cpp/pull/1471)
+* [EXAMPLE] Fix memory ownership of InMemorySpanExporter (#1473) [#1471](https://github.com/open-telemetry/opentelemetry-cpp/pull/1471)
 * [EXPORTER] OTLP http exporter allow concurrency session ([#1209](https://github.com/open-telemetry/opentelemetry-cpp/pull/1209))
 * [EXT] `curl::HttpClient` use `curl_multi_handle` instead of creating a thread
  for every request and it's able to reuse connections now. ([#1317](https://github.com/open-telemetry/opentelemetry-cpp/pull/1317))

docs/cpp-sdk-factory-design.md

@@ -0,0 +1,166 @@
+# SDK Factory Design
+
+When an application owner needs to configure the opentelemetry SDK,
+this can be done in different ways:
+
+- by assembly of available SDK elements already provided,
+- by extension of the SDK to support more functionality.
+
+The sections below investigate each use case,
+and the consequences when using shared libraries.
+
+Last, a section discuss the impact of C++ conditional parameters
+on the SDK interface, and how this affects shared libraries.
+
+## SDK assembly
+
+Assume the application owner needs to build a trace provider, using the gRPC trace
+exporter.
+
+### Case study, direct call to the SDK implementation classes
+
+A possible way to build the exporter is to call, from the application code:
+
+```cpp
+opentelemetry::exporter::otlp::OtlpGrpcExporterOptions opts;
+
+auto exporter = std::unique_ptr<opentelemetry::sdk::trace::SpanExporter>(
+ new opentelemetry::exporter::otlp::OtlpGrpcExporter(opts));
+```
+
+When the application code is built and linked against static
+opentelemetry-cpp libraries, the result is a binary that is consistent, and
+works.
+
+However, in case of a bug fixed in opentelemetry-cpp itself, the entire
+application must be rebuilt (with a fixed version), redistributed,
+and reinstalled, to have the bug effectively fixed.
+
+This is highly undesirable.
+
+Instead of static libraries, now consider the same application built and
+linked against shared (dynamic) libraries instead.
+
+The desired goal with shared libraries is to allow to:
+
+- fix a bug in opentelemetry-cpp
+- deploy a fixed shared library
+- keep the application unchanged.
+
+For this to be possible, the ABI exposed by the SDK shared library
+must be stable.
+
+Here, class OtlpGrpcExporter is the implementation itself,
+and is very likely to change, even for a bug fix,
+so it is not a good thing to expose it.
+
+Setting the constraint on the SDK implementation that class
+OtlpGrpcExporter will never change is unrealistic,
+because a bug fix might require new members (for example, to add a mutex to
+fix a race), which will change the ABI (the memory layout is different),
+breaking the application code compiled with a different version.
+
+In summary, this line of code alone, in the application space:
+
+ new opentelemetry::exporter::otlp::OtlpGrpcExporter(opts);
+
+prevents in practice any deployment using shared libraries.
+
+### Case study, using Factory and builders from the SDK
+
+The SDK also provide Factory classes, that can be used as follows
+from the application code:
+
+```cpp
+opentelemetry::exporter::otlp::OtlpGrpcExporterOptions opts;
+
+auto exporter =
+ opentelemetry::exporter::otlp::OtlpGrpcExporterFactory::Create(opts);
+```
+
+While the application code does not change much,
+the amount of SDK internals exposed to the application is reduced
+significantly.
+
+OtlpGrpcExporterFactory::Create() actually returns a abstract SpanExporter
+object, instead of a concrete OtlpGrpcExporter object.
+
+As a result, the application binary is not even aware of the implementation
+class OtlpGrpcExporter.
+
+This property makes it possible to:
+
+- implement changes in the SDK itself
+- deploy a new SDK shared library
+- keep the application unchanged
+
+## SDK extension
+
+Applications owners who want to extend existing SDK classes are expected
+to have a stronger dependency on the SDK internals.
+
+For example, with
+
+```cpp
+class MyFancyOtlpGrpcExporter : public OtlpGrpcExporter {...}
+```
+
+the build depends on the actual SDK implementation.
+
+Class OtlpGrpcExporter is visible in the SDK public header files,
+to allow this pattern.
+
+Using shared libraries in this case is not recommended,
+because the SDK implementation is at greater risk of change.
+
+## Conditional parameters
+
+Assume Foo() is part of the SDK, delivered as a shared library.
+
+When an API contains conditional parameters, as in:
+
+```cpp
+void Foo(int x = 0, int y = 0);
+```
+
+this in reality produces 3 APIs usable by the application code:
+
+```cpp
+void Foo();
+void Foo(int x);
+void Foo(int x, int y);
+```
+
+as well as 1 ABI provided in a library:
+
+```cpp
+void Foo(int x, int y);
+```
+
+The value of the defaults parameters (x = 0) is compiled in the application
+code.
+
+Assume that, for a bug fix, the API definition is changed to:
+
+```cpp
+void Foo(int x = 0, int y = 1);
+```
+
+Deploying a new version of the SDK will have no effect,
+because the default value is in the application binary, not the shared
+library.
+
+Now, assume a later change needs to add a new parameter:
+
+```cpp
+void Foo(int x = 0, int y = 1, int z = 2);
+```
+
+Here, clients will call the old Foo(int x, int y) ABI, crashing in the SDK
+because the SDK expects 3 parameters, not 2.
+
+Because of this, conditional parameters are to be avoided,
+not to be used in the SDK interface.
+
+Note that using conditional parameters in the opentelemetry-cpp API is ok,
+because the API is header only (there is no ABI).

examples/batch/main.cc

@@ -1,28 +1,27 @@
 // Copyright The OpenTelemetry Authors
 // SPDX-License-Identifier: Apache-2.0
 
-#include "opentelemetry/sdk/trace/tracer_provider.h"
+#include "opentelemetry/exporters/ostream/span_exporter_factory.h"
+#include "opentelemetry/sdk/trace/batch_span_processor_factory.h"
+#include "opentelemetry/sdk/trace/tracer_provider_factory.h"
 #include "opentelemetry/trace/provider.h"
-// Using an exporter that simply dumps span data to stdout.
-#include "opentelemetry/exporters/ostream/span_exporter.h"
-#include "opentelemetry/sdk/trace/batch_span_processor.h"
 
 #include <chrono>
 #include <thread>
 
 constexpr int kNumSpans = 10;
 namespace trace_api = opentelemetry::trace;
 namespace resource = opentelemetry::sdk::resource;
-namespace exporter_trace = opentelemetry::exporter::trace;
 namespace trace_sdk = opentelemetry::sdk::trace;
+namespace trace_exporter = opentelemetry::exporter::trace;
 namespace nostd = opentelemetry::nostd;
 
 namespace
 {
 
 void initTracer()
 {
- auto exporter = std::unique_ptr<trace_sdk::SpanExporter>(new exporter_trace::OStreamSpanExporter);
+ auto exporter = trace_exporter::OStreamSpanExporterFactory::Create();
 
  // CONFIGURE BATCH SPAN PROCESSOR PARAMETERS
 
@@ -38,11 +37,11 @@ void initTracer()
  resource::ResourceAttributes attributes = {{"service", "test_service"}, {"version", (uint32_t)1}};
  auto resource = resource::Resource::Create(attributes);
 
- auto processor = std::unique_ptr<trace_sdk::SpanProcessor>(
- new trace_sdk::BatchSpanProcessor(std::move(exporter), options));
+ auto processor = trace_sdk::BatchSpanProcessorFactory::Create(std::move(exporter), options);
+
+ std::shared_ptr<opentelemetry::trace::TracerProvider> provider =
+ trace_sdk::TracerProviderFactory::Create(std::move(processor), resource);
 
- auto provider = nostd::shared_ptr<trace_api::TracerProvider>(
- new trace_sdk::TracerProvider(std::move(processor), resource));
  // Set the global trace provider.
  trace_api::Provider::SetTracerProvider(provider);
 }

examples/grpc/tracer_common.h

@@ -2,15 +2,16 @@
 // SPDX-License-Identifier: Apache-2.0
 
 #pragma once
-#include "opentelemetry/exporters/ostream/span_exporter.h"
-#include "opentelemetry/sdk/trace/simple_processor.h"
-#include "opentelemetry/sdk/trace/tracer_provider.h"
-#include "opentelemetry/trace/provider.h"
 
 #include "opentelemetry/context/propagation/global_propagator.h"
 #include "opentelemetry/context/propagation/text_map_propagator.h"
+#include "opentelemetry/exporters/ostream/span_exporter_factory.h"
 #include "opentelemetry/nostd/shared_ptr.h"
+#include "opentelemetry/sdk/trace/simple_processor_factory.h"
+#include "opentelemetry/sdk/trace/tracer_context_factory.h"
+#include "opentelemetry/sdk/trace/tracer_provider_factory.h"
 #include "opentelemetry/trace/propagation/http_trace_context.h"
+#include "opentelemetry/trace/provider.h"
 
 #include <grpcpp/grpcpp.h>
 #include <cstring>
@@ -70,16 +71,16 @@ class GrpcServerCarrier : public opentelemetry::context::propagation::TextMapCar
 
 void initTracer()
 {
- auto exporter = std::unique_ptr<opentelemetry::sdk::trace::SpanExporter>(
- new opentelemetry::exporter::trace::OStreamSpanExporter);
- auto processor = std::unique_ptr<opentelemetry::sdk::trace::SpanProcessor>(
- new opentelemetry::sdk::trace::SimpleSpanProcessor(std::move(exporter)));
+ auto exporter = opentelemetry::exporter::trace::OStreamSpanExporterFactory::Create();
+ auto processor =
+ opentelemetry::sdk::trace::SimpleSpanProcessorFactory::Create(std::move(exporter));
  std::vector<std::unique_ptr<opentelemetry::sdk::trace::SpanProcessor>> processors;
  processors.push_back(std::move(processor));
  // Default is an always-on sampler.
- auto context = std::make_shared<opentelemetry::sdk::trace::TracerContext>(std::move(processors));
- auto provider = opentelemetry::nostd::shared_ptr<opentelemetry::trace::TracerProvider>(
- new opentelemetry::sdk::trace::TracerProvider(context));
+ std::shared_ptr<opentelemetry::sdk::trace::TracerContext> context =
+ opentelemetry::sdk::trace::TracerContextFactory::Create(std::move(processors));
+ std::shared_ptr<opentelemetry::trace::TracerProvider> provider =
+ opentelemetry::sdk::trace::TracerProviderFactory::Create(context);
  // Set the global trace provider
  opentelemetry::trace::Provider::SetTracerProvider(provider);
 

examples/http/tracer_common.h

@@ -2,9 +2,11 @@
 // SPDX-License-Identifier: Apache-2.0
 
 #pragma once
-#include "opentelemetry/exporters/ostream/span_exporter.h"
-#include "opentelemetry/sdk/trace/simple_processor.h"
-#include "opentelemetry/sdk/trace/tracer_provider.h"
+
+#include "opentelemetry/exporters/ostream/span_exporter_factory.h"
+#include "opentelemetry/sdk/trace/simple_processor_factory.h"
+#include "opentelemetry/sdk/trace/tracer_context_factory.h"
+#include "opentelemetry/sdk/trace/tracer_provider_factory.h"
 #include "opentelemetry/trace/provider.h"
 
 #include "opentelemetry/context/propagation/global_propagator.h"
@@ -59,16 +61,16 @@ class HttpTextMapCarrier : public opentelemetry::context::propagation::TextMapCa
 
 void initTracer()
 {
- auto exporter = std::unique_ptr<opentelemetry::sdk::trace::SpanExporter>(
- new opentelemetry::exporter::trace::OStreamSpanExporter);
- auto processor = std::unique_ptr<opentelemetry::sdk::trace::SpanProcessor>(
- new opentelemetry::sdk::trace::SimpleSpanProcessor(std::move(exporter)));
+ auto exporter = opentelemetry::exporter::trace::OStreamSpanExporterFactory::Create();
+ auto processor =
+ opentelemetry::sdk::trace::SimpleSpanProcessorFactory::Create(std::move(exporter));
  std::vector<std::unique_ptr<opentelemetry::sdk::trace::SpanProcessor>> processors;
  processors.push_back(std::move(processor));
  // Default is an always-on sampler.
- auto context = std::make_shared<opentelemetry::sdk::trace::TracerContext>(std::move(processors));
- auto provider = opentelemetry::nostd::shared_ptr<opentelemetry::trace::TracerProvider>(
- new opentelemetry::sdk::trace::TracerProvider(context));
+ std::shared_ptr<opentelemetry::sdk::trace::TracerContext> context =
+ opentelemetry::sdk::trace::TracerContextFactory::Create(std::move(processors));
+ std::shared_ptr<opentelemetry::trace::TracerProvider> provider =
+ opentelemetry::sdk::trace::TracerProviderFactory::Create(context);
  // Set the global trace provider
  opentelemetry::trace::Provider::SetTracerProvider(provider);
 

examples/jaeger/main.cc

@@ -1,9 +1,9 @@
 // Copyright The OpenTelemetry Authors
 // SPDX-License-Identifier: Apache-2.0
 
-#include "opentelemetry/exporters/jaeger/jaeger_exporter.h"
-#include "opentelemetry/sdk/trace/simple_processor.h"
-#include "opentelemetry/sdk/trace/tracer_provider.h"
+#include "opentelemetry/exporters/jaeger/jaeger_exporter_factory.h"
+#include "opentelemetry/sdk/trace/simple_processor_factory.h"
+#include "opentelemetry/sdk/trace/tracer_provider_factory.h"
 #include "opentelemetry/trace/provider.h"
 
 #ifdef BAZEL_BUILD
@@ -23,11 +23,10 @@ opentelemetry::exporter::jaeger::JaegerExporterOptions opts;
 void InitTracer()
 {
  // Create Jaeger exporter instance
- auto exporter = std::unique_ptr<trace_sdk::SpanExporter>(new jaeger::JaegerExporter(opts));
- auto processor = std::unique_ptr<trace_sdk::SpanProcessor>(
- new trace_sdk::SimpleSpanProcessor(std::move(exporter)));
- auto provider =
- nostd::shared_ptr<trace::TracerProvider>(new trace_sdk::TracerProvider(std::move(processor)));
+ auto exporter = jaeger::JaegerExporterFactory::Create(opts);
+ auto processor = trace_sdk::SimpleSpanProcessorFactory::Create(std::move(exporter));
+ std::shared_ptr<opentelemetry::trace::TracerProvider> provider =
+ trace_sdk::TracerProviderFactory::Create(std::move(processor));
  // Set the global trace provider
  trace::Provider::SetTracerProvider(provider);
 }

examples/multi_processor/CMakeLists.txt

@@ -4,4 +4,5 @@ include_directories(${CMAKE_SOURCE_DIR}/exporters/ostream/include
 add_executable(example_multi_processor main.cc)
 target_link_libraries(
  example_multi_processor ${CMAKE_THREAD_LIBS_INIT} common_foo_library
- opentelemetry_trace opentelemetry_exporter_ostream_span)
+ opentelemetry_trace opentelemetry_exporter_ostream_span
+ opentelemetry_exporter_in_memory)