[Trace SDK] Implement builders (open-telemetry#1393)

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
marcalff · Jul 12, 2022 · 755df64 · 755df64
1 parent 83e7a1c
commit 755df64
Show file tree

Hide file tree

Showing 11 changed files with 178 additions and 32 deletions.
diff --git a/exporters/otlp/BUILD b/exporters/otlp/BUILD
@@ -230,6 +230,21 @@ cc_test(
  ],
 )
 
+cc_test(
+ name = "otlp_grpc_exporter_factory_test",
+ srcs = ["test/otlp_grpc_exporter_factory_test.cc"],
+ tags = [
+ "otlp",
+ "otlp_grpc",
+ "test",
+ ],
+ deps = [
+ ":otlp_grpc_exporter",
+ "//api",
+ "@com_google_googletest//:gtest_main",
+ ],
+)
+
 cc_test(
  name = "otlp_http_exporter_test",
  srcs = ["test/otlp_http_exporter_test.cc"],
@@ -246,6 +261,22 @@ cc_test(
  ],
 )
 
+cc_test(
+ name = "otlp_http_exporter_factory_test",
+ srcs = ["test/otlp_http_exporter_factory_test.cc"],
+ tags = [
+ "otlp",
+ "otlp_http",
+ "test",
+ ],
+ deps = [
+ ":otlp_http_exporter",
+ "//api",
+ "//ext/src/http/client/nosend:http_client_nosend",
+ "@com_google_googletest//:gtest_main",
+ ],
+)
+
 cc_test(
  name = "otlp_http_log_exporter_test",
  srcs = ["test/otlp_http_log_exporter_test.cc"],

diff --git a/exporters/otlp/CMakeLists.txt b/exporters/otlp/CMakeLists.txt
@@ -180,6 +180,15 @@ if(BUILD_TESTING)
  TEST_PREFIX exporter.otlp.
  TEST_LIST otlp_grpc_exporter_test)
 
+ add_executable(otlp_grpc_exporter_factory_test test/otlp_grpc_exporter_factory_test.cc)
+ target_link_libraries(
+ otlp_grpc_exporter_factory_test ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}
+ ${GMOCK_LIB} opentelemetry_exporter_otlp_grpc)
+ gtest_add_tests(
+ TARGET otlp_grpc_exporter_factory_test
+ TEST_PREFIX exporter.otlp.
+ TEST_LIST otlp_grpc_exporter_factory_test)
+
  if(WITH_LOGS_PREVIEW)
  add_executable(otlp_grpc_log_exporter_test
  test/otlp_grpc_log_exporter_test.cc)
@@ -204,6 +213,15 @@ if(BUILD_TESTING)
  TEST_PREFIX exporter.otlp.
  TEST_LIST otlp_http_exporter_test)
 
+ add_executable(otlp_http_exporter_factory_test test/otlp_http_exporter_factory_test.cc)
+ target_link_libraries(
+ otlp_http_exporter_factory_test ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}
+ ${GMOCK_LIB} opentelemetry_exporter_otlp_http)
+ gtest_add_tests(
+ TARGET otlp_http_exporter_factory_test
+ TEST_PREFIX exporter.otlp.
+ TEST_LIST otlp_http_exporter_factory_test)
+
  if(WITH_LOGS_PREVIEW)
  add_executable(otlp_http_log_exporter_test
  test/otlp_http_log_exporter_test.cc)

diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http.h
@@ -0,0 +1,29 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+#pragma once
+
+#include "opentelemetry/sdk/version/version.h"
+
+OPENTELEMETRY_BEGIN_NAMESPACE
+namespace exporter
+{
+namespace otlp
+{
+
+enum class JsonBytesMappingKind
+{
+ kHexId,
+ kHex,
+ kBase64,
+};
+
+enum class HttpRequestContentType
+{
+ kJson,
+ kBinary,
+};
+
+} // namespace otlp
+} // namespace exporter
+OPENTELEMETRY_END_NAMESPACE
diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h
@@ -15,6 +15,7 @@
 #include "opentelemetry/sdk/common/exporter_utils.h"
 
 #include "opentelemetry/exporters/otlp/otlp_environment.h"
