Code review feedback

Author: chusitoo

exporters/otlp/src/otlp_environment.cc

@@ -80,32 +80,36 @@ static bool GetStringDualEnvVar(const char *signal_name,
   return exists;
 }
 
-static std::uint32_t GetUintEnvVarOrDefault(opentelemetry::nostd::string_view signal_env,
-                                            opentelemetry::nostd::string_view generic_env,
-                                            std::uint32_t default_value)
+static bool GetUintDualEnvVar(const char *signal_name,
+                              const char *generic_name,
+                              std::uint32_t &value)
 {
-  std::string value;
+  bool exists;
 
-  if (GetStringDualEnvVar(signal_env.data(), generic_env.data(), value))
+  exists = sdk_common::GetUintEnvironmentVariable(signal_name, value);
+  if (exists)
   {
-    return static_cast<std::uint32_t>(std::strtoul(value.c_str(), nullptr, 10));
+    return true;
   }
 
-  return default_value;
+  exists = sdk_common::GetUintEnvironmentVariable(generic_name, value);
+
+  return exists;
 }
 
-static float GetFloatEnvVarOrDefault(opentelemetry::nostd::string_view signal_env,
-                                     opentelemetry::nostd::string_view generic_env,
-                                     float default_value)
+static float GetFloatDualEnvVar(const char *signal_name, const char *generic_name, float &value)
 {
-  std::string value;
+  bool exists;
 
-  if (GetStringDualEnvVar(signal_env.data(), generic_env.data(), value))
+  exists = sdk_common::GetFloatEnvironmentVariable(signal_name, value);
+  if (exists)
   {
-    return std::strtof(value.c_str(), nullptr);
+    return true;
   }
 
-  return default_value;
+  exists = sdk_common::GetFloatEnvironmentVariable(generic_name, value);
+
+  return exists;
 }
 
 std::string GetOtlpDefaultGrpcTracesEndpoint()
@@ -1157,84 +1161,168 @@ std::uint32_t GetOtlpDefaultTracesRetryMaxAttempts()
 {
   constexpr char kSignalEnv[]  = "OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_ATTEMPTS";
   constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS";
-  return GetUintEnvVarOrDefault(kSignalEnv, kGenericEnv, 5U);
+  std::uint32_t value;
+
+  if (!GetUintDualEnvVar(kSignalEnv, kGenericEnv, value))
+  {
+    return 5U;
+  }
+
+  return value;
 }
 
 std::uint32_t GetOtlpDefaultMetricsRetryMaxAttempts()
 {
   constexpr char kSignalEnv[]  = "OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_ATTEMPTS";
   constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS";
-  return GetUintEnvVarOrDefault(kSignalEnv, kGenericEnv, 5U);
+  std::uint32_t value;
+
+  if (!GetUintDualEnvVar(kSignalEnv, kGenericEnv, value))
+  {
+    return 5U;
+  }
+
+  return value;
 }
 
 std::uint32_t GetOtlpDefaultLogsRetryMaxAttempts()
 {
   constexpr char kSignalEnv[]  = "OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_ATTEMPTS";
   constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS";
-  return GetUintEnvVarOrDefault(kSignalEnv, kGenericEnv, 5U);
+  std::uint32_t value;
+
+  if (!GetUintDualEnvVar(kSignalEnv, kGenericEnv, value))
+  {
+    return 5U;
+  }
+
+  return value;
 }
 
 float GetOtlpDefaultTracesRetryInitialBackoff()
 {
   constexpr char kSignalEnv[]  = "OTEL_EXPORTER_OTLP_TRACES_RETRY_INITIAL_BACKOFF";
   constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF";
-  return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.0);
+  float value;
+
+  if (!GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value))
+  {
+    return 1.0f;
+  }
+
+  return value;
 }
 
 float GetOtlpDefaultMetricsRetryInitialBackoff()
 {
   constexpr char kSignalEnv[]  = "OTEL_EXPORTER_OTLP_METRICS_RETRY_INITIAL_BACKOFF";
   constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF";
-  return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.0);
+  float value;
+
+  if (!GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value))
+  {
+    return 1.0f;
+  }
+
+  return value;
 }
 
 float GetOtlpDefaultLogsRetryInitialBackoff()
 {
   constexpr char kSignalEnv[]  = "OTEL_EXPORTER_OTLP_LOGS_RETRY_INITIAL_BACKOFF";
   constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF";
-  return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.0);
+  float value;
+
+  if (!GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value))
+  {
+    return 1.0f;
+  }
+
+  return value;
 }
 
 float GetOtlpDefaultTracesRetryMaxBackoff()
 {
   constexpr char kSignalEnv[]  = "OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_BACKOFF";
   constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF";
-  return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 5.0);
+  float value;
+
+  if (!GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value))
+  {
+    return 5.0f;
+  }
+
+  return value;
 }
 
 float GetOtlpDefaultMetricsRetryMaxBackoff()
 {
   constexpr char kSignalEnv[]  = "OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_BACKOFF";
   constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF";
-  return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 5.0);
+  float value;
+
+  if (!GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value))
+  {
+    return 5.0f;
+  }
+
+  return value;
 }
 
 float GetOtlpDefaultLogsRetryMaxBackoff()
 {
   constexpr char kSignalEnv[]  = "OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_BACKOFF";
   constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF";
-  return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 5.0);
+  float value;
+
+  if (!GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value))
+  {
+    return 5.0f;
+  }
+
+  return value;
 }
 
 float GetOtlpDefaultTracesRetryBackoffMultiplier()
 {
   constexpr char kSignalEnv[]  = "OTEL_EXPORTER_OTLP_TRACES_RETRY_BACKOFF_MULTIPLIER";
   constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER";
-  return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.5f);
+  float value;
+
+  if (!GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value))
+  {
+    return 1.5f;
+  }
+
+  return value;
 }
 
 float GetOtlpDefaultMetricsRetryBackoffMultiplier()
 {
   constexpr char kSignalEnv[]  = "OTEL_EXPORTER_OTLP_METRICS_RETRY_BACKOFF_MULTIPLIER";
   constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER";
-  return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.5f);
+  float value;
+
+  if (!GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value))
+  {
+    return 1.5f;
+  }
+
+  return value;
 }
 
 float GetOtlpDefaultLogsRetryBackoffMultiplier()
 {
   constexpr char kSignalEnv[]  = "OTEL_EXPORTER_OTLP_LOGS_RETRY_BACKOFF_MULTIPLIER";
   constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER";
-  return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.5f);
+  float value;
+
+  if (!GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value))
+  {
+    return 1.5f;
+  }
+
+  return value;
 }
 
 }  // namespace otlp

