fixup! Support plugin configuration via environment variables

johanneswuerbach · Dec 24, 2019 · 4853525 · 4853525
1 parent e1c8555
commit 4853525
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 6 deletions.
diff --git a/src/jaegertracing/ConfigTest.cpp b/src/jaegertracing/ConfigTest.cpp
@@ -176,6 +176,18 @@ TEST(Config, testFromEnv)
     ASSERT_EQ(true, config.disabled());
     ASSERT_EQ(std::string("host33:445"),
               config.reporter().localAgentHostPort());
+
+    testutils::EnvVariable::setEnv("JAEGER_AGENT_HOST", "");
+    testutils::EnvVariable::setEnv("JAEGER_AGENT_PORT", "");
+    testutils::EnvVariable::setEnv("JAEGER_ENDPOINT", "");
+    testutils::EnvVariable::setEnv("JAEGER_REPORTER_MAX_QUEUE_SIZE", "");
+    testutils::EnvVariable::setEnv("JAEGER_REPORTER_FLUSH_INTERVAL", "");
+    testutils::EnvVariable::setEnv("JAEGER_REPORTER_LOG_SPANS", "");
+    testutils::EnvVariable::setEnv("JAEGER_SAMPLER_PARAM", "");
+    testutils::EnvVariable::setEnv("JAEGER_SAMPLER_TYPE", "");
+    testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "");
+    testutils::EnvVariable::setEnv("JAEGER_TAGS", "");
+    testutils::EnvVariable::setEnv("JAEGER_DISABLED", "");
 }
 
 }  // namespace jaegertracing
diff --git a/src/jaegertracing/DynamicLoad.cpp b/src/jaegertracing/DynamicLoad.cpp
@@ -41,13 +41,16 @@ static int makeTracerFactory(const char* opentracingVersion,
         return opentracing::incompatible_library_versions_error.value();
     }
 
-    *tracerFactory = new (std::nothrow) jaegertracing::TracerFactory{};
+    const auto jaegerTracerFactory = new (std::nothrow) jaegertracing::TracerFactory{};
+    *tracerFactory = jaegerTracerFactory;
     if (*tracerFactory == nullptr) {
         *errorCategory = static_cast<const void*>(&std::generic_category());
         return static_cast<int>(std::errc::not_enough_memory);
     }
 
+    jaegerTracerFactory->enableReadFromEnv();
+
     return 0;
 }
 
-OPENTRACING_DECLARE_IMPL_FACTORY(makeTracerFactory)
+OPENTRACING_DECLARE_IMPL_FACTORY(makeTracerFactory)
diff --git a/src/jaegertracing/TracerFactory.cpp b/src/jaegertracing/TracerFactory.cpp
@@ -40,7 +40,9 @@ TracerFactory::MakeTracer(const char* configuration,
 
     auto tracerConfig = jaegertracing::Config::parse(yaml);
 
-    tracerConfig.fromEnv();
+    if (readFromEnv) {
+        tracerConfig.fromEnv();
+    }
 
     if (tracerConfig.serviceName().empty()) {
         errorMessage = "`service_name` not provided";
@@ -58,4 +60,8 @@ TracerFactory::MakeTracer(const char* configuration,
     return opentracing::make_unexpected(
         opentracing::invalid_configuration_error);
 }
+
+void TracerFactory::enableReadFromEnv() {
+    readFromEnv = true;
+}
 }  // namespace jaegertracing
diff --git a/src/jaegertracing/TracerFactory.h b/src/jaegertracing/TracerFactory.h
@@ -26,6 +26,14 @@ class TracerFactory : public opentracing::TracerFactory {
     opentracing::expected<std::shared_ptr<opentracing::Tracer>>
     MakeTracer(const char* configuration, std::string& errorMessage) const
         noexcept override;
+
+    TracerFactory() {
+      readFromEnv = false;
+    }
+
+    void enableReadFromEnv();
+  private:
+    bool readFromEnv;
 };
 
 }  // namespace jaegertracing

diff --git a/src/jaegertracing/TracerFactoryTest.cpp b/src/jaegertracing/TracerFactoryTest.cpp
@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+#include "jaegertracing/Tracer.h"
 #include "jaegertracing/TracerFactory.h"
 #include "jaegertracing/Constants.h"
 #include "jaegertracing/testutils/EnvVariable.h"
@@ -23,7 +24,6 @@ namespace jaegertracing {
 #ifdef JAEGERTRACING_WITH_YAML_CPP
 TEST(TracerFactory, testInvalidConfig)
 {
-    testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "");
     const char* invalidConfigTestCases[] = { "",
                                              "abc: {",
                                              R"({
@@ -67,23 +67,65 @@ TEST(TracerFactory, testValidConfig)
         "refreshInterval": 60
     }
   })";
-    testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "");
     TracerFactory tracerFactory;
     std::string errorMessage;
     auto tracerMaybe = tracerFactory.MakeTracer(config, errorMessage);
     ASSERT_EQ(errorMessage, "");
     ASSERT_TRUE(tracerMaybe);
 }
 
-TEST(TracerFactory, testEnvironmentConfig)
+TEST(TracerFactory, testWithoutReadFromEnv)
+{
+    const char* config = "";
+    testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "AService");
+    TracerFactory tracerFactory;
+    std::string errorMessage;
+    auto tracerMaybe = tracerFactory.MakeTracer(config, errorMessage);
+    ASSERT_FALSE(tracerMaybe);
+    ASSERT_NE(errorMessage, "");
+
+    testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "");
+}
+
+TEST(TracerFactory, testWithReadFromEnv)
 {
     const char* config = "";
     testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "AService");
     TracerFactory tracerFactory;
     std::string errorMessage;
+    tracerFactory.enableReadFromEnv();
+    auto tracerMaybe = tracerFactory.MakeTracer(config, errorMessage);
+    ASSERT_EQ(errorMessage, "");
+    ASSERT_TRUE(tracerMaybe);
+
+    auto tracer = tracerMaybe.value();
+    const auto jaegerTracer = std::dynamic_pointer_cast<jaegertracing::Tracer>(tracer);
+    ASSERT_EQ(std::string("AService"), jaegerTracer->serviceName());
+
+    testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "");
+}
+
+TEST(TracerFactory, testFileConfigTakesPrecedence)
+{
+
+    const char* config = R"(
+  {
+    "service_name": "test"
+  })";
+    testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "Ignored");
+    TracerFactory tracerFactory;
+    std::string errorMessage;
+    // TODO: This should work, but doesn't yet.
+    // tracerFactory.enableReadFromEnv();
     auto tracerMaybe = tracerFactory.MakeTracer(config, errorMessage);
     ASSERT_EQ(errorMessage, "");
     ASSERT_TRUE(tracerMaybe);
+
+    auto tracer = tracerMaybe.value();
+    const auto jaegerTracer = std::dynamic_pointer_cast<jaegertracing::Tracer>(tracer);
+    ASSERT_EQ(std::string("test"), jaegerTracer->serviceName());
+
+    testutils::EnvVariable::setEnv("JAEGER_SERVICE_NAME", "");
 }
 #else
 TEST(TracerFactory, failsWithoutYAML)