+#include "opentelemetry/exporters/otlp/otlp_http.h"
 
 #include <atomic>
 #include <chrono>
@@ -38,19 +39,6 @@ constexpr char kDefaultMetricsPath[] = "/v1/metrics";
 constexpr char kHttpJsonContentType[] = "application/json";
 constexpr char kHttpBinaryContentType[] = "application/x-protobuf";
 
-enum class JsonBytesMappingKind
-{
- kHexId,
- kHex,
- kBase64,
-};
-
-enum class HttpRequestContentType
-{
- kJson,
- kBinary,
-};
-
 /**
  * Struct to hold OTLP HTTP client options.
  */

diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter_options.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter_options.h
@@ -3,7 +3,7 @@
 
 #pragma once
 
-#include "opentelemetry/exporters/otlp/otlp_http_client.h"
+#include "opentelemetry/exporters/otlp/otlp_http.h"
 
 #include "opentelemetry/exporters/otlp/otlp_environment.h"
 

diff --git a/exporters/otlp/test/otlp_grpc_exporter_factory_test.cc b/exporters/otlp/test/otlp_grpc_exporter_factory_test.cc
@@ -0,0 +1,36 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+/*
+ Cripple the build environment on purpose.
+ This is to make sure that if protobuf headers
+ are included by OtlpGrpcExporterFactory,
+ even indirectly, the build will fail.
+*/
+#define PROTOBUF_VERSION 6666666
+
+#include <gtest/gtest.h>
+
+#include "opentelemetry/exporters/otlp/otlp_grpc_exporter_factory.h"
+#include "opentelemetry/exporters/otlp/otlp_grpc_exporter_options.h"
+
+OPENTELEMETRY_BEGIN_NAMESPACE
+namespace exporter
+{
+namespace otlp
+{
+
+TEST(OtlpGrpcExporterFactoryTest, BuildTest)
+{
+ OtlpGrpcExporterOptions opts;
+ opts.endpoint = "localhost:45454";
+
+ std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> exporter =
+ OtlpGrpcExporterFactory::Create(opts);
+
+ EXPECT_TRUE(exporter != nullptr);
+}
+
+} // namespace otlp
+} // namespace exporter
+OPENTELEMETRY_END_NAMESPACE
diff --git a/exporters/otlp/test/otlp_http_exporter_factory_test.cc b/exporters/otlp/test/otlp_http_exporter_factory_test.cc
@@ -0,0 +1,44 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+/*
+ Cripple the build environment on purpose.
+ This is to make sure that if protobuf headers
+ are included by OtlpHttpExporterFactory,
+ even indirectly, the build will fail.
+*/
+#define PROTOBUF_VERSION 6666666
+
+#include <gtest/gtest.h>
+
+#include "opentelemetry/exporters/otlp/otlp_http_exporter_factory.h"
+#include "opentelemetry/exporters/otlp/otlp_http_exporter_options.h"
+
+/*
+ Make sure OtlpHttpExporterFactory does not leak nlohmann/json.hpp,
+ even indirectly.
+*/
+#ifdef NLOHMANN_JSON_VERSION_MAJOR
+# error "OtlpHttpExporterFactory should not expose nlohmann/json.hpp"
+#endif /* NLOHMANN_JSON_VERSION_MAJOR */
+
+OPENTELEMETRY_BEGIN_NAMESPACE
+namespace exporter
+{
+namespace otlp
+{
+
+TEST(OtlpHttpExporterFactoryTest, BuildTest)
+{
+ OtlpHttpExporterOptions opts;
+ opts.url = "localhost:45454";
+
+ std::unique_ptr<opentelemetry::sdk::trace::SpanExporter> exporter =
+ OtlpHttpExporterFactory::Create(opts);
+
+ EXPECT_TRUE(exporter != nullptr);
+}
+
+} // namespace otlp
+} // namespace exporter
+OPENTELEMETRY_END_NAMESPACE
diff --git a/sdk/include/opentelemetry/sdk/trace/tracer_context_factory.h b/sdk/include/opentelemetry/sdk/trace/tracer_context_factory.h
@@ -31,22 +31,22 @@ class TracerContextFactory
  */
  static std::unique_ptr<TracerContext> Create(
  std::vector<std::unique_ptr<SpanProcessor>> &&processor,
- opentelemetry::sdk::resource::Resource resource);
+ const opentelemetry::sdk::resource::Resource &resource);
 
  /**
  * Create a TracerContext.
  */
  static std::unique_ptr<TracerContext> Create(
  std::vector<std::unique_ptr<SpanProcessor>> &&processor,
- opentelemetry::sdk::resource::Resource resource,
+ const opentelemetry::sdk::resource::Resource &resource,
  std::unique_ptr<Sampler> sampler);
 
  /**
  * Create a TracerContext.
  */
  static std::unique_ptr<TracerContext> Create(
  std::vector<std::unique_ptr<SpanProcessor>> &&processor,
- opentelemetry::sdk::resource::Resource resource,
+ const opentelemetry::sdk::resource::Resource &resource,
  std::unique_ptr<Sampler> sampler,
  std::unique_ptr<IdGenerator> id_generator);
 };