exporters/otlp/test/otlp_grpc_exporter_test.cc

@@ -19,7 +19,6 @@
 #  include "opentelemetry/exporters/otlp/otlp_grpc_exporter.h"
 #  include "opentelemetry/exporters/otlp/otlp_grpc_exporter_factory.h"
 #  include "opentelemetry/exporters/otlp/protobuf_include_prefix.h"
-#  include "opentelemetry/nostd/shared_ptr.h"
 
 // Problematic code that pulls in Gmock and breaks with vs2019/c++latest :
 #  include "opentelemetry/proto/collector/trace/v1/trace_service_mock.grpc.pb.h"
@@ -28,6 +27,7 @@
 
 #  include "opentelemetry/exporters/otlp/protobuf_include_suffix.h"
 
+#  include "opentelemetry/nostd/shared_ptr.h"
 #  include "opentelemetry/sdk/trace/simple_processor.h"
 #  include "opentelemetry/sdk/trace/simple_processor_factory.h"
 #  include "opentelemetry/sdk/trace/tracer_provider.h"

sdk/include/opentelemetry/sdk/common/env_variables.h

@@ -4,6 +4,7 @@
 #pragma once
 
 #include <chrono>
+#include <cstdint>
 #include <string>
 
 #include "opentelemetry/version.h"
@@ -39,6 +40,22 @@ bool GetDurationEnvironmentVariable(const char *env_var_name,
 */
 bool GetStringEnvironmentVariable(const char *env_var_name, std::string &value);
 
+/**
+  Read a uint32_t environment variable.
+  @param env_var_name Environment variable name
+  @param [out] value Variable value, if it exists
+  @return true if the variable exists
+*/
+bool GetUintEnvironmentVariable(const char *env_var_name, std::uint32_t &value);
+
+/**
+  Read a float environment variable.
+  @param env_var_name Environment variable name
+  @param [out] value Variable value, if it exists
+  @return true if the variable exists
+*/
+bool GetFloatEnvironmentVariable(const char *env_var_name, float &value);
+
 #if defined(_MSC_VER)
 inline int setenv(const char *name, const char *value, int)
 {