fixup! do not log, instead just bail instrumentation

Author: BridgeAR

include/datadog/trace_sampler_config.h

@@ -12,7 +12,6 @@
 
 #include "config.h"
 #include "expected.h"
-#include "logger.h"
 #include "optional.h"
 #include "rate.h"
 #include "sampling_mechanism.h"
@@ -43,7 +42,7 @@ struct TraceSamplerConfig {
 
 class FinalizedTraceSamplerConfig {
   friend Expected<FinalizedTraceSamplerConfig> finalize_config(
-      const TraceSamplerConfig& config, Logger& logger);
+      const TraceSamplerConfig& config);
   friend class FinalizedTracerConfig;
 
   FinalizedTraceSamplerConfig() = default;
@@ -59,7 +58,7 @@ class FinalizedTraceSamplerConfig {
 };
 
 Expected<FinalizedTraceSamplerConfig> finalize_config(
-    const TraceSamplerConfig& config, Logger& logger);
+    const TraceSamplerConfig& config);
 
 }  // namespace tracing
 }  // namespace datadog

src/datadog/datadog_agent_config.cpp

@@ -16,7 +16,7 @@ namespace tracing {
 
 namespace env = environment;
 
-DatadogAgentConfig load_datadog_agent_env_config(Logger& logger) {
+Expected<DatadogAgentConfig> load_datadog_agent_env_config() {
   DatadogAgentConfig env_config;
 
   if (auto rc_enabled = env::lookup<env::DD_REMOTE_CONFIGURATION_ENABLED>()) {
@@ -26,8 +26,8 @@ DatadogAgentConfig load_datadog_agent_env_config(Logger& logger) {
   auto raw_rc_poll_interval_value =
       env::lookup<env::DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS>();
   if (auto error = raw_rc_poll_interval_value.if_error()) {
-    logger.log_error(error->with_prefix(
-        "DatadogAgent: Remote Configuration poll interval error "));
+    return error->with_prefix(
+        "DatadogAgent: Remote Configuration poll interval error ");
   } else if (*raw_rc_poll_interval_value) {
     env_config.remote_configuration_poll_interval_seconds =
         **raw_rc_poll_interval_value;
@@ -37,8 +37,7 @@ DatadogAgentConfig load_datadog_agent_env_config(Logger& logger) {
   Optional<std::uint64_t> env_port = nullopt;
   const auto raw_env_port = env::lookup<env::DD_TRACE_AGENT_PORT>();
   if (auto* error = raw_env_port.if_error()) {
-    logger.log_error(
-        error->with_prefix("DatadogAgent: Agent port parsing error "));
+    return error->with_prefix("DatadogAgent: Agent port parsing error ");
   } else {
     env_port = *raw_env_port;
   }
@@ -60,7 +59,10 @@ DatadogAgentConfig load_datadog_agent_env_config(Logger& logger) {
 Expected<FinalizedDatadogAgentConfig> finalize_config(
     const DatadogAgentConfig& user_config,
     const std::shared_ptr<Logger>& logger, const Clock& clock) {
-  DatadogAgentConfig env_config = load_datadog_agent_env_config(*logger);
+  Expected<DatadogAgentConfig> env_config = load_datadog_agent_env_config();
+  if (auto error = env_config.if_error()) {
+    return *error;
+  }
 
   FinalizedDatadogAgentConfig result;
 
@@ -89,7 +91,7 @@ Expected<FinalizedDatadogAgentConfig> finalize_config(
       user_config.remote_configuration_listeners;
 
   if (auto flush_interval_milliseconds =
-          value_or(env_config.flush_interval_milliseconds,
+          value_or(env_config->flush_interval_milliseconds,
                    user_config.flush_interval_milliseconds, 2000);
       flush_interval_milliseconds > 0) {
     result.flush_interval =
@@ -101,7 +103,7 @@ Expected<FinalizedDatadogAgentConfig> finalize_config(
   }
 
   if (auto request_timeout_milliseconds =
-          value_or(env_config.request_timeout_milliseconds,
+          value_or(env_config->request_timeout_milliseconds,
                    user_config.request_timeout_milliseconds, 2000);
       request_timeout_milliseconds > 0) {
     result.request_timeout =
@@ -113,7 +115,7 @@ Expected<FinalizedDatadogAgentConfig> finalize_config(
   }
 
   if (auto shutdown_timeout_milliseconds =
-          value_or(env_config.shutdown_timeout_milliseconds,
+          value_or(env_config->shutdown_timeout_milliseconds,
                    user_config.shutdown_timeout_milliseconds, 2000);
       shutdown_timeout_milliseconds > 0) {
     result.shutdown_timeout =
@@ -125,7 +127,7 @@ Expected<FinalizedDatadogAgentConfig> finalize_config(
   }
 
   if (double rc_poll_interval_seconds =
-          value_or(env_config.remote_configuration_poll_interval_seconds,
+          value_or(env_config->remote_configuration_poll_interval_seconds,
                    user_config.remote_configuration_poll_interval_seconds, 5.0);
       rc_poll_interval_seconds >= 0.0) {
     result.remote_configuration_poll_interval =
@@ -138,11 +140,11 @@ Expected<FinalizedDatadogAgentConfig> finalize_config(
   }
 
   result.remote_configuration_enabled =
-      value_or(env_config.remote_configuration_enabled,
+      value_or(env_config->remote_configuration_enabled,
                user_config.remote_configuration_enabled, true);
 
   const auto [origin, url] =
-      pick(env_config.url, user_config.url, "http://localhost:8126");
+      pick(env_config->url, user_config.url, "http://localhost:8126");
   auto parsed_url = HTTPClient::URL::parse(url);
   if (auto* error = parsed_url.if_error()) {
     return std::move(*error);

src/datadog/telemetry/configuration.cpp

@@ -30,13 +30,17 @@ tracing::Expected<Configuration> load_telemetry_env_config() {
 
   auto metrics_interval_seconds =
       env::lookup<env::DD_TELEMETRY_METRICS_INTERVAL_SECONDS>();
-  if (!metrics_interval_seconds.if_error() && *metrics_interval_seconds) {
+  if (auto error = metrics_interval_seconds.if_error()) {
+    return *error;
+  } else if (*metrics_interval_seconds) {
     env_cfg.metrics_interval_seconds = **metrics_interval_seconds;
   }
 
   auto heartbeat_interval_seconds =
       env::lookup<env::DD_TELEMETRY_HEARTBEAT_INTERVAL>();
-  if (!heartbeat_interval_seconds.if_error() && *heartbeat_interval_seconds) {
+  if (auto error = heartbeat_interval_seconds.if_error()) {
+    return *error;
+  } else if (*heartbeat_interval_seconds) {
     env_cfg.heartbeat_interval_seconds = **heartbeat_interval_seconds;
   }
 

src/datadog/trace_sampler_config.cpp

@@ -17,7 +17,7 @@ namespace env = environment;
 
 namespace {
 
-Expected<TraceSamplerConfig> load_trace_sampler_env_config(Logger &logger) {
+Expected<TraceSamplerConfig> load_trace_sampler_env_config() {
   TraceSamplerConfig env_config;
 
   if (auto rules_env = env::lookup<env::DD_TRACE_SAMPLING_RULES>()) {
@@ -111,16 +111,22 @@ Expected<TraceSamplerConfig> load_trace_sampler_env_config(Logger &logger) {
 
   const auto sample_rate_env = env::lookup<env::DD_TRACE_SAMPLE_RATE>();
   if (auto *error = sample_rate_env.if_error()) {
-    logger.log_error(
-        error->with_prefix("Unable to parse DD_TRACE_SAMPLE_RATE: "));
+    std::string prefix;
+    prefix += "While parsing ";
+    append(prefix, name(env::DD_TRACE_SAMPLE_RATE));
+    prefix += ": ";
+    return error->with_prefix(prefix);
   } else if (*sample_rate_env) {
     env_config.sample_rate = **sample_rate_env;
   }
 
   const auto limit_env = env::lookup<env::DD_TRACE_RATE_LIMIT>();
   if (auto *error = limit_env.if_error()) {
-    logger.log_error(
-        error->with_prefix("Unable to parse DD_TRACE_RATE_LIMIT: "));
+    std::string prefix;
+    prefix += "While parsing ";
+    append(prefix, name(env::DD_TRACE_RATE_LIMIT));
+    prefix += ": ";
+    return error->with_prefix(prefix);
   } else if (*limit_env) {
     env_config.max_per_second = **limit_env;
   }
@@ -144,9 +150,8 @@ std::string to_string(const std::vector<TraceSamplerConfig::Rule> &rules) {
 TraceSamplerConfig::Rule::Rule(const SpanMatcher &base) : SpanMatcher(base) {}
 
 Expected<FinalizedTraceSamplerConfig> finalize_config(
-    const TraceSamplerConfig &config, Logger &logger) {
-  Expected<TraceSamplerConfig> env_config =
-      load_trace_sampler_env_config(logger);
+    const TraceSamplerConfig &config) {
+  Expected<TraceSamplerConfig> env_config = load_trace_sampler_env_config();
   if (auto error = env_config.if_error()) {
     return *error;
   }

src/datadog/tracer_config.cpp

@@ -92,10 +92,10 @@ Optional<StringView> lookup_propagation_env(environment::Variable variable) {
 }
 
 // Return a `std::vector<PropagationStyle>` parsed from the specified `env_var`.
-// If `env_var` is not in the environment, return `nullopt`.
-// If parsing fails, log and ignore the environment variable.
+// If `env_var` is not in the environment, return `nullopt`. If an error occurs,
+// throw an `Error`.
 Optional<std::vector<PropagationStyle>> styles_from_env(
-    environment::Variable env_var, Logger &logger) {
+    environment::Variable env_var) {
   const auto styles_env = lookup_propagation_env(env_var);
   if (!styles_env) {
     return {};
@@ -107,8 +107,7 @@ Optional<std::vector<PropagationStyle>> styles_from_env(
     prefix += "Unable to parse ";
     append(prefix, name(env_var));
     prefix += " environment variable: ";
-    logger.log_error(error->with_prefix(prefix));
-    return {};
+    throw error->with_prefix(prefix);
   }
   return *styles;
 }
@@ -134,10 +133,9 @@ Expected<TracerConfig> load_tracer_env_config(Logger &logger) {
       prefix += "Unable to parse ";
       append(prefix, name(env::DD_TAGS));
       prefix += " environment variable: ";
-      logger.log_error(error->with_prefix(prefix));
-    } else {
-      env_cfg.tags = std::move(*tags);
+      return error->with_prefix(prefix);
     }
+    env_cfg.tags = std::move(*tags);
   }
 
   if (auto startup_env = env::lookup<env::DD_TRACE_STARTUP_LOGS>()) {
@@ -168,16 +166,14 @@ Expected<TracerConfig> load_tracer_env_config(Logger &logger) {
   // Baggage
   const auto baggage_items_env = env::lookup<env::DD_TRACE_BAGGAGE_MAX_ITEMS>();
   if (auto *error = baggage_items_env.if_error()) {
-    logger.log_error(error->with_prefix(
-        "Unable to parse DD_TRACE_BAGGAGE_MAX_ITEMS environment variable: "));
+    return *error;
   } else if (*baggage_items_env) {
     env_cfg.baggage_max_items = std::move(**baggage_items_env);
   }
 
   const auto baggage_bytes_env = env::lookup<env::DD_TRACE_BAGGAGE_MAX_BYTES>();
   if (auto *error = baggage_bytes_env.if_error()) {
-    logger.log_error(error->with_prefix(
-        "Unable to parse DD_TRACE_BAGGAGE_MAX_BYTES environment variable: "));
+    return *error;
   } else if (*baggage_bytes_env) {
     env_cfg.baggage_max_bytes = std::move(**baggage_bytes_env);
   }
@@ -252,27 +248,31 @@ Expected<TracerConfig> load_tracer_env_config(Logger &logger) {
         warn_message(var_name, *value, var_name_override, *value_override)});
   }
 
-  const auto global_styles =
-      styles_from_env(environment::DD_TRACE_PROPAGATION_STYLE, logger);
+  try {
+    const auto global_styles =
+        styles_from_env(environment::DD_TRACE_PROPAGATION_STYLE);
 
-  if (auto trace_extraction_styles = styles_from_env(
-          environment::DD_TRACE_PROPAGATION_STYLE_EXTRACT, logger)) {
-    env_cfg.extraction_styles = std::move(*trace_extraction_styles);
-  } else if (auto extraction_styles = styles_from_env(
-                 environment::DD_PROPAGATION_STYLE_EXTRACT, logger)) {
-    env_cfg.extraction_styles = std::move(*extraction_styles);
-  } else {
-    env_cfg.extraction_styles = global_styles;
-  }
+    if (auto trace_extraction_styles =
+            styles_from_env(environment::DD_TRACE_PROPAGATION_STYLE_EXTRACT)) {
+      env_cfg.extraction_styles = std::move(*trace_extraction_styles);
+    } else if (auto extraction_styles =
+                   styles_from_env(environment::DD_PROPAGATION_STYLE_EXTRACT)) {
+      env_cfg.extraction_styles = std::move(*extraction_styles);
+    } else {
+      env_cfg.extraction_styles = global_styles;
+    }
 
-  if (auto trace_injection_styles = styles_from_env(
-          environment::DD_TRACE_PROPAGATION_STYLE_INJECT, logger)) {
-    env_cfg.injection_styles = std::move(*trace_injection_styles);
-  } else if (auto injection_styles = styles_from_env(
-                 environment::DD_PROPAGATION_STYLE_INJECT, logger)) {
-    env_cfg.injection_styles = std::move(*injection_styles);
-  } else {
-    env_cfg.injection_styles = global_styles;
+    if (auto trace_injection_styles =
+            styles_from_env(environment::DD_TRACE_PROPAGATION_STYLE_INJECT)) {
+      env_cfg.injection_styles = std::move(*trace_injection_styles);
+    } else if (auto injection_styles =
+                   styles_from_env(environment::DD_PROPAGATION_STYLE_INJECT)) {
+      env_cfg.injection_styles = std::move(*injection_styles);
+    } else {
+      env_cfg.injection_styles = global_styles;
+    }
+  } catch (Error &error) {
+    return std::move(error);
   }
 
   return env_cfg;
@@ -429,8 +429,7 @@ Expected<FinalizedTracerConfig> finalize_config(const TracerConfig &user_config,
     return std::move(*error);
   }
 
-  if (auto trace_sampler_config =
-          finalize_config(user_config.trace_sampler, *logger)) {
+  if (auto trace_sampler_config = finalize_config(user_config.trace_sampler)) {
     // Merge metadata vectors
     for (auto &[key, values] : trace_sampler_config->metadata) {
       auto &dest = final_config.metadata[key];

test/telemetry/test_configuration.cpp

@@ -130,6 +130,13 @@ TELEMETRY_CONFIGURATION_TEST("validation") {
       auto final_cfg = telemetry::finalize_config();
       REQUIRE(!final_cfg);
     }
+
+    SECTION("environment variable parse error") {
+      ddtest::EnvGuard env("DD_TELEMETRY_METRICS_INTERVAL_SECONDS", "nope");
+      auto final_cfg = telemetry::finalize_config();
+      REQUIRE(!final_cfg);
+      REQUIRE(final_cfg.error().code == tracing::Error::INVALID_DOUBLE);
+    }
   }
 
   SECTION("heartbeat interval validation") {
@@ -146,6 +153,13 @@ TELEMETRY_CONFIGURATION_TEST("validation") {
       auto final_cfg = telemetry::finalize_config();
       REQUIRE(!final_cfg);
     }
+
+    SECTION("environment variable parse error") {
+      ddtest::EnvGuard env("DD_TELEMETRY_HEARTBEAT_INTERVAL", "bogus");
+      auto final_cfg = telemetry::finalize_config();
+      REQUIRE(!final_cfg);
+      REQUIRE(final_cfg.error().code == tracing::Error::INVALID_DOUBLE);
+    }
   }
 }