diff --git a/sdk/include/opentelemetry/sdk/trace/tracer_provider_factory.h b/sdk/include/opentelemetry/sdk/trace/tracer_provider_factory.h
@@ -29,16 +29,16 @@ class TracerProviderFactory
 
  static std::unique_ptr<opentelemetry::trace::TracerProvider> Create(
  std::unique_ptr<SpanProcessor> processor,
- opentelemetry::sdk::resource::Resource resource);
+ const opentelemetry::sdk::resource::Resource &resource);
 
  static std::unique_ptr<opentelemetry::trace::TracerProvider> Create(
  std::unique_ptr<SpanProcessor> processor,
- opentelemetry::sdk::resource::Resource resource,
+ const opentelemetry::sdk::resource::Resource &resource,
  std::unique_ptr<Sampler> sampler);
 
  static std::unique_ptr<opentelemetry::trace::TracerProvider> Create(
  std::unique_ptr<SpanProcessor> processor,
- opentelemetry::sdk::resource::Resource resource,
+ const opentelemetry::sdk::resource::Resource &resource,
  std::unique_ptr<Sampler> sampler,
  std::unique_ptr<IdGenerator> id_generator);
 
@@ -49,16 +49,16 @@ class TracerProviderFactory
 
  static std::unique_ptr<opentelemetry::trace::TracerProvider> Create(
  std::vector<std::unique_ptr<SpanProcessor>> &&processors,
- opentelemetry::sdk::resource::Resource resource);
+ const opentelemetry::sdk::resource::Resource &resource);
 
  static std::unique_ptr<opentelemetry::trace::TracerProvider> Create(
  std::vector<std::unique_ptr<SpanProcessor>> &&processors,
- opentelemetry::sdk::resource::Resource resource,
+ const opentelemetry::sdk::resource::Resource &resource,
  std::unique_ptr<Sampler> sampler);
 
  static std::unique_ptr<opentelemetry::trace::TracerProvider> Create(
  std::vector<std::unique_ptr<SpanProcessor>> &&processors,
- opentelemetry::sdk::resource::Resource resource,
+ const opentelemetry::sdk::resource::Resource &resource,
  std::unique_ptr<Sampler> sampler,
  std::unique_ptr<IdGenerator> id_generator);
 

diff --git a/sdk/src/trace/tracer_context_factory.cc b/sdk/src/trace/tracer_context_factory.cc
@@ -22,15 +22,15 @@ std::unique_ptr<TracerContext> TracerContextFactory::Create(
 
 std::unique_ptr<TracerContext> TracerContextFactory::Create(
  std::vector<std::unique_ptr<SpanProcessor>> &&processor,
- opentelemetry::sdk::resource::Resource resource)
+ const opentelemetry::sdk::resource::Resource &resource)
 {
  auto sampler = AlwaysOnSamplerFactory::Create();
  return Create(std::move(processor), resource, std::move(sampler));
 }
 
 std::unique_ptr<TracerContext> TracerContextFactory::Create(
  std::vector<std::unique_ptr<SpanProcessor>> &&processor,
- opentelemetry::sdk::resource::Resource resource,
+ const opentelemetry::sdk::resource::Resource &resource,
  std::unique_ptr<Sampler> sampler)
 {
  auto id_generator = RandomIdGeneratorFactory::Create();
@@ -39,7 +39,7 @@ std::unique_ptr<TracerContext> TracerContextFactory::Create(
 
 std::unique_ptr<TracerContext> TracerContextFactory::Create(
  std::vector<std::unique_ptr<SpanProcessor>> &&processor,
- opentelemetry::sdk::resource::Resource resource,
+ const opentelemetry::sdk::resource::Resource &resource,
  std::unique_ptr<Sampler> sampler,
  std::unique_ptr<IdGenerator> id_generator)
 {

diff --git a/sdk/src/trace/tracer_provider_factory.cc b/sdk/src/trace/tracer_provider_factory.cc
@@ -24,15 +24,15 @@ std::unique_ptr<trace_api::TracerProvider> TracerProviderFactory::Create(
 
 std::unique_ptr<trace_api::TracerProvider> TracerProviderFactory::Create(
  std::unique_ptr<SpanProcessor> processor,
- opentelemetry::sdk::resource::Resource resource)
+ const opentelemetry::sdk::resource::Resource &resource)
 {
  auto sampler = AlwaysOnSamplerFactory::Create();
  return Create(std::move(processor), resource, std::move(sampler));
 }
 
 std::unique_ptr<opentelemetry::trace::TracerProvider> TracerProviderFactory::Create(
  std::unique_ptr<SpanProcessor> processor,
- opentelemetry::sdk::resource::Resource resource,
+ const opentelemetry::sdk::resource::Resource &resource,
  std::unique_ptr<Sampler> sampler)
 {
  auto id_generator = RandomIdGeneratorFactory::Create();
@@ -41,7 +41,7 @@ std::unique_ptr<opentelemetry::trace::TracerProvider> TracerProviderFactory::Cre
 
 std::unique_ptr<opentelemetry::trace::TracerProvider> TracerProviderFactory::Create(
  std::unique_ptr<SpanProcessor> processor,
- opentelemetry::sdk::resource::Resource resource,
+ const opentelemetry::sdk::resource::Resource &resource,
  std::unique_ptr<Sampler> sampler,
  std::unique_ptr<IdGenerator> id_generator)
 {
@@ -59,15 +59,15 @@ std::unique_ptr<opentelemetry::trace::TracerProvider> TracerProviderFactory::Cre
 
 std::unique_ptr<opentelemetry::trace::TracerProvider> TracerProviderFactory::Create(
  std::vector<std::unique_ptr<SpanProcessor>> &&processors,
- opentelemetry::sdk::resource::Resource resource)
+ const opentelemetry::sdk::resource::Resource &resource)
 {
  auto sampler = AlwaysOnSamplerFactory::Create();
  return Create(std::move(processors), resource, std::move(sampler));
 }
 
 std::unique_ptr<opentelemetry::trace::TracerProvider> TracerProviderFactory::Create(
  std::vector<std::unique_ptr<SpanProcessor>> &&processors,
- opentelemetry::sdk::resource::Resource resource,
+ const opentelemetry::sdk::resource::Resource &resource,
  std::unique_ptr<Sampler> sampler)
 {
  auto id_generator = RandomIdGeneratorFactory::Create();
@@ -76,7 +76,7 @@ std::unique_ptr<opentelemetry::trace::TracerProvider> TracerProviderFactory::Cre
 
 std::unique_ptr<opentelemetry::trace::TracerProvider> TracerProviderFactory::Create(
  std::vector<std::unique_ptr<SpanProcessor>> &&processors,
- opentelemetry::sdk::resource::Resource resource,
+ const opentelemetry::sdk::resource::Resource &resource,
  std::unique_ptr<Sampler> sampler,
  std::unique_ptr<IdGenerator> id_generator)
 {