From c4c31aa7085ada83da0f836089210bebdde706a5 Mon Sep 17 00:00:00 2001 From: Alex E Date: Mon, 23 Dec 2024 22:14:54 -0500 Subject: [PATCH 01/30] Add environment-related logic --- .../exporters/otlp/otlp_environment.h | 18 +++ exporters/otlp/src/otlp_environment.cc | 112 ++++++++++++++++++ 2 files changed, 130 insertions(+) diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_environment.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_environment.h index 74a222a9d0..799e07f27d 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_environment.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_environment.h @@ -18,6 +18,8 @@ namespace exporter namespace otlp { +using SecondsDecimal = std::chrono::duration>; + inline std::string GetOtlpDefaultUserAgent() { return "OTel-OTLP-Exporter-Cpp/" OPENTELEMETRY_SDK_VERSION; @@ -152,6 +154,22 @@ std::string GetOtlpDefaultTracesCompression(); std::string GetOtlpDefaultMetricsCompression(); std::string GetOtlpDefaultLogsCompression(); +std::uint32_t GetOtlpDefaultTracesRetryMaxAttempts(); +std::uint32_t GetOtlpDefaultMetricsRetryMaxAttempts(); +std::uint32_t GetOtlpDefaultLogsRetryMaxAttempts(); + +SecondsDecimal GetOtlpDefaultTracesRetryInitialBackoff(); +SecondsDecimal GetOtlpDefaultMetricsRetryInitialBackoff(); +SecondsDecimal GetOtlpDefaultLogsRetryInitialBackoff(); + +SecondsDecimal GetOtlpDefaultTracesRetryMaxBackoff(); +SecondsDecimal GetOtlpDefaultMetricsRetryMaxBackoff(); +SecondsDecimal GetOtlpDefaultLogsRetryMaxBackoff(); + +float GetOtlpDefaultTracesRetryBackoffMultiplier(); +float GetOtlpDefaultMetricsRetryBackoffMultiplier(); +float GetOtlpDefaultLogsRetryBackoffMultiplier(); + } // namespace otlp } // namespace exporter OPENTELEMETRY_END_NAMESPACE diff --git a/exporters/otlp/src/otlp_environment.cc b/exporters/otlp/src/otlp_environment.cc index 276d9fc709..a9e43acb08 100644 --- a/exporters/otlp/src/otlp_environment.cc +++ b/exporters/otlp/src/otlp_environment.cc @@ -80,6 +80,34 @@ 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) +{ + std::string value; + + if (GetStringDualEnvVar(signal_env.data(), generic_env.data(), value)) + { + return static_cast(std::strtoul(value.c_str(), nullptr, 10)); + } + + return default_value; +} + +static float GetFloatEnvVarOrDefault(opentelemetry::nostd::string_view signal_env, + opentelemetry::nostd::string_view generic_env, + float default_value) +{ + std::string value; + + if (GetStringDualEnvVar(signal_env.data(), generic_env.data(), value)) + { + return std::strtof(value.c_str(), nullptr); + } + + return default_value; +} + std::string GetOtlpDefaultGrpcTracesEndpoint() { constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT"; @@ -1125,6 +1153,90 @@ std::string GetOtlpDefaultLogsCompression() return std::string{"none"}; } +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 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 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); +} + +SecondsDecimal GetOtlpDefaultTracesRetryInitialBackoff() +{ + constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_TRACES_RETRY_INITIAL_BACKOFF"; + constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"; + return SecondsDecimal{GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.0)}; +} + +SecondsDecimal GetOtlpDefaultMetricsRetryInitialBackoff() +{ + constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_METRICS_RETRY_INITIAL_BACKOFF"; + constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"; + return SecondsDecimal{GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.0)}; +} + +SecondsDecimal GetOtlpDefaultLogsRetryInitialBackoff() +{ + constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_LOGS_RETRY_INITIAL_BACKOFF"; + constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"; + return SecondsDecimal{GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.0)}; +} + +SecondsDecimal GetOtlpDefaultTracesRetryMaxBackoff() +{ + constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_BACKOFF"; + constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"; + return SecondsDecimal{GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 5.0)}; +} + +SecondsDecimal GetOtlpDefaultMetricsRetryMaxBackoff() +{ + constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_BACKOFF"; + constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"; + return SecondsDecimal{GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 5.0)}; +} + +SecondsDecimal GetOtlpDefaultLogsRetryMaxBackoff() +{ + constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_BACKOFF"; + constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"; + return SecondsDecimal{GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 5.0)}; +} + +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 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 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); +} + } // namespace otlp } // namespace exporter OPENTELEMETRY_END_NAMESPACE From ed07242937060afd9b8be123e4d869474d7e190f Mon Sep 17 00:00:00 2001 From: Alex E Date: Mon, 23 Dec 2024 22:16:01 -0500 Subject: [PATCH 02/30] Update options with retry policy settings --- .../exporters/otlp/otlp_http_exporter_options.h | 13 +++++++++++++ .../otlp/otlp_http_log_record_exporter_options.h | 13 +++++++++++++ .../otlp/otlp_http_metric_exporter_options.h | 13 +++++++++++++ exporters/otlp/src/otlp_http_exporter_options.cc | 6 +++++- .../src/otlp_http_log_record_exporter_options.cc | 6 +++++- .../otlp/src/otlp_http_metric_exporter_options.cc | 6 +++++- 6 files changed, 54 insertions(+), 3 deletions(-) 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 index 7f6d5a1b35..1488484181 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter_options.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter_options.h @@ -4,6 +4,7 @@ #pragma once #include +#include #include #include "opentelemetry/exporters/otlp/otlp_environment.h" @@ -101,6 +102,18 @@ struct OPENTELEMETRY_EXPORT OtlpHttpExporterOptions /** Compression type. */ std::string compression; + + /** The maximum number of call attempts, including the original attempt. */ + std::uint32_t retry_policy_max_attempts{}; + + /** The initial backoff delay between retry attempts, random between (0, initial_backoff). */ + SecondsDecimal retry_policy_initial_backoff{}; + + /** The maximum backoff places an upper limit on exponential backoff growth. */ + SecondsDecimal retry_policy_max_backoff{}; + + /** The backoff will be multiplied by this value after each retry attempt. */ + float retry_policy_backoff_multiplier{}; }; } // namespace otlp diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_log_record_exporter_options.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_log_record_exporter_options.h index 60f674a3a7..43157dd7ca 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_log_record_exporter_options.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_log_record_exporter_options.h @@ -4,6 +4,7 @@ #pragma once #include +#include #include #include "opentelemetry/exporters/otlp/otlp_environment.h" @@ -101,6 +102,18 @@ struct OPENTELEMETRY_EXPORT OtlpHttpLogRecordExporterOptions /** Compression type. */ std::string compression; + + /** The maximum number of call attempts, including the original attempt. */ + std::uint32_t retry_policy_max_attempts{}; + + /** The initial backoff delay between retry attempts, random between (0, initial_backoff). */ + SecondsDecimal retry_policy_initial_backoff{}; + + /** The maximum backoff places an upper limit on exponential backoff growth. */ + SecondsDecimal retry_policy_max_backoff{}; + + /** The backoff will be multiplied by this value after each retry attempt. */ + float retry_policy_backoff_multiplier{}; }; } // namespace otlp diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_metric_exporter_options.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_metric_exporter_options.h index 82c91aa51f..7ec57a36b6 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_metric_exporter_options.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_metric_exporter_options.h @@ -4,6 +4,7 @@ #pragma once #include +#include #include #include "opentelemetry/exporters/otlp/otlp_environment.h" @@ -104,6 +105,18 @@ struct OPENTELEMETRY_EXPORT OtlpHttpMetricExporterOptions /** Compression type. */ std::string compression; + + /** The maximum number of call attempts, including the original attempt. */ + std::uint32_t retry_policy_max_attempts{}; + + /** The initial backoff delay between retry attempts, random between (0, initial_backoff). */ + SecondsDecimal retry_policy_initial_backoff{}; + + /** The maximum backoff places an upper limit on exponential backoff growth. */ + SecondsDecimal retry_policy_max_backoff{}; + + /** The backoff will be multiplied by this value after each retry attempt. */ + float retry_policy_backoff_multiplier{}; }; } // namespace otlp diff --git a/exporters/otlp/src/otlp_http_exporter_options.cc b/exporters/otlp/src/otlp_http_exporter_options.cc index f6f7027f50..34ba5e173a 100644 --- a/exporters/otlp/src/otlp_http_exporter_options.cc +++ b/exporters/otlp/src/otlp_http_exporter_options.cc @@ -31,7 +31,11 @@ OtlpHttpExporterOptions::OtlpHttpExporterOptions() ssl_max_tls(GetOtlpDefaultTracesSslTlsMaxVersion()), ssl_cipher(GetOtlpDefaultTracesSslTlsCipher()), ssl_cipher_suite(GetOtlpDefaultTracesSslTlsCipherSuite()), - compression(GetOtlpDefaultTracesCompression()) + compression(GetOtlpDefaultTracesCompression()), + retry_policy_max_attempts(GetOtlpDefaultTracesRetryMaxAttempts()), + retry_policy_initial_backoff(GetOtlpDefaultTracesRetryInitialBackoff()), + retry_policy_max_backoff(GetOtlpDefaultTracesRetryMaxBackoff()), + retry_policy_backoff_multiplier(GetOtlpDefaultTracesRetryBackoffMultiplier()) { #ifdef ENABLE_ASYNC_EXPORT max_concurrent_requests = 64; diff --git a/exporters/otlp/src/otlp_http_log_record_exporter_options.cc b/exporters/otlp/src/otlp_http_log_record_exporter_options.cc index b38a292476..d3c6742cb2 100644 --- a/exporters/otlp/src/otlp_http_log_record_exporter_options.cc +++ b/exporters/otlp/src/otlp_http_log_record_exporter_options.cc @@ -31,7 +31,11 @@ OtlpHttpLogRecordExporterOptions::OtlpHttpLogRecordExporterOptions() ssl_max_tls(GetOtlpDefaultLogsSslTlsMaxVersion()), ssl_cipher(GetOtlpDefaultLogsSslTlsCipher()), ssl_cipher_suite(GetOtlpDefaultLogsSslTlsCipherSuite()), - compression(GetOtlpDefaultLogsCompression()) + compression(GetOtlpDefaultLogsCompression()), + retry_policy_max_attempts(GetOtlpDefaultLogsRetryMaxAttempts()), + retry_policy_initial_backoff(GetOtlpDefaultLogsRetryInitialBackoff()), + retry_policy_max_backoff(GetOtlpDefaultLogsRetryMaxBackoff()), + retry_policy_backoff_multiplier(GetOtlpDefaultLogsRetryBackoffMultiplier()) { #ifdef ENABLE_ASYNC_EXPORT max_concurrent_requests = 64; diff --git a/exporters/otlp/src/otlp_http_metric_exporter_options.cc b/exporters/otlp/src/otlp_http_metric_exporter_options.cc index b5fb83b090..eb11fe2a88 100644 --- a/exporters/otlp/src/otlp_http_metric_exporter_options.cc +++ b/exporters/otlp/src/otlp_http_metric_exporter_options.cc @@ -33,7 +33,11 @@ OtlpHttpMetricExporterOptions::OtlpHttpMetricExporterOptions() ssl_max_tls(GetOtlpDefaultMetricsSslTlsMaxVersion()), ssl_cipher(GetOtlpDefaultMetricsSslTlsCipher()), ssl_cipher_suite(GetOtlpDefaultMetricsSslTlsCipherSuite()), - compression(GetOtlpDefaultMetricsCompression()) + compression(GetOtlpDefaultMetricsCompression()), + retry_policy_max_attempts(GetOtlpDefaultMetricsRetryMaxAttempts()), + retry_policy_initial_backoff(GetOtlpDefaultMetricsRetryInitialBackoff()), + retry_policy_max_backoff(GetOtlpDefaultMetricsRetryMaxBackoff()), + retry_policy_backoff_multiplier(GetOtlpDefaultMetricsRetryBackoffMultiplier()) { #ifdef ENABLE_ASYNC_EXPORT max_concurrent_requests = 64; From 1dba246600864ba4f7e1065aa8cb61ee765c461e Mon Sep 17 00:00:00 2001 From: Alex E Date: Mon, 23 Dec 2024 22:17:35 -0500 Subject: [PATCH 03/30] Test config and retry mechanism --- .../otlp/test/otlp_http_exporter_test.cc | 159 +++++++++++++++++- .../otlp_http_log_record_exporter_test.cc | 51 +++++- .../test/otlp_http_metric_exporter_test.cc | 52 +++++- 3 files changed, 259 insertions(+), 3 deletions(-) diff --git a/exporters/otlp/test/otlp_http_exporter_test.cc b/exporters/otlp/test/otlp_http_exporter_test.cc index 4c4a6dbeb2..c30b6df738 100644 --- a/exporters/otlp/test/otlp_http_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_exporter_test.cc @@ -7,6 +7,7 @@ # include # include "opentelemetry/exporters/otlp/otlp_http_exporter.h" +# include "opentelemetry/exporters/otlp/otlp_http_exporter_factory.h" # include "opentelemetry/exporters/otlp/protobuf_include_prefix.h" @@ -18,10 +19,14 @@ # include "opentelemetry/ext/http/server/http_server.h" # include "opentelemetry/sdk/trace/batch_span_processor.h" # include "opentelemetry/sdk/trace/batch_span_processor_options.h" +# include "opentelemetry/sdk/trace/simple_processor.h" +# include "opentelemetry/sdk/trace/simple_processor_factory.h" # include "opentelemetry/sdk/trace/tracer_provider.h" +# include "opentelemetry/sdk/trace/tracer_provider_factory.h" # include "opentelemetry/test_common/ext/http/client/http_client_test_factory.h" # include "opentelemetry/test_common/ext/http/client/nosend/http_client_nosend.h" # include "opentelemetry/trace/provider.h" +# include "opentelemetry/trace/tracer_provider.h" # include # include @@ -620,7 +625,159 @@ TEST_F(OtlpHttpExporterTestPeer, ConfigFromTracesEnv) unsetenv("OTEL_EXPORTER_OTLP_TRACES_HEADERS"); unsetenv("OTEL_EXPORTER_OTLP_TRACES_PROTOCOL"); } -# endif + +TEST_F(OtlpHttpExporterTestPeer, ConfigRetryDefaultValues) +{ + std::unique_ptr exporter(new OtlpHttpExporter()); + const auto options = GetOptions(exporter); + ASSERT_EQ(options.retry_policy_max_attempts, 5); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 1); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 5); + ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 1.5); +} + +TEST_F(OtlpHttpExporterTestPeer, ConfigRetryValuesFromEnv) +{ + setenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_ATTEMPTS", "123", 1); + setenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_INITIAL_BACKOFF", "4.5", 1); + setenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_BACKOFF", "6.7", 1); + setenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_BACKOFF_MULTIPLIER", "8.9", 1); + + std::unique_ptr exporter(new OtlpHttpExporter()); + const auto options = GetOptions(exporter); + ASSERT_EQ(options.retry_policy_max_attempts, 123); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 4.5); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 6.7); + ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 8.9); + + unsetenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_ATTEMPTS"); + unsetenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_INITIAL_BACKOFF"); + unsetenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_BACKOFF"); + unsetenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_BACKOFF_MULTIPLIER"); +} + +TEST_F(OtlpHttpExporterTestPeer, ConfigRetryGenericValuesFromEnv) +{ + setenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS", "321", 1); + setenv("OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF", "5.4", 1); + setenv("OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF", "7.6", 1); + setenv("OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER", "9.8", 1); + + std::unique_ptr exporter(new OtlpHttpExporter()); + const auto options = GetOptions(exporter); + ASSERT_EQ(options.retry_policy_max_attempts, 321); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 5.4); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 7.6); + ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 9.8); + + unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"); + unsetenv("OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"); + unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"); + unsetenv("OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"); +} +# endif // NO_GETENV + +using StatusCodeVector = std::vector; + +class OtlpHttpExporterRetryIntegrationTests + : public ::testing::TestWithParam> +{}; + +INSTANTIATE_TEST_SUITE_P(StatusCodes, + OtlpHttpExporterRetryIntegrationTests, + testing::Values( + // With retry policy enabled + std::make_tuple(true, StatusCodeVector{100}, 1), + std::make_tuple(true, StatusCodeVector{200}, 1), + std::make_tuple(true, StatusCodeVector{201}, 1), + std::make_tuple(true, StatusCodeVector{202}, 1), + std::make_tuple(true, StatusCodeVector{204}, 1), + std::make_tuple(true, StatusCodeVector{302}, 1), + std::make_tuple(true, StatusCodeVector{400}, 1), + std::make_tuple(true, StatusCodeVector{401}, 1), + std::make_tuple(true, StatusCodeVector{403}, 1), + std::make_tuple(true, StatusCodeVector{404}, 1), + std::make_tuple(true, StatusCodeVector{405}, 1), + std::make_tuple(true, StatusCodeVector{429}, 5), + std::make_tuple(true, StatusCodeVector{500}, 1), + std::make_tuple(true, StatusCodeVector{501}, 1), + std::make_tuple(true, StatusCodeVector{502}, 5), + std::make_tuple(true, StatusCodeVector{503}, 5), + std::make_tuple(true, StatusCodeVector{504}, 5), + std::make_tuple(true, StatusCodeVector{429, 502, 503, 504}, 5), + std::make_tuple(true, StatusCodeVector{503, 503, 503, 200}, 4), + std::make_tuple(true, StatusCodeVector{429, 503, 504, 200}, 4), + // With retry policy disabled + std::make_tuple(false, StatusCodeVector{100}, 1), + std::make_tuple(false, StatusCodeVector{200}, 1), + std::make_tuple(false, StatusCodeVector{201}, 1), + std::make_tuple(false, StatusCodeVector{202}, 1), + std::make_tuple(false, StatusCodeVector{204}, 1), + std::make_tuple(false, StatusCodeVector{302}, 1), + std::make_tuple(false, StatusCodeVector{400}, 1), + std::make_tuple(false, StatusCodeVector{401}, 1), + std::make_tuple(false, StatusCodeVector{403}, 1), + std::make_tuple(false, StatusCodeVector{404}, 1), + std::make_tuple(false, StatusCodeVector{405}, 1), + std::make_tuple(false, StatusCodeVector{429}, 1), + std::make_tuple(false, StatusCodeVector{500}, 1), + std::make_tuple(false, StatusCodeVector{501}, 1), + std::make_tuple(false, StatusCodeVector{502}, 1), + std::make_tuple(false, StatusCodeVector{503}, 1), + std::make_tuple(false, StatusCodeVector{504}, 1), + std::make_tuple(true, StatusCodeVector{429, 502, 503, 504}, 1), + std::make_tuple(true, StatusCodeVector{503, 503, 503, 200}, 1), + std::make_tuple(true, StatusCodeVector{429, 503, 504, 200}, 1))); + +TEST_P(OtlpHttpExporterRetryIntegrationTests, StatusCodes) +{ + namespace otlp = opentelemetry::exporter::otlp; + namespace trace_sdk = opentelemetry::sdk::trace; + + const auto is_retry_enabled = std::get<0>(GetParam()); + const auto status_codes = std::get<1>(GetParam()); + const auto expected_attempts = std::get<2>(GetParam()); + + size_t request_count = 0UL; + HTTP_SERVER_NS::HttpRequestCallback request_handler{ + [&request_count, &status_codes](HTTP_SERVER_NS::HttpRequest const &request, + HTTP_SERVER_NS::HttpResponse &response) { + response.body = "TEST!"; + response.code = status_codes.at(request_count++ % status_codes.size()); + return response.code; + }}; + HTTP_SERVER_NS::HttpServer server; + server.setKeepalive(false); + server.addHandler("/v1/traces", request_handler); + ASSERT_EQ(server.addListeningPort(4318), 4318); + server.start(); + + otlp::OtlpHttpExporterOptions opts{}; + + if (is_retry_enabled) + { + opts.retry_policy_max_attempts = 5; + opts.retry_policy_initial_backoff = SecondsDecimal{0.1}; + opts.retry_policy_max_backoff = SecondsDecimal{5}; + opts.retry_policy_backoff_multiplier = 1; + } + else + { + opts.retry_policy_max_attempts = 0; + opts.retry_policy_initial_backoff = SecondsDecimal{0}; + opts.retry_policy_max_backoff = SecondsDecimal{0}; + opts.retry_policy_backoff_multiplier = 0; + } + + auto exporter = otlp::OtlpHttpExporterFactory::Create(opts); + auto processor = trace_sdk::SimpleSpanProcessorFactory::Create(std::move(exporter)); + auto provider = trace_sdk::TracerProviderFactory::Create(std::move(processor)); + provider->GetTracer("Test tracer")->StartSpan("Test span")->End(); + provider->ForceFlush(); + server.stop(); + + ASSERT_EQ(expected_attempts, request_count); +} } // namespace otlp } // namespace exporter diff --git a/exporters/otlp/test/otlp_http_log_record_exporter_test.cc b/exporters/otlp/test/otlp_http_log_record_exporter_test.cc index 35a0bb62e2..f12899e677 100644 --- a/exporters/otlp/test/otlp_http_log_record_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_log_record_exporter_test.cc @@ -748,7 +748,56 @@ TEST_F(OtlpHttpLogRecordExporterTestPeer, DefaultEndpoint) EXPECT_EQ("http://localhost:4317", GetOtlpDefaultGrpcEndpoint()); } -# endif +TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigRetryDefaultValues) +{ + std::unique_ptr exporter(new OtlpHttpLogRecordExporter()); + const auto options = GetOptions(exporter); + ASSERT_EQ(options.retry_policy_max_attempts, 5); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 1); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 5); + ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 1.5); +} + +TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigRetryValuesFromEnv) +{ + setenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_ATTEMPTS", "123", 1); + setenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_INITIAL_BACKOFF", "4.5", 1); + setenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_BACKOFF", "6.7", 1); + setenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_BACKOFF_MULTIPLIER", "8.9", 1); + + std::unique_ptr exporter(new OtlpHttpLogRecordExporter()); + const auto options = GetOptions(exporter); + ASSERT_EQ(options.retry_policy_max_attempts, 123); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 4.5); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 6.7); + ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 8.9); + + unsetenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_ATTEMPTS"); + unsetenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_INITIAL_BACKOFF"); + unsetenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_BACKOFF"); + unsetenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_BACKOFF_MULTIPLIER"); +} + +TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigRetryGenericValuesFromEnv) +{ + setenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS", "321", 1); + setenv("OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF", "5.4", 1); + setenv("OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF", "7.6", 1); + setenv("OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER", "9.8", 1); + + std::unique_ptr exporter(new OtlpHttpLogRecordExporter()); + const auto options = GetOptions(exporter); + ASSERT_EQ(options.retry_policy_max_attempts, 321); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 5.4); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 7.6); + ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 9.8); + + unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"); + unsetenv("OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"); + unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"); + unsetenv("OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"); +} +# endif // NO_GETENV } // namespace otlp } // namespace exporter diff --git a/exporters/otlp/test/otlp_http_metric_exporter_test.cc b/exporters/otlp/test/otlp_http_metric_exporter_test.cc index 0b44d98379..222c22c983 100644 --- a/exporters/otlp/test/otlp_http_metric_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_metric_exporter_test.cc @@ -1003,7 +1003,57 @@ TEST_F(OtlpHttpMetricExporterTestPeer, CheckDefaultTemporality) exporter->GetAggregationTemporality( opentelemetry::sdk::metrics::InstrumentType::kObservableUpDownCounter)); } -#endif + +TEST_F(OtlpHttpMetricExporterTestPeer, ConfigRetryDefaultValues) +{ + std::unique_ptr exporter(new OtlpHttpMetricExporter()); + const auto options = GetOptions(exporter); + ASSERT_EQ(options.retry_policy_max_attempts, 5); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 1); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 5); + ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 1.5); +} + +TEST_F(OtlpHttpMetricExporterTestPeer, ConfigRetryValuesFromEnv) +{ + setenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_ATTEMPTS", "123", 1); + setenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_INITIAL_BACKOFF", "4.5", 1); + setenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_BACKOFF", "6.7", 1); + setenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_BACKOFF_MULTIPLIER", "8.9", 1); + + std::unique_ptr exporter(new OtlpHttpMetricExporter()); + const auto options = GetOptions(exporter); + ASSERT_EQ(options.retry_policy_max_attempts, 123); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 4.5); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 6.7); + ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 8.9); + + unsetenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_ATTEMPTS"); + unsetenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_INITIAL_BACKOFF"); + unsetenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_BACKOFF"); + unsetenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_BACKOFF_MULTIPLIER"); +} + +TEST_F(OtlpHttpMetricExporterTestPeer, ConfigRetryGenericValuesFromEnv) +{ + setenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS", "321", 1); + setenv("OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF", "5.4", 1); + setenv("OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF", "7.6", 1); + setenv("OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER", "9.8", 1); + + std::unique_ptr exporter(new OtlpHttpMetricExporter()); + const auto options = GetOptions(exporter); + ASSERT_EQ(options.retry_policy_max_attempts, 321); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 5.4); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 7.6); + ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 9.8); + + unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"); + unsetenv("OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"); + unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"); + unsetenv("OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"); +} +#endif // NO_GETENV // Test Preferred aggregtion temporality selection TEST_F(OtlpHttpMetricExporterTestPeer, PreferredAggergationTemporality) From ae47b20f25c8a822bf0fce7630131758053a26b8 Mon Sep 17 00:00:00 2001 From: Alex E Date: Sat, 28 Dec 2024 23:25:39 -0500 Subject: [PATCH 04/30] Functional retry policy --- .../exporters/otlp/otlp_http_client.h | 9 ++ exporters/otlp/src/otlp_http_client.cc | 1 + exporters/otlp/src/otlp_http_exporter.cc | 6 +- .../otlp/test/otlp_http_exporter_test.cc | 10 ++- .../ext/http/client/curl/http_client_curl.h | 10 +++ .../http/client/curl/http_operation_curl.h | 35 ++++++-- .../ext/http/client/http_client.h | 24 +++++ ext/src/http/client/curl/http_client_curl.cc | 90 +++++++++++++++---- .../http/client/curl/http_operation_curl.cc | 53 ++++++++--- .../http/client/nosend/http_client_nosend.h | 7 ++ 10 files changed, 202 insertions(+), 43 deletions(-) diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h index 3753704bb3..9875d586fe 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h @@ -80,6 +80,9 @@ struct OtlpHttpClientOptions // Requests per connections std::size_t max_requests_per_connection = 8; + // Retry policy for select failure codes + ext::http::client::RetryPolicy retry_policy; + // User agent std::string user_agent; @@ -102,6 +105,10 @@ struct OtlpHttpClientOptions bool input_console_debug, std::chrono::system_clock::duration input_timeout, const OtlpHeaders &input_http_headers, + std::uint32_t input_retry_policy_max_attempts, + SecondsDecimal input_retry_policy_initial_backoff, + SecondsDecimal input_retry_policy_max_backoff, + float input_retry_policy_backoff_multiplier, std::size_t input_concurrent_sessions = 64, std::size_t input_max_requests_per_connection = 8, nostd::string_view input_user_agent = GetOtlpDefaultUserAgent()) @@ -125,6 +132,8 @@ struct OtlpHttpClientOptions console_debug(input_console_debug), timeout(input_timeout), http_headers(input_http_headers), + retry_policy{input_retry_policy_max_attempts, input_retry_policy_initial_backoff, + input_retry_policy_max_backoff, input_retry_policy_backoff_multiplier}, max_concurrent_requests(input_concurrent_sessions), max_requests_per_connection(input_max_requests_per_connection), user_agent(input_user_agent) diff --git a/exporters/otlp/src/otlp_http_client.cc b/exporters/otlp/src/otlp_http_client.cc index a2620756cc..29cdacf618 100644 --- a/exporters/otlp/src/otlp_http_client.cc +++ b/exporters/otlp/src/otlp_http_client.cc @@ -991,6 +991,7 @@ OtlpHttpClient::createSession( request->ReplaceHeader("Content-Type", content_type); request->ReplaceHeader("User-Agent", options_.user_agent); request->EnableLogging(options_.console_debug); + request->SetRetryPolicy(options_.retry_policy); if (options_.compression == "gzip") { diff --git a/exporters/otlp/src/otlp_http_exporter.cc b/exporters/otlp/src/otlp_http_exporter.cc index f740a0babf..c0efcc47a8 100644 --- a/exporters/otlp/src/otlp_http_exporter.cc +++ b/exporters/otlp/src/otlp_http_exporter.cc @@ -57,7 +57,11 @@ OtlpHttpExporter::OtlpHttpExporter(const OtlpHttpExporterOptions &options) options.use_json_name, options.console_debug, options.timeout, - options.http_headers + options.http_headers, + options.retry_policy_max_attempts, + options.retry_policy_initial_backoff, + options.retry_policy_max_backoff, + options.retry_policy_backoff_multiplier #ifdef ENABLE_ASYNC_EXPORT , options.max_concurrent_requests, diff --git a/exporters/otlp/test/otlp_http_exporter_test.cc b/exporters/otlp/test/otlp_http_exporter_test.cc index c30b6df738..2e4c168e80 100644 --- a/exporters/otlp/test/otlp_http_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_exporter_test.cc @@ -75,7 +75,8 @@ OtlpHttpClientOptions MakeOtlpHttpClientOptions(HttpRequestContentType content_t "", /* ssl_cipher */ "", /* ssl_cipher_suite */ options.content_type, options.json_bytes_mapping, options.compression, options.use_json_name, - options.console_debug, options.timeout, options.http_headers); + options.console_debug, options.timeout, options.http_headers, 0, SecondsDecimal::zero(), + SecondsDecimal::zero(), 0); if (!async_mode) { otlp_http_client_options.max_concurrent_requests = 0; @@ -725,9 +726,9 @@ INSTANTIATE_TEST_SUITE_P(StatusCodes, std::make_tuple(false, StatusCodeVector{502}, 1), std::make_tuple(false, StatusCodeVector{503}, 1), std::make_tuple(false, StatusCodeVector{504}, 1), - std::make_tuple(true, StatusCodeVector{429, 502, 503, 504}, 1), - std::make_tuple(true, StatusCodeVector{503, 503, 503, 200}, 1), - std::make_tuple(true, StatusCodeVector{429, 503, 504, 200}, 1))); + std::make_tuple(false, StatusCodeVector{429, 502, 503, 504}, 1), + std::make_tuple(false, StatusCodeVector{503, 503, 503, 200}, 1), + std::make_tuple(false, StatusCodeVector{429, 503, 504, 200}, 1))); TEST_P(OtlpHttpExporterRetryIntegrationTests, StatusCodes) { @@ -748,6 +749,7 @@ TEST_P(OtlpHttpExporterRetryIntegrationTests, StatusCodes) }}; HTTP_SERVER_NS::HttpServer server; server.setKeepalive(false); + server.setServerName("test_server"); server.addHandler("/v1/traces", request_handler); ASSERT_EQ(server.addListeningPort(4318), 4318); server.start(); diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index bef15997fc..728558691c 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -104,6 +105,12 @@ class Request : public opentelemetry::ext::http::client::Request void EnableLogging(bool is_log_enabled) noexcept override { is_log_enabled_ = is_log_enabled; } + void SetRetryPolicy( + const opentelemetry::ext::http::client::RetryPolicy &retry_policy) noexcept override + { + retry_policy_ = retry_policy; + } + public: opentelemetry::ext::http::client::Method method_; opentelemetry::ext::http::client::HttpSslOptions ssl_options_; @@ -114,6 +121,7 @@ class Request : public opentelemetry::ext::http::client::Request opentelemetry::ext::http::client::Compression compression_{ opentelemetry::ext::http::client::Compression::kNone}; bool is_log_enabled_{false}; + opentelemetry::ext::http::client::RetryPolicy retry_policy_; }; class Response : public opentelemetry::ext::http::client::Response @@ -338,6 +346,7 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient bool doAddSessions(); bool doAbortSessions(); bool doRemoveSessions(); + bool doRetrySessions(); void resetMultiHandle(); std::mutex multi_handle_m_; @@ -352,6 +361,7 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient std::unordered_map> pending_to_abort_sessions_; std::unordered_map pending_to_remove_session_handles_; std::list> pending_to_remove_sessions_; + std::deque pending_to_retry_sessions_; std::mutex background_thread_m_; std::unique_ptr background_thread_; diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h index b94c53b2d0..b6ad5a66a3 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h @@ -135,14 +135,17 @@ class HttpOperation /** * Create local CURL instance for url and body - * @param method // HTTP Method - * @param url // HTTP URL + * @param method HTTP Method + * @param url HTTP URL * @param callback - * @param request_mode // sync or async - * @param request Request Headers - * @param body Reques Body - * @param raw_response whether to parse the response - * @param httpConnTimeout HTTP connection timeout in seconds + * @param request_mode Sync or async + * @param request Request Headers + * @param body Request Body + * @param raw_response Whether to parse the response + * @param http_conn_timeout HTTP connection timeout in seconds + * @param reuse_connection Whether connection should be reused or closed + * @param is_log_enabled To intercept some information from cURL request + * @param retry_policy Retry policy for select failure status codes */ HttpOperation(opentelemetry::ext::http::client::Method method, std::string url, @@ -159,7 +162,8 @@ class HttpOperation bool is_raw_response = false, std::chrono::milliseconds http_conn_timeout = default_http_conn_timeout, bool reuse_connection = false, - bool is_log_enabled = false); + bool is_log_enabled = false, + const opentelemetry::ext::http::client::RetryPolicy &retry_policy = {}); /** * Destroy CURL instance @@ -176,6 +180,16 @@ class HttpOperation */ void Cleanup(); + /** + * Determine if operation is retryable + */ + bool IsRetryable(); + + /** + * Calculate next time to retry request + */ + std::chrono::system_clock::time_point NextRetryTime(); + /** * Setup request */ @@ -216,7 +230,7 @@ class HttpOperation bool WasAborted() { return is_aborted_.load(std::memory_order_acquire); } /** - * Return a copy of resposne headers + * Return a copy of response headers * * @return */ @@ -309,6 +323,9 @@ class HttpOperation const bool is_log_enabled_; + RetryPolicy retry_policy_; + std::chrono::system_clock::time_point last_attempt_time_; + // Processed response headers and body long response_code_; std::vector response_headers_; diff --git a/ext/include/opentelemetry/ext/http/client/http_client.h b/ext/include/opentelemetry/ext/http/client/http_client.h index e467f9ef63..36f4e2d0a6 100644 --- a/ext/include/opentelemetry/ext/http/client/http_client.h +++ b/ext/include/opentelemetry/ext/http/client/http_client.h @@ -226,6 +226,28 @@ struct HttpSslOptions std::string ssl_cipher_suite{}; }; +using SecondsDecimal = std::chrono::duration>; + +struct RetryPolicy +{ + RetryPolicy() = default; + + RetryPolicy(std::uint32_t input_max_attempts, + SecondsDecimal input_initial_backoff, + SecondsDecimal input_max_backoff, + float input_backoff_multiplier) + : max_attempts(input_max_attempts), + initial_backoff(input_initial_backoff), + max_backoff(input_max_backoff), + backoff_multiplier(input_backoff_multiplier) + {} + + std::uint32_t max_attempts{}; + SecondsDecimal initial_backoff{}; + SecondsDecimal max_backoff{}; + float backoff_multiplier{}; +}; + class Request { public: @@ -247,6 +269,8 @@ class Request virtual void EnableLogging(bool is_log_enabled) noexcept = 0; + virtual void SetRetryPolicy(const RetryPolicy &retry_policy) noexcept = 0; + virtual ~Request() = default; }; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index a2b73ecb2b..e96da867d4 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -196,10 +196,11 @@ void Session::SendRequest( #endif // ENABLE_OTLP_COMPRESSION_PREVIEW } - curl_operation_.reset(new HttpOperation( - http_request_->method_, url, http_request_->ssl_options_, callback_ptr, - http_request_->headers_, http_request_->body_, http_request_->compression_, false, - http_request_->timeout_ms_, reuse_connection, http_request_->is_log_enabled_)); + curl_operation_.reset( + new HttpOperation(http_request_->method_, url, http_request_->ssl_options_, callback_ptr, + http_request_->headers_, http_request_->body_, http_request_->compression_, + false, http_request_->timeout_ms_, reuse_connection, + http_request_->is_log_enabled_, http_request_->retry_policy_)); bool success = CURLE_OK == curl_operation_->SendAsync(this, [this, callback](HttpOperation &operation) { if (operation.WasAborted()) @@ -222,8 +223,8 @@ void Session::SendRequest( if (success) { - // We will try to create a background to poll events.But when the background is running, we will - // reuse it instead of creating a new one. + // We will try to create a background to poll events. But when the background is running, we + // will reuse it instead of creating a new one. http_client_.MaybeSpawnBackgroundThread(); } else @@ -319,7 +320,7 @@ std::shared_ptr HttpClient::CreateSes std::lock_guard lock_guard{sessions_m_}; sessions_.insert({session_id, session}); - // FIXME: Session may leak if it do not call SendRequest + // FIXME: Session may leak if it does not call SendRequest return session; } @@ -428,16 +429,15 @@ bool HttpClient::MaybeSpawnBackgroundThread() background_thread_.reset(new std::thread( [](HttpClient *self) { - int still_running = 1; - std::chrono::system_clock::time_point last_free_job_timepoint = - std::chrono::system_clock::now(); - bool need_wait_more = false; + auto still_running = 1; + auto last_free_job_timepoint = std::chrono::system_clock::now(); + auto need_wait_more = false; + while (true) { - CURLMsg *msg; - int queued; + CURLMsg *msg = nullptr; + int queued = 0; CURLMcode mc = curl_multi_perform(self->multi_handle_, &still_running); - // According to https://curl.se/libcurl/c/curl_multi_perform.html, when mc is not OK, we // can not curl_multi_perform it again if (mc != CURLM_OK) @@ -446,8 +446,8 @@ bool HttpClient::MaybeSpawnBackgroundThread() } else if (still_running || need_wait_more) { - // curl_multi_poll is added from libcurl 7.66.0, before 7.68.0, we can only wait util - // timeout to do the rest jobs + // curl_multi_poll is added from libcurl 7.66.0, before 7.68.0, we can only wait until + // timeout to do the remaining jobs #if LIBCURL_VERSION_NUM >= 0x074200 /* wait for activity, timeout or "nothing" */ mc = curl_multi_poll(self->multi_handle_, nullptr, 0, @@ -474,13 +474,20 @@ bool HttpClient::MaybeSpawnBackgroundThread() CURLcode result = msg->data.result; Session *session = nullptr; curl_easy_getinfo(easy_handle, CURLINFO_PRIVATE, &session); - // If it's already moved into pending_to_remove_session_handles_, we just ingore this + const auto operation = (nullptr != session) ? session->GetOperation().get() : nullptr; + + // If it's already moved into pending_to_remove_session_handles_, we just ignore this // message. - if (nullptr != session && session->GetOperation()) + if (operation) { // Session can not be destroyed when calling PerformCurlMessage auto hold_session = session->shared_from_this(); - session->GetOperation()->PerformCurlMessage(result); + operation->PerformCurlMessage(result); + + if (operation->IsRetryable()) + { + self->pending_to_retry_sessions_.push_front(session); + } } } } while (true); @@ -503,6 +510,12 @@ bool HttpClient::MaybeSpawnBackgroundThread() still_running = 1; } + // Check if pending easy handles can be retried + if (self->doRetrySessions()) + { + still_running = 1; + } + std::chrono::system_clock::time_point now = std::chrono::system_clock::now(); if (still_running > 0) { @@ -553,6 +566,12 @@ bool HttpClient::MaybeSpawnBackgroundThread() still_running = 1; } + // Check if pending easy handles can be retried + if (self->doRetrySessions()) + { + still_running = 1; + } + // If there is no pending jobs, we can stop the background thread. if (still_running == 0) { @@ -769,6 +788,39 @@ bool HttpClient::doRemoveSessions() return has_data; } +bool HttpClient::doRetrySessions() +{ + auto has_data = false; + + for (auto it = pending_to_retry_sessions_.crbegin(); it != pending_to_retry_sessions_.crend();) + { + const auto session = *it; + const auto operation = (nullptr != session) ? session->GetOperation().get() : nullptr; + + if (operation) + { + const auto now = std::chrono::system_clock::now(); + const auto next_retry_time = operation->NextRetryTime(); + + if (next_retry_time > (now - scheduled_delay_milliseconds_) && + next_retry_time < (now + scheduled_delay_milliseconds_)) + { + auto easy_handle = operation->GetCurlEasyHandle(); + curl_multi_remove_handle(multi_handle_, easy_handle); + curl_multi_add_handle(multi_handle_, easy_handle); + has_data = true; + it = decltype(it)(pending_to_retry_sessions_.erase(std::next(it).base())); + } + else + { + ++it; + } + } + } + + return has_data; +} + void HttpClient::resetMultiHandle() { std::list> sessions; diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index c27cff1b41..7e593b8673 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -263,7 +264,8 @@ HttpOperation::HttpOperation(opentelemetry::ext::http::client::Method method, bool is_raw_response, std::chrono::milliseconds http_conn_timeout, bool reuse_connection, - bool is_log_enabled) + bool is_log_enabled, + const RetryPolicy &retry_policy) : is_aborted_(false), is_finished_(false), is_cleaned_(false), @@ -284,6 +286,7 @@ HttpOperation::HttpOperation(opentelemetry::ext::http::client::Method method, session_state_(opentelemetry::ext::http::client::SessionState::Created), compression_(compression), is_log_enabled_(is_log_enabled), + retry_policy_(retry_policy), response_code_(0) { /* get a curl handle */ @@ -426,6 +429,29 @@ void HttpOperation::Cleanup() } } +bool HttpOperation::IsRetryable() +{ + static constexpr auto kRetryableStatusCodes = std::array{ + 429, // Too Many Requests + 502, // Bad Gateway + 503, // Service Unavailable + 504 // Gateway Timeout + }; + + const auto is_retryable = std::find(kRetryableStatusCodes.cbegin(), kRetryableStatusCodes.cend(), + response_code_) != kRetryableStatusCodes.cend(); + + return is_retryable && (retry_policy_.max_attempts > 0) && + (retry_policy_.backoff_multiplier > 0.f) && + (retry_policy_.initial_backoff > SecondsDecimal::zero()) && + (retry_policy_.max_backoff > SecondsDecimal::zero()); +} + +std::chrono::system_clock::time_point HttpOperation::NextRetryTime() +{ + return last_attempt_time_ + std::chrono::seconds(1); +} + /* Support for TLS min version, TLS max version. @@ -1198,11 +1224,6 @@ CURLcode HttpOperation::Send() CURLcode code = curl_easy_perform(curl_resource_.easy_handle); PerformCurlMessage(code); - if (CURLE_OK != code) - { - return code; - } - return code; } @@ -1248,7 +1269,7 @@ CURLcode HttpOperation::SendAsync(Session *session, std::functioncallback = std::move(callback); session->GetHttpClient().ScheduleAddSession(session->GetSessionId()); - return code; + return CURLE_OK; } Headers HttpOperation::GetResponseHeaders() @@ -1306,7 +1327,10 @@ void HttpOperation::Abort() void HttpOperation::PerformCurlMessage(CURLcode code) { - last_curl_result_ = code; + retry_policy_.max_attempts--; + last_attempt_time_ = std::chrono::system_clock::now(); + last_curl_result_ = code; + if (code != CURLE_OK) { switch (GetSessionState()) @@ -1352,8 +1376,17 @@ void HttpOperation::PerformCurlMessage(CURLcode code) DispatchEvent(opentelemetry::ext::http::client::SessionState::Response); } - // Cleanup and unbind easy handle from multi handle, and finish callback - Cleanup(); + if (IsRetryable()) + { + // Clear any request offset and response data received in previous attempt + ReleaseResponse(); + request_nwrite_ = 0; + } + else + { + // Cleanup and unbind easy handle from multi handle, and finish callback + Cleanup(); + } } } // namespace curl diff --git a/test_common/include/opentelemetry/test_common/ext/http/client/nosend/http_client_nosend.h b/test_common/include/opentelemetry/test_common/ext/http/client/nosend/http_client_nosend.h index 211b5b3720..6a30e4fd26 100644 --- a/test_common/include/opentelemetry/test_common/ext/http/client/nosend/http_client_nosend.h +++ b/test_common/include/opentelemetry/test_common/ext/http/client/nosend/http_client_nosend.h @@ -71,6 +71,12 @@ class Request : public opentelemetry::ext::http::client::Request void EnableLogging(bool is_log_enabled) noexcept override { is_log_enabled_ = is_log_enabled; } + void SetRetryPolicy( + const opentelemetry::ext::http::client::RetryPolicy &retry_policy) noexcept override + { + retry_policy_ = retry_policy; + } + public: opentelemetry::ext::http::client::Method method_; opentelemetry::ext::http::client::HttpSslOptions ssl_options_; @@ -81,6 +87,7 @@ class Request : public opentelemetry::ext::http::client::Request opentelemetry::ext::http::client::Compression compression_{ opentelemetry::ext::http::client::Compression::kNone}; bool is_log_enabled_{false}; + opentelemetry::ext::http::client::RetryPolicy retry_policy_; }; class Response : public opentelemetry::ext::http::client::Response From 3c2c24a37cc88c49be9431cd68b1087977e5012b Mon Sep 17 00:00:00 2001 From: Alex E Date: Sun, 29 Dec 2024 11:46:10 -0500 Subject: [PATCH 05/30] Model exponential backoff after grpc retry policy --- .../http/client/curl/http_operation_curl.h | 4 ++- ext/src/http/client/curl/http_client_curl.cc | 6 +---- .../http/client/curl/http_operation_curl.cc | 27 ++++++++++++++----- 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h index b6ad5a66a3..e2ae64bf18 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h @@ -13,6 +13,7 @@ # include #endif +#include #include #include #include @@ -323,7 +324,8 @@ class HttpOperation const bool is_log_enabled_; - RetryPolicy retry_policy_; + const RetryPolicy retry_policy_; + std::uint32_t retry_attempts_; std::chrono::system_clock::time_point last_attempt_time_; // Processed response headers and body diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index e96da867d4..130b6fe320 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -799,11 +799,7 @@ bool HttpClient::doRetrySessions() if (operation) { - const auto now = std::chrono::system_clock::now(); - const auto next_retry_time = operation->NextRetryTime(); - - if (next_retry_time > (now - scheduled_delay_milliseconds_) && - next_retry_time < (now + scheduled_delay_milliseconds_)) + if (operation->NextRetryTime() < std::chrono::system_clock::now()) { auto easy_handle = operation->GetCurlEasyHandle(); curl_multi_remove_handle(multi_handle_, easy_handle); diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 7e593b8673..bff54a0b97 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -15,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -287,6 +289,7 @@ HttpOperation::HttpOperation(opentelemetry::ext::http::client::Method method, compression_(compression), is_log_enabled_(is_log_enabled), retry_policy_(retry_policy), + retry_attempts_(0), response_code_(0) { /* get a curl handle */ @@ -441,15 +444,27 @@ bool HttpOperation::IsRetryable() const auto is_retryable = std::find(kRetryableStatusCodes.cbegin(), kRetryableStatusCodes.cend(), response_code_) != kRetryableStatusCodes.cend(); - return is_retryable && (retry_policy_.max_attempts > 0) && - (retry_policy_.backoff_multiplier > 0.f) && - (retry_policy_.initial_backoff > SecondsDecimal::zero()) && - (retry_policy_.max_backoff > SecondsDecimal::zero()); + return is_retryable && retry_attempts_ < retry_policy_.max_attempts; } std::chrono::system_clock::time_point HttpOperation::NextRetryTime() { - return last_attempt_time_ + std::chrono::seconds(1); + static std::random_device rd; + static std::mt19937 gen(rd()); + static std::uniform_real_distribution dis(0.8, 1.2); + + SecondsDecimal backoff = retry_policy_.initial_backoff * dis(gen); + + if (retry_attempts_ > 1) + { + backoff = std::min(retry_policy_.initial_backoff * + std::pow(retry_policy_.backoff_multiplier, + static_cast(retry_attempts_ - 1)), + retry_policy_.max_backoff) * + dis(gen); + } + + return last_attempt_time_ + std::chrono::duration_cast(backoff); } /* @@ -1327,7 +1342,7 @@ void HttpOperation::Abort() void HttpOperation::PerformCurlMessage(CURLcode code) { - retry_policy_.max_attempts--; + ++retry_attempts_; last_attempt_time_ = std::chrono::system_clock::now(); last_curl_result_ = code; From 2f9b6299d914dcb395ae4c78869fd26bf4853750 Mon Sep 17 00:00:00 2001 From: Alex E Date: Sun, 29 Dec 2024 20:30:37 -0500 Subject: [PATCH 06/30] Fix some neglected bits --- .../exporters/otlp/otlp_http_client.h | 6 ++--- exporters/otlp/src/otlp_http_exporter.cc | 21 ++++++++++------ .../otlp/src/otlp_http_log_record_exporter.cc | 25 +++++++++++++------ .../otlp/src/otlp_http_metric_exporter.cc | 11 +++++++- .../otlp/test/otlp_http_exporter_test.cc | 2 +- .../otlp_http_log_record_exporter_test.cc | 3 ++- .../test/otlp_http_metric_exporter_test.cc | 3 ++- .../http/client/curl/http_operation_curl.h | 2 +- ext/src/http/client/curl/http_client_curl.cc | 15 +++++++---- 9 files changed, 59 insertions(+), 29 deletions(-) diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h index 9875d586fe..4f4749f48e 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h @@ -74,15 +74,15 @@ struct OtlpHttpClientOptions // Additional HTTP headers OtlpHeaders http_headers; + // Retry policy for select failure codes + ext::http::client::RetryPolicy retry_policy; + // Concurrent requests std::size_t max_concurrent_requests = 64; // Requests per connections std::size_t max_requests_per_connection = 8; - // Retry policy for select failure codes - ext::http::client::RetryPolicy retry_policy; - // User agent std::string user_agent; diff --git a/exporters/otlp/src/otlp_http_exporter.cc b/exporters/otlp/src/otlp_http_exporter.cc index c0efcc47a8..2b48f36dcd 100644 --- a/exporters/otlp/src/otlp_http_exporter.cc +++ b/exporters/otlp/src/otlp_http_exporter.cc @@ -73,14 +73,19 @@ OtlpHttpExporter::OtlpHttpExporter(const OtlpHttpExporterOptions &options) OtlpHttpExporter::OtlpHttpExporter(std::unique_ptr http_client) : options_(OtlpHttpExporterOptions()), http_client_(std::move(http_client)) { - OtlpHttpExporterOptions &options = const_cast(options_); - options.url = http_client_->GetOptions().url; - options.content_type = http_client_->GetOptions().content_type; - options.json_bytes_mapping = http_client_->GetOptions().json_bytes_mapping; - options.use_json_name = http_client_->GetOptions().use_json_name; - options.console_debug = http_client_->GetOptions().console_debug; - options.timeout = http_client_->GetOptions().timeout; - options.http_headers = http_client_->GetOptions().http_headers; + OtlpHttpExporterOptions &options = const_cast(options_); + options.url = http_client_->GetOptions().url; + options.content_type = http_client_->GetOptions().content_type; + options.json_bytes_mapping = http_client_->GetOptions().json_bytes_mapping; + options.use_json_name = http_client_->GetOptions().use_json_name; + options.console_debug = http_client_->GetOptions().console_debug; + options.timeout = http_client_->GetOptions().timeout; + options.http_headers = http_client_->GetOptions().http_headers; + options.retry_policy_max_attempts = http_client_->GetOptions().retry_policy.max_attempts; + options.retry_policy_initial_backoff = http_client_->GetOptions().retry_policy.initial_backoff; + options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff; + options.retry_policy_backoff_multiplier = + http_client_->GetOptions().retry_policy.backoff_multiplier; #ifdef ENABLE_ASYNC_EXPORT options.max_concurrent_requests = http_client_->GetOptions().max_concurrent_requests; options.max_requests_per_connection = http_client_->GetOptions().max_requests_per_connection; diff --git a/exporters/otlp/src/otlp_http_log_record_exporter.cc b/exporters/otlp/src/otlp_http_log_record_exporter.cc index bd76bd10bd..b48006f137 100644 --- a/exporters/otlp/src/otlp_http_log_record_exporter.cc +++ b/exporters/otlp/src/otlp_http_log_record_exporter.cc @@ -60,7 +60,11 @@ OtlpHttpLogRecordExporter::OtlpHttpLogRecordExporter( options.use_json_name, options.console_debug, options.timeout, - options.http_headers + options.http_headers, + options.retry_policy_max_attempts, + options.retry_policy_initial_backoff, + options.retry_policy_max_backoff, + options.retry_policy_backoff_multiplier #ifdef ENABLE_ASYNC_EXPORT , options.max_concurrent_requests, @@ -74,13 +78,18 @@ OtlpHttpLogRecordExporter::OtlpHttpLogRecordExporter(std::unique_ptr(options_); - options.url = http_client_->GetOptions().url; - options.content_type = http_client_->GetOptions().content_type; - options.json_bytes_mapping = http_client_->GetOptions().json_bytes_mapping; - options.use_json_name = http_client_->GetOptions().use_json_name; - options.console_debug = http_client_->GetOptions().console_debug; - options.timeout = http_client_->GetOptions().timeout; - options.http_headers = http_client_->GetOptions().http_headers; + options.url = http_client_->GetOptions().url; + options.content_type = http_client_->GetOptions().content_type; + options.json_bytes_mapping = http_client_->GetOptions().json_bytes_mapping; + options.use_json_name = http_client_->GetOptions().use_json_name; + options.console_debug = http_client_->GetOptions().console_debug; + options.timeout = http_client_->GetOptions().timeout; + options.http_headers = http_client_->GetOptions().http_headers; + options.retry_policy_max_attempts = http_client_->GetOptions().retry_policy.max_attempts; + options.retry_policy_initial_backoff = http_client_->GetOptions().retry_policy.initial_backoff; + options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff; + options.retry_policy_backoff_multiplier = + http_client_->GetOptions().retry_policy.backoff_multiplier; #ifdef ENABLE_ASYNC_EXPORT options.max_concurrent_requests = http_client_->GetOptions().max_concurrent_requests; options.max_requests_per_connection = http_client_->GetOptions().max_requests_per_connection; diff --git a/exporters/otlp/src/otlp_http_metric_exporter.cc b/exporters/otlp/src/otlp_http_metric_exporter.cc index 18b4ed0712..875155e424 100644 --- a/exporters/otlp/src/otlp_http_metric_exporter.cc +++ b/exporters/otlp/src/otlp_http_metric_exporter.cc @@ -61,7 +61,11 @@ OtlpHttpMetricExporter::OtlpHttpMetricExporter(const OtlpHttpMetricExporterOptio options.use_json_name, options.console_debug, options.timeout, - options.http_headers + options.http_headers, + options.retry_policy_max_attempts, + options.retry_policy_initial_backoff, + options.retry_policy_max_backoff, + options.retry_policy_backoff_multiplier #ifdef ENABLE_ASYNC_EXPORT , options.max_concurrent_requests, @@ -84,6 +88,11 @@ OtlpHttpMetricExporter::OtlpHttpMetricExporter(std::unique_ptr h options.console_debug = http_client_->GetOptions().console_debug; options.timeout = http_client_->GetOptions().timeout; options.http_headers = http_client_->GetOptions().http_headers; + options.retry_policy_max_attempts = http_client_->GetOptions().retry_policy.max_attempts; + options.retry_policy_initial_backoff = http_client_->GetOptions().retry_policy.initial_backoff; + options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff; + options.retry_policy_backoff_multiplier = + http_client_->GetOptions().retry_policy.backoff_multiplier; #ifdef ENABLE_ASYNC_EXPORT options.max_concurrent_requests = http_client_->GetOptions().max_concurrent_requests; options.max_requests_per_connection = http_client_->GetOptions().max_requests_per_connection; diff --git a/exporters/otlp/test/otlp_http_exporter_test.cc b/exporters/otlp/test/otlp_http_exporter_test.cc index 2e4c168e80..cb7dbbbe06 100644 --- a/exporters/otlp/test/otlp_http_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_exporter_test.cc @@ -741,7 +741,7 @@ TEST_P(OtlpHttpExporterRetryIntegrationTests, StatusCodes) size_t request_count = 0UL; HTTP_SERVER_NS::HttpRequestCallback request_handler{ - [&request_count, &status_codes](HTTP_SERVER_NS::HttpRequest const &request, + [&request_count, &status_codes](HTTP_SERVER_NS::HttpRequest const & /* request */, HTTP_SERVER_NS::HttpResponse &response) { response.body = "TEST!"; response.code = status_codes.at(request_count++ % status_codes.size()); diff --git a/exporters/otlp/test/otlp_http_log_record_exporter_test.cc b/exporters/otlp/test/otlp_http_log_record_exporter_test.cc index f12899e677..b1071b6f04 100644 --- a/exporters/otlp/test/otlp_http_log_record_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_log_record_exporter_test.cc @@ -69,7 +69,8 @@ OtlpHttpClientOptions MakeOtlpHttpClientOptions(HttpRequestContentType content_t "", /* ssl_cipher */ "", /* ssl_cipher_suite */ options.content_type, options.json_bytes_mapping, options.compression, options.use_json_name, - options.console_debug, options.timeout, options.http_headers); + options.console_debug, options.timeout, options.http_headers, 0, SecondsDecimal::zero(), + SecondsDecimal::zero(), 0); if (!async_mode) { otlp_http_client_options.max_concurrent_requests = 0; diff --git a/exporters/otlp/test/otlp_http_metric_exporter_test.cc b/exporters/otlp/test/otlp_http_metric_exporter_test.cc index 222c22c983..ac269b733a 100644 --- a/exporters/otlp/test/otlp_http_metric_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_metric_exporter_test.cc @@ -91,7 +91,8 @@ OtlpHttpClientOptions MakeOtlpHttpClientOptions(HttpRequestContentType content_t "", /* ssl_cipher */ "", /* ssl_cipher_suite */ options.content_type, options.json_bytes_mapping, options.compression, options.use_json_name, - options.console_debug, options.timeout, options.http_headers); + options.console_debug, options.timeout, options.http_headers, 0, SecondsDecimal::zero(), + SecondsDecimal::zero(), 0); if (!async_mode) { otlp_http_client_options.max_concurrent_requests = 0; diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h index e2ae64bf18..529047d56b 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h @@ -325,7 +325,7 @@ class HttpOperation const bool is_log_enabled_; const RetryPolicy retry_policy_; - std::uint32_t retry_attempts_; + decltype(RetryPolicy::max_attempts) retry_attempts_; std::chrono::system_clock::time_point last_attempt_time_; // Processed response headers and body diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 130b6fe320..c1df09806d 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -792,9 +792,14 @@ bool HttpClient::doRetrySessions() { auto has_data = false; - for (auto it = pending_to_retry_sessions_.crbegin(); it != pending_to_retry_sessions_.crend();) - { - const auto session = *it; + // Assumptions: + // - This is a FIFO list so older sessions, pushed at the front, always end up at the tail + // - Retry policy is not changed once HTTP client is initialized, so same settings apply to all + // - Iterating backwards should result in removing those items that are due for a retry attempt + for (auto retry_it = pending_to_retry_sessions_.crbegin(); + retry_it != pending_to_retry_sessions_.crend();) + { + const auto session = *retry_it; const auto operation = (nullptr != session) ? session->GetOperation().get() : nullptr; if (operation) @@ -805,11 +810,11 @@ bool HttpClient::doRetrySessions() curl_multi_remove_handle(multi_handle_, easy_handle); curl_multi_add_handle(multi_handle_, easy_handle); has_data = true; - it = decltype(it)(pending_to_retry_sessions_.erase(std::next(it).base())); + retry_it = decltype(retry_it){pending_to_retry_sessions_.erase(std::next(retry_it).base())}; } else { - ++it; + ++retry_it; } } } From 9cd471709108c8ece6ebf8ae7cbf073e61b029a8 Mon Sep 17 00:00:00 2001 From: Alex E Date: Sun, 29 Dec 2024 21:23:58 -0500 Subject: [PATCH 07/30] Reduce usage of decimal seconds to http client et al --- .../exporters/otlp/otlp_environment.h | 14 +++++----- .../exporters/otlp/otlp_http_client.h | 4 +-- .../otlp/otlp_http_exporter_options.h | 4 +-- .../otlp_http_log_record_exporter_options.h | 4 +-- .../otlp/otlp_http_metric_exporter_options.h | 4 +-- exporters/otlp/src/otlp_environment.cc | 24 ++++++++--------- exporters/otlp/src/otlp_http_exporter.cc | 23 ++++++++-------- .../otlp/src/otlp_http_log_record_exporter.cc | 21 ++++++++------- .../otlp/src/otlp_http_metric_exporter.cc | 5 ++-- .../otlp/test/otlp_http_exporter_test.cc | 27 +++++++++---------- .../otlp_http_log_record_exporter_test.cc | 15 +++++------ .../test/otlp_http_metric_exporter_test.cc | 15 +++++------ .../ext/http/client/http_client.h | 4 +-- .../http/client/curl/http_operation_curl.cc | 6 ++--- 14 files changed, 84 insertions(+), 86 deletions(-) diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_environment.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_environment.h index 799e07f27d..5eb1c58037 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_environment.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_environment.h @@ -18,8 +18,6 @@ namespace exporter namespace otlp { -using SecondsDecimal = std::chrono::duration>; - inline std::string GetOtlpDefaultUserAgent() { return "OTel-OTLP-Exporter-Cpp/" OPENTELEMETRY_SDK_VERSION; @@ -158,13 +156,13 @@ std::uint32_t GetOtlpDefaultTracesRetryMaxAttempts(); std::uint32_t GetOtlpDefaultMetricsRetryMaxAttempts(); std::uint32_t GetOtlpDefaultLogsRetryMaxAttempts(); -SecondsDecimal GetOtlpDefaultTracesRetryInitialBackoff(); -SecondsDecimal GetOtlpDefaultMetricsRetryInitialBackoff(); -SecondsDecimal GetOtlpDefaultLogsRetryInitialBackoff(); +float GetOtlpDefaultTracesRetryInitialBackoff(); +float GetOtlpDefaultMetricsRetryInitialBackoff(); +float GetOtlpDefaultLogsRetryInitialBackoff(); -SecondsDecimal GetOtlpDefaultTracesRetryMaxBackoff(); -SecondsDecimal GetOtlpDefaultMetricsRetryMaxBackoff(); -SecondsDecimal GetOtlpDefaultLogsRetryMaxBackoff(); +float GetOtlpDefaultTracesRetryMaxBackoff(); +float GetOtlpDefaultMetricsRetryMaxBackoff(); +float GetOtlpDefaultLogsRetryMaxBackoff(); float GetOtlpDefaultTracesRetryBackoffMultiplier(); float GetOtlpDefaultMetricsRetryBackoffMultiplier(); diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h index 4f4749f48e..7621169b5a 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h @@ -106,8 +106,8 @@ struct OtlpHttpClientOptions std::chrono::system_clock::duration input_timeout, const OtlpHeaders &input_http_headers, std::uint32_t input_retry_policy_max_attempts, - SecondsDecimal input_retry_policy_initial_backoff, - SecondsDecimal input_retry_policy_max_backoff, + float input_retry_policy_initial_backoff, + float input_retry_policy_max_backoff, float input_retry_policy_backoff_multiplier, std::size_t input_concurrent_sessions = 64, std::size_t input_max_requests_per_connection = 8, 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 index 1488484181..7f0785e4e8 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter_options.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter_options.h @@ -107,10 +107,10 @@ struct OPENTELEMETRY_EXPORT OtlpHttpExporterOptions std::uint32_t retry_policy_max_attempts{}; /** The initial backoff delay between retry attempts, random between (0, initial_backoff). */ - SecondsDecimal retry_policy_initial_backoff{}; + float retry_policy_initial_backoff{}; /** The maximum backoff places an upper limit on exponential backoff growth. */ - SecondsDecimal retry_policy_max_backoff{}; + float retry_policy_max_backoff{}; /** The backoff will be multiplied by this value after each retry attempt. */ float retry_policy_backoff_multiplier{}; diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_log_record_exporter_options.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_log_record_exporter_options.h index 43157dd7ca..ca26a74d27 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_log_record_exporter_options.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_log_record_exporter_options.h @@ -107,10 +107,10 @@ struct OPENTELEMETRY_EXPORT OtlpHttpLogRecordExporterOptions std::uint32_t retry_policy_max_attempts{}; /** The initial backoff delay between retry attempts, random between (0, initial_backoff). */ - SecondsDecimal retry_policy_initial_backoff{}; + float retry_policy_initial_backoff{}; /** The maximum backoff places an upper limit on exponential backoff growth. */ - SecondsDecimal retry_policy_max_backoff{}; + float retry_policy_max_backoff{}; /** The backoff will be multiplied by this value after each retry attempt. */ float retry_policy_backoff_multiplier{}; diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_metric_exporter_options.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_metric_exporter_options.h index 7ec57a36b6..127acbfd68 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_metric_exporter_options.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_metric_exporter_options.h @@ -110,10 +110,10 @@ struct OPENTELEMETRY_EXPORT OtlpHttpMetricExporterOptions std::uint32_t retry_policy_max_attempts{}; /** The initial backoff delay between retry attempts, random between (0, initial_backoff). */ - SecondsDecimal retry_policy_initial_backoff{}; + float retry_policy_initial_backoff{}; /** The maximum backoff places an upper limit on exponential backoff growth. */ - SecondsDecimal retry_policy_max_backoff{}; + float retry_policy_max_backoff{}; /** The backoff will be multiplied by this value after each retry attempt. */ float retry_policy_backoff_multiplier{}; diff --git a/exporters/otlp/src/otlp_environment.cc b/exporters/otlp/src/otlp_environment.cc index a9e43acb08..3fcc34c6ff 100644 --- a/exporters/otlp/src/otlp_environment.cc +++ b/exporters/otlp/src/otlp_environment.cc @@ -1174,46 +1174,46 @@ std::uint32_t GetOtlpDefaultLogsRetryMaxAttempts() return GetUintEnvVarOrDefault(kSignalEnv, kGenericEnv, 5U); } -SecondsDecimal GetOtlpDefaultTracesRetryInitialBackoff() +float GetOtlpDefaultTracesRetryInitialBackoff() { constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_TRACES_RETRY_INITIAL_BACKOFF"; constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"; - return SecondsDecimal{GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.0)}; + return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.0); } -SecondsDecimal GetOtlpDefaultMetricsRetryInitialBackoff() +float GetOtlpDefaultMetricsRetryInitialBackoff() { constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_METRICS_RETRY_INITIAL_BACKOFF"; constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"; - return SecondsDecimal{GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.0)}; + return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.0); } -SecondsDecimal GetOtlpDefaultLogsRetryInitialBackoff() +float GetOtlpDefaultLogsRetryInitialBackoff() { constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_LOGS_RETRY_INITIAL_BACKOFF"; constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"; - return SecondsDecimal{GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.0)}; + return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 1.0); } -SecondsDecimal GetOtlpDefaultTracesRetryMaxBackoff() +float GetOtlpDefaultTracesRetryMaxBackoff() { constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_BACKOFF"; constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"; - return SecondsDecimal{GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 5.0)}; + return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 5.0); } -SecondsDecimal GetOtlpDefaultMetricsRetryMaxBackoff() +float GetOtlpDefaultMetricsRetryMaxBackoff() { constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_BACKOFF"; constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"; - return SecondsDecimal{GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 5.0)}; + return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 5.0); } -SecondsDecimal GetOtlpDefaultLogsRetryMaxBackoff() +float GetOtlpDefaultLogsRetryMaxBackoff() { constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_BACKOFF"; constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"; - return SecondsDecimal{GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 5.0)}; + return GetFloatEnvVarOrDefault(kSignalEnv, kGenericEnv, 5.0); } float GetOtlpDefaultTracesRetryBackoffMultiplier() diff --git a/exporters/otlp/src/otlp_http_exporter.cc b/exporters/otlp/src/otlp_http_exporter.cc index 2b48f36dcd..868e7730d1 100644 --- a/exporters/otlp/src/otlp_http_exporter.cc +++ b/exporters/otlp/src/otlp_http_exporter.cc @@ -73,17 +73,18 @@ OtlpHttpExporter::OtlpHttpExporter(const OtlpHttpExporterOptions &options) OtlpHttpExporter::OtlpHttpExporter(std::unique_ptr http_client) : options_(OtlpHttpExporterOptions()), http_client_(std::move(http_client)) { - OtlpHttpExporterOptions &options = const_cast(options_); - options.url = http_client_->GetOptions().url; - options.content_type = http_client_->GetOptions().content_type; - options.json_bytes_mapping = http_client_->GetOptions().json_bytes_mapping; - options.use_json_name = http_client_->GetOptions().use_json_name; - options.console_debug = http_client_->GetOptions().console_debug; - options.timeout = http_client_->GetOptions().timeout; - options.http_headers = http_client_->GetOptions().http_headers; - options.retry_policy_max_attempts = http_client_->GetOptions().retry_policy.max_attempts; - options.retry_policy_initial_backoff = http_client_->GetOptions().retry_policy.initial_backoff; - options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff; + OtlpHttpExporterOptions &options = const_cast(options_); + options.url = http_client_->GetOptions().url; + options.content_type = http_client_->GetOptions().content_type; + options.json_bytes_mapping = http_client_->GetOptions().json_bytes_mapping; + options.use_json_name = http_client_->GetOptions().use_json_name; + options.console_debug = http_client_->GetOptions().console_debug; + options.timeout = http_client_->GetOptions().timeout; + options.http_headers = http_client_->GetOptions().http_headers; + options.retry_policy_max_attempts = http_client_->GetOptions().retry_policy.max_attempts; + options.retry_policy_initial_backoff = + http_client_->GetOptions().retry_policy.initial_backoff.count(); + options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff.count(); options.retry_policy_backoff_multiplier = http_client_->GetOptions().retry_policy.backoff_multiplier; #ifdef ENABLE_ASYNC_EXPORT diff --git a/exporters/otlp/src/otlp_http_log_record_exporter.cc b/exporters/otlp/src/otlp_http_log_record_exporter.cc index b48006f137..6ebcf575f9 100644 --- a/exporters/otlp/src/otlp_http_log_record_exporter.cc +++ b/exporters/otlp/src/otlp_http_log_record_exporter.cc @@ -78,16 +78,17 @@ OtlpHttpLogRecordExporter::OtlpHttpLogRecordExporter(std::unique_ptr(options_); - options.url = http_client_->GetOptions().url; - options.content_type = http_client_->GetOptions().content_type; - options.json_bytes_mapping = http_client_->GetOptions().json_bytes_mapping; - options.use_json_name = http_client_->GetOptions().use_json_name; - options.console_debug = http_client_->GetOptions().console_debug; - options.timeout = http_client_->GetOptions().timeout; - options.http_headers = http_client_->GetOptions().http_headers; - options.retry_policy_max_attempts = http_client_->GetOptions().retry_policy.max_attempts; - options.retry_policy_initial_backoff = http_client_->GetOptions().retry_policy.initial_backoff; - options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff; + options.url = http_client_->GetOptions().url; + options.content_type = http_client_->GetOptions().content_type; + options.json_bytes_mapping = http_client_->GetOptions().json_bytes_mapping; + options.use_json_name = http_client_->GetOptions().use_json_name; + options.console_debug = http_client_->GetOptions().console_debug; + options.timeout = http_client_->GetOptions().timeout; + options.http_headers = http_client_->GetOptions().http_headers; + options.retry_policy_max_attempts = http_client_->GetOptions().retry_policy.max_attempts; + options.retry_policy_initial_backoff = + http_client_->GetOptions().retry_policy.initial_backoff.count(); + options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff.count(); options.retry_policy_backoff_multiplier = http_client_->GetOptions().retry_policy.backoff_multiplier; #ifdef ENABLE_ASYNC_EXPORT diff --git a/exporters/otlp/src/otlp_http_metric_exporter.cc b/exporters/otlp/src/otlp_http_metric_exporter.cc index 875155e424..3ac4706497 100644 --- a/exporters/otlp/src/otlp_http_metric_exporter.cc +++ b/exporters/otlp/src/otlp_http_metric_exporter.cc @@ -89,8 +89,9 @@ OtlpHttpMetricExporter::OtlpHttpMetricExporter(std::unique_ptr h options.timeout = http_client_->GetOptions().timeout; options.http_headers = http_client_->GetOptions().http_headers; options.retry_policy_max_attempts = http_client_->GetOptions().retry_policy.max_attempts; - options.retry_policy_initial_backoff = http_client_->GetOptions().retry_policy.initial_backoff; - options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff; + options.retry_policy_initial_backoff = + http_client_->GetOptions().retry_policy.initial_backoff.count(); + options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff.count(); options.retry_policy_backoff_multiplier = http_client_->GetOptions().retry_policy.backoff_multiplier; #ifdef ENABLE_ASYNC_EXPORT diff --git a/exporters/otlp/test/otlp_http_exporter_test.cc b/exporters/otlp/test/otlp_http_exporter_test.cc index cb7dbbbe06..3f31a2476f 100644 --- a/exporters/otlp/test/otlp_http_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_exporter_test.cc @@ -75,8 +75,7 @@ OtlpHttpClientOptions MakeOtlpHttpClientOptions(HttpRequestContentType content_t "", /* ssl_cipher */ "", /* ssl_cipher_suite */ options.content_type, options.json_bytes_mapping, options.compression, options.use_json_name, - options.console_debug, options.timeout, options.http_headers, 0, SecondsDecimal::zero(), - SecondsDecimal::zero(), 0); + options.console_debug, options.timeout, options.http_headers, 0, 0.0, 0.0, 0); if (!async_mode) { otlp_http_client_options.max_concurrent_requests = 0; @@ -632,8 +631,8 @@ TEST_F(OtlpHttpExporterTestPeer, ConfigRetryDefaultValues) std::unique_ptr exporter(new OtlpHttpExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 5); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 1); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 5); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 1.0); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 5.0); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 1.5); } @@ -647,8 +646,8 @@ TEST_F(OtlpHttpExporterTestPeer, ConfigRetryValuesFromEnv) std::unique_ptr exporter(new OtlpHttpExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 123); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 4.5); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 6.7); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 4.5); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 6.7); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 8.9); unsetenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_ATTEMPTS"); @@ -667,8 +666,8 @@ TEST_F(OtlpHttpExporterTestPeer, ConfigRetryGenericValuesFromEnv) std::unique_ptr exporter(new OtlpHttpExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 321); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 5.4); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 7.6); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 5.4); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 7.6); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 9.8); unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"); @@ -759,16 +758,16 @@ TEST_P(OtlpHttpExporterRetryIntegrationTests, StatusCodes) if (is_retry_enabled) { opts.retry_policy_max_attempts = 5; - opts.retry_policy_initial_backoff = SecondsDecimal{0.1}; - opts.retry_policy_max_backoff = SecondsDecimal{5}; - opts.retry_policy_backoff_multiplier = 1; + opts.retry_policy_initial_backoff = 0.1f; + opts.retry_policy_max_backoff = 5.0f; + opts.retry_policy_backoff_multiplier = 1.0f; } else { opts.retry_policy_max_attempts = 0; - opts.retry_policy_initial_backoff = SecondsDecimal{0}; - opts.retry_policy_max_backoff = SecondsDecimal{0}; - opts.retry_policy_backoff_multiplier = 0; + opts.retry_policy_initial_backoff = 0.0f; + opts.retry_policy_max_backoff = 0.0f; + opts.retry_policy_backoff_multiplier = 0.0f; } auto exporter = otlp::OtlpHttpExporterFactory::Create(opts); diff --git a/exporters/otlp/test/otlp_http_log_record_exporter_test.cc b/exporters/otlp/test/otlp_http_log_record_exporter_test.cc index b1071b6f04..4c5405ecb6 100644 --- a/exporters/otlp/test/otlp_http_log_record_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_log_record_exporter_test.cc @@ -69,8 +69,7 @@ OtlpHttpClientOptions MakeOtlpHttpClientOptions(HttpRequestContentType content_t "", /* ssl_cipher */ "", /* ssl_cipher_suite */ options.content_type, options.json_bytes_mapping, options.compression, options.use_json_name, - options.console_debug, options.timeout, options.http_headers, 0, SecondsDecimal::zero(), - SecondsDecimal::zero(), 0); + options.console_debug, options.timeout, options.http_headers, 0, 0.0, 0.0, 0); if (!async_mode) { otlp_http_client_options.max_concurrent_requests = 0; @@ -754,8 +753,8 @@ TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigRetryDefaultValues) std::unique_ptr exporter(new OtlpHttpLogRecordExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 5); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 1); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 5); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 1.0); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 5.0); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 1.5); } @@ -769,8 +768,8 @@ TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigRetryValuesFromEnv) std::unique_ptr exporter(new OtlpHttpLogRecordExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 123); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 4.5); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 6.7); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 4.5); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 6.7); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 8.9); unsetenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_ATTEMPTS"); @@ -789,8 +788,8 @@ TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigRetryGenericValuesFromEnv) std::unique_ptr exporter(new OtlpHttpLogRecordExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 321); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 5.4); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 7.6); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 5.4); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 7.6); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 9.8); unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"); diff --git a/exporters/otlp/test/otlp_http_metric_exporter_test.cc b/exporters/otlp/test/otlp_http_metric_exporter_test.cc index ac269b733a..b7420008d9 100644 --- a/exporters/otlp/test/otlp_http_metric_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_metric_exporter_test.cc @@ -91,8 +91,7 @@ OtlpHttpClientOptions MakeOtlpHttpClientOptions(HttpRequestContentType content_t "", /* ssl_cipher */ "", /* ssl_cipher_suite */ options.content_type, options.json_bytes_mapping, options.compression, options.use_json_name, - options.console_debug, options.timeout, options.http_headers, 0, SecondsDecimal::zero(), - SecondsDecimal::zero(), 0); + options.console_debug, options.timeout, options.http_headers, 0, 0.0, 0.0, 0); if (!async_mode) { otlp_http_client_options.max_concurrent_requests = 0; @@ -1010,8 +1009,8 @@ TEST_F(OtlpHttpMetricExporterTestPeer, ConfigRetryDefaultValues) std::unique_ptr exporter(new OtlpHttpMetricExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 5); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 1); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 5); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 1.0); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 5.0); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 1.5); } @@ -1025,8 +1024,8 @@ TEST_F(OtlpHttpMetricExporterTestPeer, ConfigRetryValuesFromEnv) std::unique_ptr exporter(new OtlpHttpMetricExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 123); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 4.5); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 6.7); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 4.5); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 6.7); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 8.9); unsetenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_ATTEMPTS"); @@ -1045,8 +1044,8 @@ TEST_F(OtlpHttpMetricExporterTestPeer, ConfigRetryGenericValuesFromEnv) std::unique_ptr exporter(new OtlpHttpMetricExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 321); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 5.4); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 7.6); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 5.4); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 7.6); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 9.8); unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"); diff --git a/ext/include/opentelemetry/ext/http/client/http_client.h b/ext/include/opentelemetry/ext/http/client/http_client.h index 36f4e2d0a6..8dbdc682ee 100644 --- a/ext/include/opentelemetry/ext/http/client/http_client.h +++ b/ext/include/opentelemetry/ext/http/client/http_client.h @@ -233,8 +233,8 @@ struct RetryPolicy RetryPolicy() = default; RetryPolicy(std::uint32_t input_max_attempts, - SecondsDecimal input_initial_backoff, - SecondsDecimal input_max_backoff, + SecondsDecimal::rep input_initial_backoff, + SecondsDecimal::rep input_max_backoff, float input_backoff_multiplier) : max_attempts(input_max_attempts), initial_backoff(input_initial_backoff), diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index bff54a0b97..7f039e0dbc 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -453,17 +453,17 @@ std::chrono::system_clock::time_point HttpOperation::NextRetryTime() static std::mt19937 gen(rd()); static std::uniform_real_distribution dis(0.8, 1.2); - SecondsDecimal backoff = retry_policy_.initial_backoff * dis(gen); + auto backoff = retry_policy_.initial_backoff; if (retry_attempts_ > 1) { backoff = std::min(retry_policy_.initial_backoff * std::pow(retry_policy_.backoff_multiplier, static_cast(retry_attempts_ - 1)), - retry_policy_.max_backoff) * - dis(gen); + retry_policy_.max_backoff); } + backoff *= dis(gen); return last_attempt_time_ + std::chrono::duration_cast(backoff); } From cec3b988361a1539ba4bc5a4e291b6434b8fc03f Mon Sep 17 00:00:00 2001 From: Alex E Date: Sun, 29 Dec 2024 23:46:32 -0500 Subject: [PATCH 08/30] Fix overlook on iterator increment --- ext/src/http/client/curl/http_client_curl.cc | 27 ++++++++++---------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index c1df09806d..6b938087a2 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -802,20 +802,21 @@ bool HttpClient::doRetrySessions() const auto session = *retry_it; const auto operation = (nullptr != session) ? session->GetOperation().get() : nullptr; - if (operation) + if (!operation) { - if (operation->NextRetryTime() < std::chrono::system_clock::now()) - { - auto easy_handle = operation->GetCurlEasyHandle(); - curl_multi_remove_handle(multi_handle_, easy_handle); - curl_multi_add_handle(multi_handle_, easy_handle); - has_data = true; - retry_it = decltype(retry_it){pending_to_retry_sessions_.erase(std::next(retry_it).base())}; - } - else - { - ++retry_it; - } + retry_it = decltype(retry_it){pending_to_retry_sessions_.erase(std::next(retry_it).base())}; + } + else if (operation->NextRetryTime() < std::chrono::system_clock::now()) + { + auto easy_handle = operation->GetCurlEasyHandle(); + curl_multi_remove_handle(multi_handle_, easy_handle); + curl_multi_add_handle(multi_handle_, easy_handle); + retry_it = decltype(retry_it){pending_to_retry_sessions_.erase(std::next(retry_it).base())}; + has_data = true; + } + else + { + ++retry_it; } } From 082597a9dd2f556a12b5c20f501439bdfde5ae05 Mon Sep 17 00:00:00 2001 From: Alex E Date: Mon, 30 Dec 2024 01:30:55 -0500 Subject: [PATCH 09/30] Unit test http retries with exponential backoff --- .../http/client/curl/http_operation_curl.h | 8 +-- .../http/client/curl/http_operation_curl.cc | 2 +- ext/test/http/curl_http_test.cc | 67 +++++++++++++++++++ 3 files changed, 72 insertions(+), 5 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h index 529047d56b..2c3565177c 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_operation_curl.h @@ -39,9 +39,9 @@ namespace client { namespace curl { -const std::chrono::milliseconds default_http_conn_timeout(5000); // ms -const std::string http_status_regexp = "HTTP\\/\\d\\.\\d (\\d+)\\ .*"; -const std::string http_header_regexp = "(.*)\\: (.*)\\n*"; +const std::chrono::milliseconds kDefaultHttpConnTimeout(5000); // ms +const std::string kHttpStatusRegexp = "HTTP\\/\\d\\.\\d (\\d+)\\ .*"; +const std::string kHttpHeaderRegexp = "(.*)\\: (.*)\\n*"; class HttpClient; class Session; @@ -161,7 +161,7 @@ class HttpOperation opentelemetry::ext::http::client::Compression::kNone, // Default connectivity and response size options bool is_raw_response = false, - std::chrono::milliseconds http_conn_timeout = default_http_conn_timeout, + std::chrono::milliseconds http_conn_timeout = kDefaultHttpConnTimeout, bool reuse_connection = false, bool is_log_enabled = false, const opentelemetry::ext::http::client::RetryPolicy &retry_policy = {}); diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 7f039e0dbc..54077af729 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -1305,7 +1305,7 @@ Headers HttpOperation::GetResponseHeaders() // switching to string comparison. Need to debug and revert back. /*std::smatch match; - std::regex http_headers_regex(http_header_regexp); + std::regex http_headers_regex(kHttpHeaderRegexp); if (std::regex_search(header, match, http_headers_regex)) result.insert(std::pair( static_cast(match[1]), static_cast(match[2]))); diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index f1fbe495b7..73d19fdc18 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 #include +#include #include #include #include @@ -108,6 +109,17 @@ class FinishInCallbackHandler : public CustomEventHandler std::shared_ptr session_; }; +class RetryEventHandler : public CustomEventHandler +{ + void OnResponse(http_client::Response &response) noexcept override + { + ASSERT_EQ(429, response.GetStatusCode()); + ASSERT_EQ(response.GetBody().size(), 0); + is_called_.store(true, std::memory_order_release); + got_response_.store(true, std::memory_order_release); + } +}; + class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRequestCallback { protected: @@ -139,6 +151,7 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe server_.addHandler("/simple/", *this); server_.addHandler("/get/", *this); server_.addHandler("/post/", *this); + server_.addHandler("/retry/", *this); server_.start(); is_running_ = true; } @@ -170,6 +183,13 @@ class BasicCurlHttpTests : public ::testing::Test, public HTTP_SERVER_NS::HttpRe response.body = "{'k1':'v1', 'k2':'v2', 'k3':'v3'}"; response_status = 200; } + else if (request.uri == "/retry/") + { + std::unique_lock lk1(mtx_requests); + received_requests_.push_back(request); + response.headers["Content-Type"] = "text/plain"; + response_status = 429; + } cv_got_events.notify_one(); @@ -330,6 +350,52 @@ TEST_F(BasicCurlHttpTests, CurlHttpOperations) delete handler; } +TEST_F(BasicCurlHttpTests, ExponentialBackoffRetry) +{ + using ::testing::AllOf; + using ::testing::Gt; + using ::testing::Lt; + + RetryEventHandler handler; + http_client::HttpSslOptions no_ssl; + http_client::Body body; + http_client::Headers headers; + http_client::Compression compression = http_client::Compression::kNone; + http_client::RetryPolicy retry_policy = {4, 1.0f, 5.0f, 2.0f}; + + curl::HttpOperation operation(http_client::Method::Post, "http://127.0.0.1:19000/retry/", no_ssl, + &handler, headers, body, compression, false, + curl::kDefaultHttpConnTimeout, false, false, retry_policy); + + auto first_attempt_time = std::chrono::system_clock::now(); + ASSERT_EQ(CURLE_OK, operation.Send()); + ASSERT_TRUE(operation.IsRetryable()); + ASSERT_THAT( + operation.NextRetryTime().time_since_epoch().count(), + AllOf(Gt((first_attempt_time + std::chrono::milliseconds{750}).time_since_epoch().count()), + Lt((first_attempt_time + std::chrono::milliseconds{1250}).time_since_epoch().count()))); + + auto second_attempt_time = std::chrono::system_clock::now(); + ASSERT_EQ(CURLE_OK, operation.Send()); + ASSERT_TRUE(operation.IsRetryable()); + ASSERT_THAT( + operation.NextRetryTime().time_since_epoch().count(), + AllOf( + Gt((second_attempt_time + std::chrono::milliseconds{1550}).time_since_epoch().count()), + Lt((second_attempt_time + std::chrono::milliseconds{2450}).time_since_epoch().count()))); + + auto third_attempt_time = std::chrono::system_clock::now(); + ASSERT_EQ(CURLE_OK, operation.Send()); + ASSERT_TRUE(operation.IsRetryable()); + ASSERT_THAT( + operation.NextRetryTime().time_since_epoch().count(), + AllOf(Gt((third_attempt_time + std::chrono::milliseconds{3150}).time_since_epoch().count()), + Lt((third_attempt_time + std::chrono::milliseconds{4850}).time_since_epoch().count()))); + + ASSERT_EQ(CURLE_OK, operation.Send()); + ASSERT_FALSE(operation.IsRetryable()); +} + TEST_F(BasicCurlHttpTests, SendGetRequestSync) { received_requests_.clear(); @@ -559,6 +625,7 @@ TEST_F(BasicCurlHttpTests, ElegantQuitQuick) ASSERT_TRUE(handler->is_called_); ASSERT_TRUE(handler->got_response_); } + TEST_F(BasicCurlHttpTests, BackgroundThreadWaitMore) { { From 8008e8cabc137f2fc3566704765076b1b24bb3bf Mon Sep 17 00:00:00 2001 From: Alex E Date: Mon, 30 Dec 2024 10:33:01 -0500 Subject: [PATCH 10/30] Update comments for clarity --- ext/src/http/client/curl/http_client_curl.cc | 5 +++-- ext/src/http/client/curl/http_operation_curl.cc | 10 +++++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 6b938087a2..6f591c45fe 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -794,8 +794,9 @@ bool HttpClient::doRetrySessions() // Assumptions: // - This is a FIFO list so older sessions, pushed at the front, always end up at the tail - // - Retry policy is not changed once HTTP client is initialized, so same settings apply to all - // - Iterating backwards should result in removing those items that are due for a retry attempt + // - Locking not required because only the background thread would be pushing to this container + // - Retry policy is not changed once HTTP client is initialized, so same settings for everyone + // - Iterating backwards should result in removing items with minimal or no compacting required for (auto retry_it = pending_to_retry_sessions_.crbegin(); retry_it != pending_to_retry_sessions_.crend();) { diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 54077af729..3b5b79b191 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -453,8 +453,11 @@ std::chrono::system_clock::time_point HttpOperation::NextRetryTime() static std::mt19937 gen(rd()); static std::uniform_real_distribution dis(0.8, 1.2); + // The initial retry attempt will occur after initialBackoff * random(0.8, 1.2) auto backoff = retry_policy_.initial_backoff; + // After that, the n-th attempt will occur after + // min(initialBackoff*backoffMultiplier**(n-1), maxBackoff) * random(0.8, 1.2)) if (retry_attempts_ > 1) { backoff = std::min(retry_policy_.initial_backoff * @@ -463,7 +466,11 @@ std::chrono::system_clock::time_point HttpOperation::NextRetryTime() retry_policy_.max_backoff); } + // Jitter of plus or minus 0.2 is applied to the backoff delay to avoid hammering servers at the + // same time from a large number of clients. Note that this means that the backoff delay may + // actually be slightly lower than initialBackoff or slightly higher than maxBackoff backoff *= dis(gen); + return last_attempt_time_ + std::chrono::duration_cast(backoff); } @@ -1393,8 +1400,9 @@ void HttpOperation::PerformCurlMessage(CURLcode code) if (IsRetryable()) { - // Clear any request offset and response data received in previous attempt + // Clear any response data received in previous attempt ReleaseResponse(); + // Rewind request data so that read callback can re-transfer the payload request_nwrite_ = 0; } else From 48402d983a1f464d5e59fcb8e0f9a17bac121031 Mon Sep 17 00:00:00 2001 From: Alex E Date: Mon, 30 Dec 2024 11:31:08 -0500 Subject: [PATCH 11/30] Fix unit tests and minor touchups --- exporters/otlp/test/otlp_http_exporter_test.cc | 2 +- ext/src/http/client/curl/http_client_curl.cc | 2 +- ext/src/http/client/curl/http_operation_curl.cc | 5 ++++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/exporters/otlp/test/otlp_http_exporter_test.cc b/exporters/otlp/test/otlp_http_exporter_test.cc index 3f31a2476f..e193a1c2e3 100644 --- a/exporters/otlp/test/otlp_http_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_exporter_test.cc @@ -747,7 +747,7 @@ TEST_P(OtlpHttpExporterRetryIntegrationTests, StatusCodes) return response.code; }}; HTTP_SERVER_NS::HttpServer server; - server.setKeepalive(false); + server.setKeepalive(true); server.setServerName("test_server"); server.addHandler("/v1/traces", request_handler); ASSERT_EQ(server.addListeningPort(4318), 4318); diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 6f591c45fe..5dddd8b933 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -801,7 +801,7 @@ bool HttpClient::doRetrySessions() retry_it != pending_to_retry_sessions_.crend();) { const auto session = *retry_it; - const auto operation = (nullptr != session) ? session->GetOperation().get() : nullptr; + const auto operation = session ? session->GetOperation().get() : nullptr; if (!operation) { diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 3b5b79b191..3c8c559e67 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -444,7 +444,8 @@ bool HttpOperation::IsRetryable() const auto is_retryable = std::find(kRetryableStatusCodes.cbegin(), kRetryableStatusCodes.cend(), response_code_) != kRetryableStatusCodes.cend(); - return is_retryable && retry_attempts_ < retry_policy_.max_attempts; + return is_retryable && (last_curl_result_ == CURLE_OK) && + (retry_attempts_ < retry_policy_.max_attempts); } std::chrono::system_clock::time_point HttpOperation::NextRetryTime() @@ -1404,6 +1405,8 @@ void HttpOperation::PerformCurlMessage(CURLcode code) ReleaseResponse(); // Rewind request data so that read callback can re-transfer the payload request_nwrite_ = 0; + // Reset session state + DispatchEvent(opentelemetry::ext::http::client::SessionState::Connecting); } else { From 6de20c1c95c54c8fe110176fb80787e00374d4f2 Mon Sep 17 00:00:00 2001 From: Alex E Date: Mon, 30 Dec 2024 22:28:55 -0500 Subject: [PATCH 12/30] Make intent clear in options used by UTs --- .../otlp/test/otlp_http_exporter_test.cc | 28 ++++++++++++------- .../otlp_http_log_record_exporter_test.cc | 28 ++++++++++++------- .../test/otlp_http_metric_exporter_test.cc | 28 ++++++++++++------- 3 files changed, 54 insertions(+), 30 deletions(-) diff --git a/exporters/otlp/test/otlp_http_exporter_test.cc b/exporters/otlp/test/otlp_http_exporter_test.cc index e193a1c2e3..5928a39fca 100644 --- a/exporters/otlp/test/otlp_http_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_exporter_test.cc @@ -64,18 +64,26 @@ OtlpHttpClientOptions MakeOtlpHttpClientOptions(HttpRequestContentType content_t options.console_debug = true; options.timeout = std::chrono::system_clock::duration::zero(); options.http_headers.insert(std::make_pair("Custom-Header-Key", "Custom-Header-Value")); + options.retry_policy_max_attempts = 0U; + options.retry_policy_initial_backoff = 0.0f; + options.retry_policy_max_backoff = 0.0f; + options.retry_policy_backoff_multiplier = 0.0f; OtlpHttpClientOptions otlp_http_client_options( - options.url, false, /* ssl_insecure_skip_verify */ - "", /* ssl_ca_cert_path */ "", /* ssl_ca_cert_string */ - "", /* ssl_client_key_path */ - "", /* ssl_client_key_string */ "", /* ssl_client_cert_path */ - "", /* ssl_client_cert_string */ - "", /* ssl_min_tls */ - "", /* ssl_max_tls */ - "", /* ssl_cipher */ - "", /* ssl_cipher_suite */ + options.url, false, /* ssl_insecure_skip_verify */ + "", /* ssl_ca_cert_path */ + "", /* ssl_ca_cert_string */ + "", /* ssl_client_key_path */ + "", /* ssl_client_key_string */ + "", /* ssl_client_cert_path */ + "", /* ssl_client_cert_string */ + "", /* ssl_min_tls */ + "", /* ssl_max_tls */ + "", /* ssl_cipher */ + "", /* ssl_cipher_suite */ options.content_type, options.json_bytes_mapping, options.compression, options.use_json_name, - options.console_debug, options.timeout, options.http_headers, 0, 0.0, 0.0, 0); + options.console_debug, options.timeout, options.http_headers, + options.retry_policy_max_attempts, options.retry_policy_initial_backoff, + options.retry_policy_max_backoff, options.retry_policy_backoff_multiplier); if (!async_mode) { otlp_http_client_options.max_concurrent_requests = 0; diff --git a/exporters/otlp/test/otlp_http_log_record_exporter_test.cc b/exporters/otlp/test/otlp_http_log_record_exporter_test.cc index 4c5405ecb6..09fbc23eee 100644 --- a/exporters/otlp/test/otlp_http_log_record_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_log_record_exporter_test.cc @@ -58,18 +58,26 @@ OtlpHttpClientOptions MakeOtlpHttpClientOptions(HttpRequestContentType content_t options.content_type = content_type; options.console_debug = true; options.http_headers.insert(std::make_pair("Custom-Header-Key", "Custom-Header-Value")); + options.retry_policy_max_attempts = 0U; + options.retry_policy_initial_backoff = 0.0f; + options.retry_policy_max_backoff = 0.0f; + options.retry_policy_backoff_multiplier = 0.0f; OtlpHttpClientOptions otlp_http_client_options( - options.url, false, /* ssl_insecure_skip_verify */ - "", /* ssl_ca_cert_path */ "", /* ssl_ca_cert_string */ - "", /* ssl_client_key_path */ - "", /* ssl_client_key_string */ "", /* ssl_client_cert_path */ - "", /* ssl_client_cert_string */ - "", /* ssl_min_tls */ - "", /* ssl_max_tls */ - "", /* ssl_cipher */ - "", /* ssl_cipher_suite */ + options.url, false, /* ssl_insecure_skip_verify */ + "", /* ssl_ca_cert_path */ + "", /* ssl_ca_cert_string */ + "", /* ssl_client_key_path */ + "", /* ssl_client_key_string */ + "", /* ssl_client_cert_path */ + "", /* ssl_client_cert_string */ + "", /* ssl_min_tls */ + "", /* ssl_max_tls */ + "", /* ssl_cipher */ + "", /* ssl_cipher_suite */ options.content_type, options.json_bytes_mapping, options.compression, options.use_json_name, - options.console_debug, options.timeout, options.http_headers, 0, 0.0, 0.0, 0); + options.console_debug, options.timeout, options.http_headers, + options.retry_policy_max_attempts, options.retry_policy_initial_backoff, + options.retry_policy_max_backoff, options.retry_policy_backoff_multiplier); if (!async_mode) { otlp_http_client_options.max_concurrent_requests = 0; diff --git a/exporters/otlp/test/otlp_http_metric_exporter_test.cc b/exporters/otlp/test/otlp_http_metric_exporter_test.cc index b7420008d9..1ce39416f1 100644 --- a/exporters/otlp/test/otlp_http_metric_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_metric_exporter_test.cc @@ -80,18 +80,26 @@ OtlpHttpClientOptions MakeOtlpHttpClientOptions(HttpRequestContentType content_t options.content_type = content_type; options.console_debug = true; options.http_headers.insert(std::make_pair("Custom-Header-Key", "Custom-Header-Value")); + options.retry_policy_max_attempts = 0U; + options.retry_policy_initial_backoff = 0.0f; + options.retry_policy_max_backoff = 0.0f; + options.retry_policy_backoff_multiplier = 0.0f; OtlpHttpClientOptions otlp_http_client_options( - options.url, false, /* ssl_insecure_skip_verify */ - "", /* ssl_ca_cert_path */ "", /* ssl_ca_cert_string */ - "", /* ssl_client_key_path */ - "", /* ssl_client_key_string */ "", /* ssl_client_cert_path */ - "", /* ssl_client_cert_string */ - "", /* ssl_min_tls */ - "", /* ssl_max_tls */ - "", /* ssl_cipher */ - "", /* ssl_cipher_suite */ + options.url, false, /* ssl_insecure_skip_verify */ + "", /* ssl_ca_cert_path */ + "", /* ssl_ca_cert_string */ + "", /* ssl_client_key_path */ + "", /* ssl_client_key_string */ + "", /* ssl_client_cert_path */ + "", /* ssl_client_cert_string */ + "", /* ssl_min_tls */ + "", /* ssl_max_tls */ + "", /* ssl_cipher */ + "", /* ssl_cipher_suite */ options.content_type, options.json_bytes_mapping, options.compression, options.use_json_name, - options.console_debug, options.timeout, options.http_headers, 0, 0.0, 0.0, 0); + options.console_debug, options.timeout, options.http_headers, + options.retry_policy_max_attempts, options.retry_policy_initial_backoff, + options.retry_policy_max_backoff, options.retry_policy_backoff_multiplier); if (!async_mode) { otlp_http_client_options.max_concurrent_requests = 0; From 4de2f811ac030f70c0397a002c4ce3032766041a Mon Sep 17 00:00:00 2001 From: Alex E Date: Tue, 31 Dec 2024 11:12:01 -0500 Subject: [PATCH 13/30] Cleanup --- .../ext/http/client/curl/http_client_curl.h | 2 +- ext/src/http/client/curl/http_client_curl.cc | 7 ++-- .../http/client/curl/http_operation_curl.cc | 7 +++- ext/test/http/curl_http_test.cc | 34 +++++++++++++++++++ 4 files changed, 45 insertions(+), 5 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index 728558691c..1d1355545d 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -361,7 +361,7 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient std::unordered_map> pending_to_abort_sessions_; std::unordered_map pending_to_remove_session_handles_; std::list> pending_to_remove_sessions_; - std::deque pending_to_retry_sessions_; + std::deque> pending_to_retry_sessions_; std::mutex background_thread_m_; std::unique_ptr background_thread_; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 5dddd8b933..43e1022d2b 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -486,7 +486,7 @@ bool HttpClient::MaybeSpawnBackgroundThread() if (operation->IsRetryable()) { - self->pending_to_retry_sessions_.push_front(session); + self->pending_to_retry_sessions_.push_front(hold_session); } } } @@ -790,7 +790,8 @@ bool HttpClient::doRemoveSessions() bool HttpClient::doRetrySessions() { - auto has_data = false; + const auto now = std::chrono::system_clock::now(); + auto has_data = false; // Assumptions: // - This is a FIFO list so older sessions, pushed at the front, always end up at the tail @@ -807,7 +808,7 @@ bool HttpClient::doRetrySessions() { retry_it = decltype(retry_it){pending_to_retry_sessions_.erase(std::next(retry_it).base())}; } - else if (operation->NextRetryTime() < std::chrono::system_clock::now()) + else if (operation->NextRetryTime() < now) { auto easy_handle = operation->GetCurlEasyHandle(); curl_multi_remove_handle(multi_handle_, easy_handle); diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 3c8c559e67..6e57259ea0 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -289,7 +289,12 @@ HttpOperation::HttpOperation(opentelemetry::ext::http::client::Method method, compression_(compression), is_log_enabled_(is_log_enabled), retry_policy_(retry_policy), - retry_attempts_(0), + retry_attempts_((retry_policy.max_attempts > 0U && + retry_policy.initial_backoff > SecondsDecimal::zero() && + retry_policy.max_backoff > SecondsDecimal::zero() && + retry_policy.backoff_multiplier > 0.0f) + ? 0 + : retry_policy.max_attempts), response_code_(0) { /* get a curl handle */ diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 73d19fdc18..ad93df625e 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -350,6 +350,40 @@ TEST_F(BasicCurlHttpTests, CurlHttpOperations) delete handler; } +TEST_F(BasicCurlHttpTests, RetryPolicyEnabled) +{ + RetryEventHandler handler; + http_client::HttpSslOptions no_ssl; + http_client::Body body; + http_client::Headers headers; + http_client::Compression compression = http_client::Compression::kNone; + http_client::RetryPolicy retry_policy = {5, 1.0f, 5.0f, 1.5f}; + + curl::HttpOperation operation(http_client::Method::Post, "http://127.0.0.1:19000/retry/", no_ssl, + &handler, headers, body, compression, false, + curl::kDefaultHttpConnTimeout, false, false, retry_policy); + + ASSERT_EQ(CURLE_OK, operation.Send()); + ASSERT_TRUE(operation.IsRetryable()); +} + +TEST_F(BasicCurlHttpTests, RetryPolicyDisabled) +{ + RetryEventHandler handler; + http_client::HttpSslOptions no_ssl; + http_client::Body body; + http_client::Headers headers; + http_client::Compression compression = http_client::Compression::kNone; + http_client::RetryPolicy no_retry_policy = {0, 0.0f, 0.0f, 0.0f}; + + curl::HttpOperation operation(http_client::Method::Post, "http://127.0.0.1:19000/retry/", no_ssl, + &handler, headers, body, compression, false, + curl::kDefaultHttpConnTimeout, false, false, no_retry_policy); + + ASSERT_EQ(CURLE_OK, operation.Send()); + ASSERT_FALSE(operation.IsRetryable()); +} + TEST_F(BasicCurlHttpTests, ExponentialBackoffRetry) { using ::testing::AllOf; From 413b28417913129f3be0d5ce7791247aba290bc5 Mon Sep 17 00:00:00 2001 From: Alex E Date: Sat, 4 Jan 2025 17:08:42 -0500 Subject: [PATCH 14/30] Fix to support windows macro --- ext/src/http/client/curl/http_operation_curl.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 6e57259ea0..b6457c0d81 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -466,10 +466,10 @@ std::chrono::system_clock::time_point HttpOperation::NextRetryTime() // min(initialBackoff*backoffMultiplier**(n-1), maxBackoff) * random(0.8, 1.2)) if (retry_attempts_ > 1) { - backoff = std::min(retry_policy_.initial_backoff * - std::pow(retry_policy_.backoff_multiplier, - static_cast(retry_attempts_ - 1)), - retry_policy_.max_backoff); + backoff = (std::min)(retry_policy_.initial_backoff * + std::pow(retry_policy_.backoff_multiplier, + static_cast(retry_attempts_ - 1)), + retry_policy_.max_backoff); } // Jitter of plus or minus 0.2 is applied to the backoff delay to avoid hammering servers at the From 25b332c2505fc8badc2e2d3d3023981d648f4593 Mon Sep 17 00:00:00 2001 From: Alex E Date: Tue, 7 Jan 2025 11:42:58 -0500 Subject: [PATCH 15/30] Code review feedback --- exporters/otlp/src/otlp_environment.cc | 140 +++++++++++--- .../opentelemetry/sdk/common/env_variables.h | 17 ++ sdk/src/common/env_variables.cc | 172 +++++++++++++++++- sdk/test/common/env_var_test.cc | 125 ++++++++++++- 4 files changed, 417 insertions(+), 37 deletions(-) diff --git a/exporters/otlp/src/otlp_environment.cc b/exporters/otlp/src/otlp_environment.cc index 3fcc34c6ff..22efeb93bb 100644 --- a/exporters/otlp/src/otlp_environment.cc +++ b/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::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 value; + } + + return 5U; } 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 value; + } + + return 5U; } 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 value; + } + + return 5U; } 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 value; + } + + return 1.0f; } 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 value; + } + + return 1.0f; } 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 value; + } + + return 1.0f; } 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 value; + } + + return 5.0f; } 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 value; + } + + return 5.0f; } 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 value; + } + + return 5.0f; } 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 value; + } + + return 1.5f; } 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 value; + } + + return 1.5f; } 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 value; + } + + return 1.5f; } } // namespace otlp diff --git a/sdk/include/opentelemetry/sdk/common/env_variables.h b/sdk/include/opentelemetry/sdk/common/env_variables.h index a02a66c29e..7bf28e2c1c 100644 --- a/sdk/include/opentelemetry/sdk/common/env_variables.h +++ b/sdk/include/opentelemetry/sdk/common/env_variables.h @@ -4,6 +4,7 @@ #pragma once #include +#include #include #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) { diff --git a/sdk/src/common/env_variables.cc b/sdk/src/common/env_variables.cc index 586a8ed420..2d9c63278f 100644 --- a/sdk/src/common/env_variables.cc +++ b/sdk/src/common/env_variables.cc @@ -10,8 +10,12 @@ # include #endif +#include +#include #include +#include #include +#include #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/sdk/common/global_log_handler.h" @@ -23,6 +27,117 @@ namespace sdk namespace common { +template ::value, bool> = true> +constexpr bool ParseNumber(T &&input, U &output) +{ + constexpr auto max_value = std::numeric_limits::max(); + + // Skip spaces + for (; *input && std::isspace(*input); ++input) + ; + + const auto is_negative = (*input && *input == '-'); + + if (is_negative) + { + ++input; + + // Reject negative when expecting unsigned + if (std::is_unsigned::value) + { + return false; + } + } + + const auto pos = input; + U result = 0; + U temp = 0; + + for (; *input && std::isdigit(*input); ++input) + { + temp = result * 10 + (*input - '0'); + + if (max_value - temp > max_value - result) + { + return false; // Overflow protection + } + + result = temp; + } + + result *= (is_negative) ? -1 : 1; + + // Reject if nothing parsed + if (pos != input) + { + output = result; + return true; + } + + return false; +} + +template ::value, bool> = true> +constexpr bool ParseNumber(T &&input, U &output) +{ + // Skip spaces + for (; *input && std::isspace(*input); ++input) + ; + + const auto is_negative = (*input && *input == '-'); + + if (is_negative) + { + ++input; + } + + const auto pos = input; + U result = 0; + U temp = 0; + U decimal_mul = 0.1f; + bool is_decimal = false; + + for (; *input && (std::isdigit(*input) || !is_decimal); ++input) + { + if (*input == '.' && !is_decimal) + { + is_decimal = true; + continue; + } + else if (*input == '.' && is_decimal) + { + break; + } + else if (is_decimal) + { + temp = result + decimal_mul * (*input - '0'); + decimal_mul *= 0.1f; + } + else + { + temp = result * 10 + (*input - '0'); + } + + if (std::isinf(temp)) + { + return false; // Overflow protection + } + + result = temp; + } + + result *= (is_negative) ? -1.0f : 1.0f; + + // Reject if nothing parsed + if (pos != input && !std::isnan(output) && !std::isinf(output)) + { + output = result; + return true; + } + + return false; +} + bool GetRawEnvironmentVariable(const char *env_var_name, std::string &value) { #if !defined(NO_GETENV) @@ -88,16 +203,7 @@ static bool GetTimeoutFromString(const char *input, std::chrono::system_clock::d { std::chrono::system_clock::duration::rep result = 0; - // Skip spaces - for (; *input && (' ' == *input || '\t' == *input || '\r' == *input || '\n' == *input); ++input) - ; - - for (; *input && (*input >= '0' && *input <= '9'); ++input) - { - result = result * 10 + (*input - '0'); - } - - if (result == 0) + if (!ParseNumber(input, result)) { // Rejecting duration 0 as invalid. return false; @@ -193,6 +299,52 @@ bool GetStringEnvironmentVariable(const char *env_var_name, std::string &value) return true; } +bool GetUintEnvironmentVariable(const char *env_var_name, std::uint32_t &value) +{ + static constexpr auto kDefaultValue = 0U; + std::string raw_value; + bool exists = GetRawEnvironmentVariable(env_var_name, raw_value); + if (!exists || raw_value.empty()) + { + value = kDefaultValue; + return false; + } + + if (!ParseNumber(raw_value.c_str(), value)) + { + OTEL_INTERNAL_LOG_WARN("Environment variable <" << env_var_name << "> has an invalid value <" + << raw_value << ">, defaulting to " + << kDefaultValue); + value = kDefaultValue; + return false; + } + + return true; +} + +bool GetFloatEnvironmentVariable(const char *env_var_name, float &value) +{ + static constexpr auto kDefaultValue = 0.0f; + std::string raw_value; + bool exists = GetRawEnvironmentVariable(env_var_name, raw_value); + if (!exists || raw_value.empty()) + { + value = kDefaultValue; + return false; + } + + if (!ParseNumber(raw_value.c_str(), value)) + { + OTEL_INTERNAL_LOG_WARN("Environment variable <" << env_var_name << "> has an invalid value <" + << raw_value << ">, defaulting to " + << kDefaultValue); + value = kDefaultValue; + return false; + } + + return true; +} + } // namespace common } // namespace sdk OPENTELEMETRY_END_NAMESPACE diff --git a/sdk/test/common/env_var_test.cc b/sdk/test/common/env_var_test.cc index 8f64dc1f10..d529f3fcf9 100644 --- a/sdk/test/common/env_var_test.cc +++ b/sdk/test/common/env_var_test.cc @@ -15,7 +15,9 @@ using opentelemetry::sdk::common::unsetenv; using opentelemetry::sdk::common::GetBoolEnvironmentVariable; using opentelemetry::sdk::common::GetDurationEnvironmentVariable; +using opentelemetry::sdk::common::GetFloatEnvironmentVariable; using opentelemetry::sdk::common::GetStringEnvironmentVariable; +using opentelemetry::sdk::common::GetUintEnvironmentVariable; #ifndef NO_GETENV TEST(EnvVarTest, BoolEnvVar) @@ -210,4 +212,125 @@ TEST(EnvVarTest, DurationEnvVar) unsetenv("STRING_ENV_VAR_BROKEN_2"); } -#endif +TEST(EnvVarTest, UintEnvVar) +{ + unsetenv("UINT_ENV_VAR_NONE"); + setenv("UINT_ENV_VAR_EMPTY", "", 1); + setenv("UINT_ENV_VAR_POSITIVE_INT", "42", 1); + setenv("UINT_ENV_VAR_NEGATIVE_INT", "-42", 1); + setenv("UINT_ENV_VAR_POSITIVE_DEC", "12.34", 1); + setenv("UINT_ENV_VAR_NEGATIVE_DEC", "-12.34", 1); + setenv("UINT_ENV_VAR_POSITIVE_INT_MAX", "4294967295", 1); + setenv("UINT_ENV_VAR_POSITIVE_OVERFLOW", "4294967296", 1); + setenv("UINT_ENV_VAR_NEGATIVE_INT_MIN", "-2147483648", 1); + setenv("UINT_ENV_VAR_NEGATIVE_OVERFLOW", "-4294967296", 1); + setenv("UINT_ENV_VAR_WITH_NOISE", " \t \n 9.12345678.9", 1); + setenv("UINT_ENV_VAR_ONLY_SPACES", " ", 1); + + std::uint32_t value; + + ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_NONE", value)); + + ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_EMPTY", value)); + + ASSERT_TRUE(GetUintEnvironmentVariable("UINT_ENV_VAR_POSITIVE_INT", value)); + ASSERT_EQ(42, value); + + ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_NEGATIVE_INT", value)); + + ASSERT_TRUE(GetUintEnvironmentVariable("UINT_ENV_VAR_POSITIVE_DEC", value)); + ASSERT_EQ(12, value); + + ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_NEGATIVE_DEC", value)); + + ASSERT_TRUE(GetUintEnvironmentVariable("UINT_ENV_VAR_POSITIVE_INT_MAX", value)); + ASSERT_EQ(4294967295, value); + + ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_POSITIVE_OVERFLOW", value)); + + ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_NEGATIVE_INT_MIN", value)); + + ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_NEGATIVE_OVERFLOW", value)); + + ASSERT_TRUE(GetUintEnvironmentVariable("UINT_ENV_VAR_WITH_NOISE", value)); + ASSERT_EQ(9, value); + + ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_ONLY_SPACES", value)); + + unsetenv("UINT_ENV_VAR_EMPTY"); + unsetenv("UINT_ENV_VAR_POSITIVE_INT"); + unsetenv("UINT_ENV_VAR_NEGATIVE_INT"); + unsetenv("UINT_ENV_VAR_POSITIVE_DEC"); + unsetenv("UINT_ENV_VAR_NEGATIVE_DEC"); + unsetenv("UINT_ENV_VAR_POSITIVE_INT_MAX"); + unsetenv("UINT_ENV_VAR_POSITIVE_OVERFLOW"); + unsetenv("UINT_ENV_VAR_NEGATIVE_INT_MIN"); + unsetenv("UINT_ENV_VAR_NEGATIVE_OVERFLOW"); + unsetenv("UINT_ENV_VAR_WITH_NOISE"); + unsetenv("UINT_ENV_VAR_ONLY_SPACES"); +} + +TEST(EnvVarTest, FloatEnvVar) +{ + unsetenv("FLOAT_ENV_VAR_NONE"); + setenv("FLOAT_ENV_VAR_EMPTY", "", 1); + setenv("FLOAT_ENV_VAR_POSITIVE_INT", "42", 1); + setenv("FLOAT_ENV_VAR_NEGATIVE_INT", "-42", 1); + setenv("FLOAT_ENV_VAR_POSITIVE_DEC", "12.34", 1); + setenv("FLOAT_ENV_VAR_NEGATIVE_DEC", "-12.34", 1); + setenv("FLOAT_ENV_VAR_POSITIVE_INT_MAX", "4294967295", 1); + setenv("FLOAT_ENV_VAR_POSITIVE_OVERFLOW", "4294967296", 1); + setenv("FLOAT_ENV_VAR_NEGATIVE_INT_MIN", "-2147483648", 1); + setenv("FLOAT_ENV_VAR_NEGATIVE_OVERFLOW", "-4294967296", 1); + setenv("FLOAT_ENV_VAR_WITH_NOISE", " \t \n 9.12345678.9", 1); + setenv("FLOAT_ENV_VAR_ONLY_SPACES", " ", 1); + + float value; + + ASSERT_FALSE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_NONE", value)); + + ASSERT_FALSE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_EMPTY", value)); + + ASSERT_TRUE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_POSITIVE_INT", value)); + ASSERT_FLOAT_EQ(42.f, value); + + ASSERT_TRUE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_NEGATIVE_INT", value)); + ASSERT_FLOAT_EQ(-42.f, value); + + ASSERT_TRUE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_POSITIVE_DEC", value)); + ASSERT_FLOAT_EQ(12.34f, value); + + ASSERT_TRUE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_NEGATIVE_DEC", value)); + ASSERT_FLOAT_EQ(-12.34f, value); + + ASSERT_TRUE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_POSITIVE_INT_MAX", value)); + ASSERT_FLOAT_EQ(4294967295.f, value); + + ASSERT_TRUE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_POSITIVE_OVERFLOW", value)); + ASSERT_FLOAT_EQ(4294967296.f, value); + + ASSERT_TRUE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_NEGATIVE_INT_MIN", value)); + ASSERT_FLOAT_EQ(-2147483648.f, value); + + ASSERT_TRUE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_NEGATIVE_OVERFLOW", value)); + ASSERT_FLOAT_EQ(-4294967296.f, value); + + ASSERT_TRUE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_WITH_NOISE", value)); + ASSERT_FLOAT_EQ(9.12345678f, value); + + ASSERT_FALSE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_ONLY_SPACES", value)); + + unsetenv("FLOAT_ENV_VAR_EMPTY"); + unsetenv("FLOAT_ENV_VAR_POSITIVE_INT"); + unsetenv("FLOAT_ENV_VAR_NEGATIVE_INT"); + unsetenv("FLOAT_ENV_VAR_POSITIVE_DEC"); + unsetenv("FLOAT_ENV_VAR_NEGATIVE_DEC"); + unsetenv("FLOAT_ENV_VAR_POSITIVE_INT_MAX"); + unsetenv("FLOAT_ENV_VAR_POSITIVE_OVERFLOW"); + unsetenv("FLOAT_ENV_VAR_NEGATIVE_INT_MIN"); + unsetenv("FLOAT_ENV_VAR_NEGATIVE_OVERFLOW"); + unsetenv("FLOAT_ENV_VAR_WITH_NOISE"); + unsetenv("FLOAT_ENV_VAR_ONLY_SPACES"); +} + +#endif // NO_GETENV From f511021579eb29c68f9efeb9dcd6ac6118f34d43 Mon Sep 17 00:00:00 2001 From: Alex E Date: Tue, 7 Jan 2025 21:34:50 -0500 Subject: [PATCH 16/30] Code review feedback pt 2 --- sdk/src/common/env_variables.cc | 171 +++++++++----------------------- sdk/test/common/env_var_test.cc | 26 +++-- 2 files changed, 69 insertions(+), 128 deletions(-) diff --git a/sdk/src/common/env_variables.cc b/sdk/src/common/env_variables.cc index 2d9c63278f..5ed139f821 100644 --- a/sdk/src/common/env_variables.cc +++ b/sdk/src/common/env_variables.cc @@ -11,11 +11,9 @@ #endif #include -#include #include #include #include -#include #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/sdk/common/global_log_handler.h" @@ -27,117 +25,6 @@ namespace sdk namespace common { -template ::value, bool> = true> -constexpr bool ParseNumber(T &&input, U &output) -{ - constexpr auto max_value = std::numeric_limits::max(); - - // Skip spaces - for (; *input && std::isspace(*input); ++input) - ; - - const auto is_negative = (*input && *input == '-'); - - if (is_negative) - { - ++input; - - // Reject negative when expecting unsigned - if (std::is_unsigned::value) - { - return false; - } - } - - const auto pos = input; - U result = 0; - U temp = 0; - - for (; *input && std::isdigit(*input); ++input) - { - temp = result * 10 + (*input - '0'); - - if (max_value - temp > max_value - result) - { - return false; // Overflow protection - } - - result = temp; - } - - result *= (is_negative) ? -1 : 1; - - // Reject if nothing parsed - if (pos != input) - { - output = result; - return true; - } - - return false; -} - -template ::value, bool> = true> -constexpr bool ParseNumber(T &&input, U &output) -{ - // Skip spaces - for (; *input && std::isspace(*input); ++input) - ; - - const auto is_negative = (*input && *input == '-'); - - if (is_negative) - { - ++input; - } - - const auto pos = input; - U result = 0; - U temp = 0; - U decimal_mul = 0.1f; - bool is_decimal = false; - - for (; *input && (std::isdigit(*input) || !is_decimal); ++input) - { - if (*input == '.' && !is_decimal) - { - is_decimal = true; - continue; - } - else if (*input == '.' && is_decimal) - { - break; - } - else if (is_decimal) - { - temp = result + decimal_mul * (*input - '0'); - decimal_mul *= 0.1f; - } - else - { - temp = result * 10 + (*input - '0'); - } - - if (std::isinf(temp)) - { - return false; // Overflow protection - } - - result = temp; - } - - result *= (is_negative) ? -1.0f : 1.0f; - - // Reject if nothing parsed - if (pos != input && !std::isnan(output) && !std::isinf(output)) - { - output = result; - return true; - } - - return false; -} - bool GetRawEnvironmentVariable(const char *env_var_name, std::string &value) { #if !defined(NO_GETENV) @@ -203,7 +90,16 @@ static bool GetTimeoutFromString(const char *input, std::chrono::system_clock::d { std::chrono::system_clock::duration::rep result = 0; - if (!ParseNumber(input, result)) + // Skip spaces + for (; *input && std::isspace(*input); ++input) + ; + + for (; *input && std::isdigit(*input); ++input) + { + result = result * 10 + (*input - '0'); + } + + if (result == 0) { // Rejecting duration 0 as invalid. return false; @@ -304,22 +200,38 @@ bool GetUintEnvironmentVariable(const char *env_var_name, std::uint32_t &value) static constexpr auto kDefaultValue = 0U; std::string raw_value; bool exists = GetRawEnvironmentVariable(env_var_name, raw_value); + if (!exists || raw_value.empty()) { value = kDefaultValue; return false; } - if (!ParseNumber(raw_value.c_str(), value)) + const char *end = raw_value.c_str() + raw_value.length(); + char *actual_end = nullptr; + const auto temp = std::strtoul(raw_value.c_str(), &actual_end, 10); + + if (errno == ERANGE) + { + errno = 0; + OTEL_INTERNAL_LOG_WARN("Environment variable <" << env_var_name << "> is out of range <" + << raw_value << ">, defaulting to " + << kDefaultValue); + } + else if (actual_end != end || std::numeric_limits::max() < temp) { OTEL_INTERNAL_LOG_WARN("Environment variable <" << env_var_name << "> has an invalid value <" << raw_value << ">, defaulting to " << kDefaultValue); - value = kDefaultValue; - return false; + } + else + { + value = static_cast(temp); + return true; } - return true; + value = kDefaultValue; + return false; } bool GetFloatEnvironmentVariable(const char *env_var_name, float &value) @@ -327,22 +239,37 @@ bool GetFloatEnvironmentVariable(const char *env_var_name, float &value) static constexpr auto kDefaultValue = 0.0f; std::string raw_value; bool exists = GetRawEnvironmentVariable(env_var_name, raw_value); + if (!exists || raw_value.empty()) { value = kDefaultValue; return false; } - if (!ParseNumber(raw_value.c_str(), value)) + const char *end = raw_value.c_str() + raw_value.length(); + char *actual_end = nullptr; + value = std::strtof(raw_value.c_str(), &actual_end); + + if (errno == ERANGE) + { + errno = 0; + OTEL_INTERNAL_LOG_WARN("Environment variable <" << env_var_name << "> is out of range <" + << raw_value << ">, defaulting to " + << kDefaultValue); + } + else if (actual_end != end) { OTEL_INTERNAL_LOG_WARN("Environment variable <" << env_var_name << "> has an invalid value <" << raw_value << ">, defaulting to " << kDefaultValue); - value = kDefaultValue; - return false; + } + else + { + return true; } - return true; + value = kDefaultValue; + return false; } } // namespace common diff --git a/sdk/test/common/env_var_test.cc b/sdk/test/common/env_var_test.cc index d529f3fcf9..e2adf932a7 100644 --- a/sdk/test/common/env_var_test.cc +++ b/sdk/test/common/env_var_test.cc @@ -224,6 +224,8 @@ TEST(EnvVarTest, UintEnvVar) setenv("UINT_ENV_VAR_POSITIVE_OVERFLOW", "4294967296", 1); setenv("UINT_ENV_VAR_NEGATIVE_INT_MIN", "-2147483648", 1); setenv("UINT_ENV_VAR_NEGATIVE_OVERFLOW", "-4294967296", 1); + setenv("UINT_ENV_VAR_TOO_LARGE_INT", "99999999999999999999", 1); + setenv("UINT_ENV_VAR_TOO_LARGE_DEC", "3.9999e+99", 1); setenv("UINT_ENV_VAR_WITH_NOISE", " \t \n 9.12345678.9", 1); setenv("UINT_ENV_VAR_ONLY_SPACES", " ", 1); @@ -238,8 +240,7 @@ TEST(EnvVarTest, UintEnvVar) ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_NEGATIVE_INT", value)); - ASSERT_TRUE(GetUintEnvironmentVariable("UINT_ENV_VAR_POSITIVE_DEC", value)); - ASSERT_EQ(12, value); + ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_POSITIVE_DEC", value)); ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_NEGATIVE_DEC", value)); @@ -252,8 +253,11 @@ TEST(EnvVarTest, UintEnvVar) ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_NEGATIVE_OVERFLOW", value)); - ASSERT_TRUE(GetUintEnvironmentVariable("UINT_ENV_VAR_WITH_NOISE", value)); - ASSERT_EQ(9, value); + ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_TOO_LARGE_INT", value)); + + ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_TOO_LARGE_DEC", value)); + + ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_WITH_NOISE", value)); ASSERT_FALSE(GetUintEnvironmentVariable("UINT_ENV_VAR_ONLY_SPACES", value)); @@ -266,6 +270,8 @@ TEST(EnvVarTest, UintEnvVar) unsetenv("UINT_ENV_VAR_POSITIVE_OVERFLOW"); unsetenv("UINT_ENV_VAR_NEGATIVE_INT_MIN"); unsetenv("UINT_ENV_VAR_NEGATIVE_OVERFLOW"); + unsetenv("UINT_ENV_VAR_TOO_LARGE_INT"); + unsetenv("UINT_ENV_VAR_TOO_LARGE_DEC"); unsetenv("UINT_ENV_VAR_WITH_NOISE"); unsetenv("UINT_ENV_VAR_ONLY_SPACES"); } @@ -282,6 +288,8 @@ TEST(EnvVarTest, FloatEnvVar) setenv("FLOAT_ENV_VAR_POSITIVE_OVERFLOW", "4294967296", 1); setenv("FLOAT_ENV_VAR_NEGATIVE_INT_MIN", "-2147483648", 1); setenv("FLOAT_ENV_VAR_NEGATIVE_OVERFLOW", "-4294967296", 1); + setenv("FLOAT_ENV_VAR_TOO_LARGE_INT", "99999999999999999999", 1); + setenv("FLOAT_ENV_VAR_TOO_LARGE_DEC", "3.9999e+99", 1); setenv("FLOAT_ENV_VAR_WITH_NOISE", " \t \n 9.12345678.9", 1); setenv("FLOAT_ENV_VAR_ONLY_SPACES", " ", 1); @@ -315,8 +323,12 @@ TEST(EnvVarTest, FloatEnvVar) ASSERT_TRUE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_NEGATIVE_OVERFLOW", value)); ASSERT_FLOAT_EQ(-4294967296.f, value); - ASSERT_TRUE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_WITH_NOISE", value)); - ASSERT_FLOAT_EQ(9.12345678f, value); + ASSERT_TRUE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_TOO_LARGE_INT", value)); + ASSERT_FLOAT_EQ(99999999999999999999.f, value); + + ASSERT_FALSE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_TOO_LARGE_DEC", value)); + + ASSERT_FALSE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_WITH_NOISE", value)); ASSERT_FALSE(GetFloatEnvironmentVariable("FLOAT_ENV_VAR_ONLY_SPACES", value)); @@ -329,6 +341,8 @@ TEST(EnvVarTest, FloatEnvVar) unsetenv("FLOAT_ENV_VAR_POSITIVE_OVERFLOW"); unsetenv("FLOAT_ENV_VAR_NEGATIVE_INT_MIN"); unsetenv("FLOAT_ENV_VAR_NEGATIVE_OVERFLOW"); + unsetenv("FLOAT_ENV_VAR_TOO_LARGE_INT"); + unsetenv("FLOAT_ENV_VAR_TOO_LARGE_DEC"); unsetenv("FLOAT_ENV_VAR_WITH_NOISE"); unsetenv("FLOAT_ENV_VAR_ONLY_SPACES"); } From 20b347f509317fbdc4fc5603820a30d7811b2440 Mon Sep 17 00:00:00 2001 From: Alex E Date: Tue, 7 Jan 2025 22:31:34 -0500 Subject: [PATCH 17/30] Code review feedback pt 3 --- .../exporters/otlp/otlp_environment.h | 12 +-- .../exporters/otlp/otlp_http_client.h | 4 +- .../otlp/otlp_http_exporter_options.h | 4 +- .../otlp_http_log_record_exporter_options.h | 4 +- .../otlp/otlp_http_metric_exporter_options.h | 4 +- exporters/otlp/src/otlp_environment.cc | 84 +++++++++---------- exporters/otlp/src/otlp_http_exporter.cc | 23 +++-- .../otlp/src/otlp_http_log_record_exporter.cc | 21 +++-- .../otlp/src/otlp_http_metric_exporter.cc | 5 +- .../otlp/test/otlp_http_exporter_test.cc | 56 ++++++------- .../otlp_http_log_record_exporter_test.cc | 48 +++++------ .../test/otlp_http_metric_exporter_test.cc | 48 +++++------ .../ext/http/client/http_client.h | 4 +- ext/test/http/curl_http_test.cc | 9 +- 14 files changed, 163 insertions(+), 163 deletions(-) diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_environment.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_environment.h index 5eb1c58037..f6ea55d32d 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_environment.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_environment.h @@ -156,13 +156,13 @@ std::uint32_t GetOtlpDefaultTracesRetryMaxAttempts(); std::uint32_t GetOtlpDefaultMetricsRetryMaxAttempts(); std::uint32_t GetOtlpDefaultLogsRetryMaxAttempts(); -float GetOtlpDefaultTracesRetryInitialBackoff(); -float GetOtlpDefaultMetricsRetryInitialBackoff(); -float GetOtlpDefaultLogsRetryInitialBackoff(); +std::chrono::duration GetOtlpDefaultTracesRetryInitialBackoff(); +std::chrono::duration GetOtlpDefaultMetricsRetryInitialBackoff(); +std::chrono::duration GetOtlpDefaultLogsRetryInitialBackoff(); -float GetOtlpDefaultTracesRetryMaxBackoff(); -float GetOtlpDefaultMetricsRetryMaxBackoff(); -float GetOtlpDefaultLogsRetryMaxBackoff(); +std::chrono::duration GetOtlpDefaultTracesRetryMaxBackoff(); +std::chrono::duration GetOtlpDefaultMetricsRetryMaxBackoff(); +std::chrono::duration GetOtlpDefaultLogsRetryMaxBackoff(); float GetOtlpDefaultTracesRetryBackoffMultiplier(); float GetOtlpDefaultMetricsRetryBackoffMultiplier(); diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h index 7621169b5a..c348c728ec 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h @@ -106,8 +106,8 @@ struct OtlpHttpClientOptions std::chrono::system_clock::duration input_timeout, const OtlpHeaders &input_http_headers, std::uint32_t input_retry_policy_max_attempts, - float input_retry_policy_initial_backoff, - float input_retry_policy_max_backoff, + std::chrono::duration input_retry_policy_initial_backoff, + std::chrono::duration input_retry_policy_max_backoff, float input_retry_policy_backoff_multiplier, std::size_t input_concurrent_sessions = 64, std::size_t input_max_requests_per_connection = 8, 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 index 7f0785e4e8..314854f292 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter_options.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_exporter_options.h @@ -107,10 +107,10 @@ struct OPENTELEMETRY_EXPORT OtlpHttpExporterOptions std::uint32_t retry_policy_max_attempts{}; /** The initial backoff delay between retry attempts, random between (0, initial_backoff). */ - float retry_policy_initial_backoff{}; + std::chrono::duration retry_policy_initial_backoff{}; /** The maximum backoff places an upper limit on exponential backoff growth. */ - float retry_policy_max_backoff{}; + std::chrono::duration retry_policy_max_backoff{}; /** The backoff will be multiplied by this value after each retry attempt. */ float retry_policy_backoff_multiplier{}; diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_log_record_exporter_options.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_log_record_exporter_options.h index ca26a74d27..e7c47a5564 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_log_record_exporter_options.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_log_record_exporter_options.h @@ -107,10 +107,10 @@ struct OPENTELEMETRY_EXPORT OtlpHttpLogRecordExporterOptions std::uint32_t retry_policy_max_attempts{}; /** The initial backoff delay between retry attempts, random between (0, initial_backoff). */ - float retry_policy_initial_backoff{}; + std::chrono::duration retry_policy_initial_backoff{}; /** The maximum backoff places an upper limit on exponential backoff growth. */ - float retry_policy_max_backoff{}; + std::chrono::duration retry_policy_max_backoff{}; /** The backoff will be multiplied by this value after each retry attempt. */ float retry_policy_backoff_multiplier{}; diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_metric_exporter_options.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_metric_exporter_options.h index 127acbfd68..5ff1b28321 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_metric_exporter_options.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_metric_exporter_options.h @@ -110,10 +110,10 @@ struct OPENTELEMETRY_EXPORT OtlpHttpMetricExporterOptions std::uint32_t retry_policy_max_attempts{}; /** The initial backoff delay between retry attempts, random between (0, initial_backoff). */ - float retry_policy_initial_backoff{}; + std::chrono::duration retry_policy_initial_backoff{}; /** The maximum backoff places an upper limit on exponential backoff growth. */ - float retry_policy_max_backoff{}; + std::chrono::duration retry_policy_max_backoff{}; /** The backoff will be multiplied by this value after each retry attempt. */ float retry_policy_backoff_multiplier{}; diff --git a/exporters/otlp/src/otlp_environment.cc b/exporters/otlp/src/otlp_environment.cc index 22efeb93bb..02af28666c 100644 --- a/exporters/otlp/src/otlp_environment.cc +++ b/exporters/otlp/src/otlp_environment.cc @@ -1159,8 +1159,8 @@ std::string GetOtlpDefaultLogsCompression() std::uint32_t GetOtlpDefaultTracesRetryMaxAttempts() { - constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_ATTEMPTS"; - constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"; + constexpr char kSignalEnv[] = "OTEL_CPP_EXPORTER_OTLP_TRACES_RETRY_MAX_ATTEMPTS"; + constexpr char kGenericEnv[] = "OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"; std::uint32_t value{}; if (GetUintDualEnvVar(kSignalEnv, kGenericEnv, value)) @@ -1173,8 +1173,8 @@ std::uint32_t GetOtlpDefaultTracesRetryMaxAttempts() std::uint32_t GetOtlpDefaultMetricsRetryMaxAttempts() { - constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_ATTEMPTS"; - constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"; + constexpr char kSignalEnv[] = "OTEL_CPP_EXPORTER_OTLP_METRICS_RETRY_MAX_ATTEMPTS"; + constexpr char kGenericEnv[] = "OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"; std::uint32_t value{}; if (GetUintDualEnvVar(kSignalEnv, kGenericEnv, value)) @@ -1187,8 +1187,8 @@ std::uint32_t GetOtlpDefaultMetricsRetryMaxAttempts() std::uint32_t GetOtlpDefaultLogsRetryMaxAttempts() { - constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_ATTEMPTS"; - constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"; + constexpr char kSignalEnv[] = "OTEL_CPP_EXPORTER_OTLP_LOGS_RETRY_MAX_ATTEMPTS"; + constexpr char kGenericEnv[] = "OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"; std::uint32_t value{}; if (GetUintDualEnvVar(kSignalEnv, kGenericEnv, value)) @@ -1199,94 +1199,94 @@ std::uint32_t GetOtlpDefaultLogsRetryMaxAttempts() return 5U; } -float GetOtlpDefaultTracesRetryInitialBackoff() +std::chrono::duration GetOtlpDefaultTracesRetryInitialBackoff() { - constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_TRACES_RETRY_INITIAL_BACKOFF"; - constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"; + constexpr char kSignalEnv[] = "OTEL_CPP_EXPORTER_OTLP_TRACES_RETRY_INITIAL_BACKOFF"; + constexpr char kGenericEnv[] = "OTEL_CPP_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"; float value{}; if (GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value)) { - return value; + return std::chrono::duration{value}; } - return 1.0f; + return std::chrono::duration{1.0f}; } -float GetOtlpDefaultMetricsRetryInitialBackoff() +std::chrono::duration GetOtlpDefaultMetricsRetryInitialBackoff() { - constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_METRICS_RETRY_INITIAL_BACKOFF"; - constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"; + constexpr char kSignalEnv[] = "OTEL_CPP_EXPORTER_OTLP_METRICS_RETRY_INITIAL_BACKOFF"; + constexpr char kGenericEnv[] = "OTEL_CPP_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"; float value{}; if (GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value)) { - return value; + return std::chrono::duration{value}; } - return 1.0f; + return std::chrono::duration{1.0f}; } -float GetOtlpDefaultLogsRetryInitialBackoff() +std::chrono::duration GetOtlpDefaultLogsRetryInitialBackoff() { - constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_LOGS_RETRY_INITIAL_BACKOFF"; - constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"; + constexpr char kSignalEnv[] = "OTEL_CPP_EXPORTER_OTLP_LOGS_RETRY_INITIAL_BACKOFF"; + constexpr char kGenericEnv[] = "OTEL_CPP_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"; float value{}; if (GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value)) { - return value; + return std::chrono::duration{value}; } - return 1.0f; + return std::chrono::duration{1.0f}; } -float GetOtlpDefaultTracesRetryMaxBackoff() +std::chrono::duration GetOtlpDefaultTracesRetryMaxBackoff() { - constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_BACKOFF"; - constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"; + constexpr char kSignalEnv[] = "OTEL_CPP_EXPORTER_OTLP_TRACES_RETRY_MAX_BACKOFF"; + constexpr char kGenericEnv[] = "OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_BACKOFF"; float value{}; if (GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value)) { - return value; + return std::chrono::duration{value}; } - return 5.0f; + return std::chrono::duration{5.0f}; } -float GetOtlpDefaultMetricsRetryMaxBackoff() +std::chrono::duration GetOtlpDefaultMetricsRetryMaxBackoff() { - constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_BACKOFF"; - constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"; + constexpr char kSignalEnv[] = "OTEL_CPP_EXPORTER_OTLP_METRICS_RETRY_MAX_BACKOFF"; + constexpr char kGenericEnv[] = "OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_BACKOFF"; float value{}; if (GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value)) { - return value; + return std::chrono::duration{value}; } - return 5.0f; + return std::chrono::duration{5.0f}; } -float GetOtlpDefaultLogsRetryMaxBackoff() +std::chrono::duration GetOtlpDefaultLogsRetryMaxBackoff() { - constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_BACKOFF"; - constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"; + constexpr char kSignalEnv[] = "OTEL_CPP_EXPORTER_OTLP_LOGS_RETRY_MAX_BACKOFF"; + constexpr char kGenericEnv[] = "OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_BACKOFF"; float value{}; if (GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value)) { - return value; + return std::chrono::duration{value}; } - return 5.0f; + return std::chrono::duration{5.0f}; } float GetOtlpDefaultTracesRetryBackoffMultiplier() { - constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_TRACES_RETRY_BACKOFF_MULTIPLIER"; - constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"; + constexpr char kSignalEnv[] = "OTEL_CPP_EXPORTER_OTLP_TRACES_RETRY_BACKOFF_MULTIPLIER"; + constexpr char kGenericEnv[] = "OTEL_CPP_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"; float value{}; if (GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value)) @@ -1299,8 +1299,8 @@ float GetOtlpDefaultTracesRetryBackoffMultiplier() float GetOtlpDefaultMetricsRetryBackoffMultiplier() { - constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_METRICS_RETRY_BACKOFF_MULTIPLIER"; - constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"; + constexpr char kSignalEnv[] = "OTEL_CPP_EXPORTER_OTLP_METRICS_RETRY_BACKOFF_MULTIPLIER"; + constexpr char kGenericEnv[] = "OTEL_CPP_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"; float value{}; if (GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value)) @@ -1313,8 +1313,8 @@ float GetOtlpDefaultMetricsRetryBackoffMultiplier() float GetOtlpDefaultLogsRetryBackoffMultiplier() { - constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_LOGS_RETRY_BACKOFF_MULTIPLIER"; - constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"; + constexpr char kSignalEnv[] = "OTEL_CPP_EXPORTER_OTLP_LOGS_RETRY_BACKOFF_MULTIPLIER"; + constexpr char kGenericEnv[] = "OTEL_CPP_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"; float value{}; if (GetFloatDualEnvVar(kSignalEnv, kGenericEnv, value)) diff --git a/exporters/otlp/src/otlp_http_exporter.cc b/exporters/otlp/src/otlp_http_exporter.cc index 868e7730d1..2b48f36dcd 100644 --- a/exporters/otlp/src/otlp_http_exporter.cc +++ b/exporters/otlp/src/otlp_http_exporter.cc @@ -73,18 +73,17 @@ OtlpHttpExporter::OtlpHttpExporter(const OtlpHttpExporterOptions &options) OtlpHttpExporter::OtlpHttpExporter(std::unique_ptr http_client) : options_(OtlpHttpExporterOptions()), http_client_(std::move(http_client)) { - OtlpHttpExporterOptions &options = const_cast(options_); - options.url = http_client_->GetOptions().url; - options.content_type = http_client_->GetOptions().content_type; - options.json_bytes_mapping = http_client_->GetOptions().json_bytes_mapping; - options.use_json_name = http_client_->GetOptions().use_json_name; - options.console_debug = http_client_->GetOptions().console_debug; - options.timeout = http_client_->GetOptions().timeout; - options.http_headers = http_client_->GetOptions().http_headers; - options.retry_policy_max_attempts = http_client_->GetOptions().retry_policy.max_attempts; - options.retry_policy_initial_backoff = - http_client_->GetOptions().retry_policy.initial_backoff.count(); - options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff.count(); + OtlpHttpExporterOptions &options = const_cast(options_); + options.url = http_client_->GetOptions().url; + options.content_type = http_client_->GetOptions().content_type; + options.json_bytes_mapping = http_client_->GetOptions().json_bytes_mapping; + options.use_json_name = http_client_->GetOptions().use_json_name; + options.console_debug = http_client_->GetOptions().console_debug; + options.timeout = http_client_->GetOptions().timeout; + options.http_headers = http_client_->GetOptions().http_headers; + options.retry_policy_max_attempts = http_client_->GetOptions().retry_policy.max_attempts; + options.retry_policy_initial_backoff = http_client_->GetOptions().retry_policy.initial_backoff; + options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff; options.retry_policy_backoff_multiplier = http_client_->GetOptions().retry_policy.backoff_multiplier; #ifdef ENABLE_ASYNC_EXPORT diff --git a/exporters/otlp/src/otlp_http_log_record_exporter.cc b/exporters/otlp/src/otlp_http_log_record_exporter.cc index 6ebcf575f9..b48006f137 100644 --- a/exporters/otlp/src/otlp_http_log_record_exporter.cc +++ b/exporters/otlp/src/otlp_http_log_record_exporter.cc @@ -78,17 +78,16 @@ OtlpHttpLogRecordExporter::OtlpHttpLogRecordExporter(std::unique_ptr(options_); - options.url = http_client_->GetOptions().url; - options.content_type = http_client_->GetOptions().content_type; - options.json_bytes_mapping = http_client_->GetOptions().json_bytes_mapping; - options.use_json_name = http_client_->GetOptions().use_json_name; - options.console_debug = http_client_->GetOptions().console_debug; - options.timeout = http_client_->GetOptions().timeout; - options.http_headers = http_client_->GetOptions().http_headers; - options.retry_policy_max_attempts = http_client_->GetOptions().retry_policy.max_attempts; - options.retry_policy_initial_backoff = - http_client_->GetOptions().retry_policy.initial_backoff.count(); - options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff.count(); + options.url = http_client_->GetOptions().url; + options.content_type = http_client_->GetOptions().content_type; + options.json_bytes_mapping = http_client_->GetOptions().json_bytes_mapping; + options.use_json_name = http_client_->GetOptions().use_json_name; + options.console_debug = http_client_->GetOptions().console_debug; + options.timeout = http_client_->GetOptions().timeout; + options.http_headers = http_client_->GetOptions().http_headers; + options.retry_policy_max_attempts = http_client_->GetOptions().retry_policy.max_attempts; + options.retry_policy_initial_backoff = http_client_->GetOptions().retry_policy.initial_backoff; + options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff; options.retry_policy_backoff_multiplier = http_client_->GetOptions().retry_policy.backoff_multiplier; #ifdef ENABLE_ASYNC_EXPORT diff --git a/exporters/otlp/src/otlp_http_metric_exporter.cc b/exporters/otlp/src/otlp_http_metric_exporter.cc index 3ac4706497..875155e424 100644 --- a/exporters/otlp/src/otlp_http_metric_exporter.cc +++ b/exporters/otlp/src/otlp_http_metric_exporter.cc @@ -89,9 +89,8 @@ OtlpHttpMetricExporter::OtlpHttpMetricExporter(std::unique_ptr h options.timeout = http_client_->GetOptions().timeout; options.http_headers = http_client_->GetOptions().http_headers; options.retry_policy_max_attempts = http_client_->GetOptions().retry_policy.max_attempts; - options.retry_policy_initial_backoff = - http_client_->GetOptions().retry_policy.initial_backoff.count(); - options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff.count(); + options.retry_policy_initial_backoff = http_client_->GetOptions().retry_policy.initial_backoff; + options.retry_policy_max_backoff = http_client_->GetOptions().retry_policy.max_backoff; options.retry_policy_backoff_multiplier = http_client_->GetOptions().retry_policy.backoff_multiplier; #ifdef ENABLE_ASYNC_EXPORT diff --git a/exporters/otlp/test/otlp_http_exporter_test.cc b/exporters/otlp/test/otlp_http_exporter_test.cc index 5928a39fca..d72417422a 100644 --- a/exporters/otlp/test/otlp_http_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_exporter_test.cc @@ -65,8 +65,8 @@ OtlpHttpClientOptions MakeOtlpHttpClientOptions(HttpRequestContentType content_t options.timeout = std::chrono::system_clock::duration::zero(); options.http_headers.insert(std::make_pair("Custom-Header-Key", "Custom-Header-Value")); options.retry_policy_max_attempts = 0U; - options.retry_policy_initial_backoff = 0.0f; - options.retry_policy_max_backoff = 0.0f; + options.retry_policy_initial_backoff = std::chrono::duration::zero(); + options.retry_policy_max_backoff = std::chrono::duration::zero(); options.retry_policy_backoff_multiplier = 0.0f; OtlpHttpClientOptions otlp_http_client_options( options.url, false, /* ssl_insecure_skip_verify */ @@ -639,49 +639,49 @@ TEST_F(OtlpHttpExporterTestPeer, ConfigRetryDefaultValues) std::unique_ptr exporter(new OtlpHttpExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 5); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 1.0); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 5.0); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 1.0); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 5.0); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 1.5); } TEST_F(OtlpHttpExporterTestPeer, ConfigRetryValuesFromEnv) { - setenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_ATTEMPTS", "123", 1); - setenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_INITIAL_BACKOFF", "4.5", 1); - setenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_BACKOFF", "6.7", 1); - setenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_BACKOFF_MULTIPLIER", "8.9", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_TRACES_RETRY_MAX_ATTEMPTS", "123", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_TRACES_RETRY_INITIAL_BACKOFF", "4.5", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_TRACES_RETRY_MAX_BACKOFF", "6.7", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_TRACES_RETRY_BACKOFF_MULTIPLIER", "8.9", 1); std::unique_ptr exporter(new OtlpHttpExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 123); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 4.5); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 6.7); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 4.5); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 6.7); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 8.9); - unsetenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_ATTEMPTS"); - unsetenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_INITIAL_BACKOFF"); - unsetenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_MAX_BACKOFF"); - unsetenv("OTEL_EXPORTER_OTLP_TRACES_RETRY_BACKOFF_MULTIPLIER"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_TRACES_RETRY_MAX_ATTEMPTS"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_TRACES_RETRY_INITIAL_BACKOFF"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_TRACES_RETRY_MAX_BACKOFF"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_TRACES_RETRY_BACKOFF_MULTIPLIER"); } TEST_F(OtlpHttpExporterTestPeer, ConfigRetryGenericValuesFromEnv) { - setenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS", "321", 1); - setenv("OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF", "5.4", 1); - setenv("OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF", "7.6", 1); - setenv("OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER", "9.8", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS", "321", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF", "5.4", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_BACKOFF", "7.6", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER", "9.8", 1); std::unique_ptr exporter(new OtlpHttpExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 321); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 5.4); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 7.6); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 5.4); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 7.6); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 9.8); - unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"); - unsetenv("OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"); - unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"); - unsetenv("OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_BACKOFF"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"); } # endif // NO_GETENV @@ -766,15 +766,15 @@ TEST_P(OtlpHttpExporterRetryIntegrationTests, StatusCodes) if (is_retry_enabled) { opts.retry_policy_max_attempts = 5; - opts.retry_policy_initial_backoff = 0.1f; - opts.retry_policy_max_backoff = 5.0f; + opts.retry_policy_initial_backoff = std::chrono::duration{0.1f}; + opts.retry_policy_max_backoff = std::chrono::duration{5.0f}; opts.retry_policy_backoff_multiplier = 1.0f; } else { opts.retry_policy_max_attempts = 0; - opts.retry_policy_initial_backoff = 0.0f; - opts.retry_policy_max_backoff = 0.0f; + opts.retry_policy_initial_backoff = std::chrono::duration::zero(); + opts.retry_policy_max_backoff = std::chrono::duration::zero(); opts.retry_policy_backoff_multiplier = 0.0f; } diff --git a/exporters/otlp/test/otlp_http_log_record_exporter_test.cc b/exporters/otlp/test/otlp_http_log_record_exporter_test.cc index 09fbc23eee..81983c6d8b 100644 --- a/exporters/otlp/test/otlp_http_log_record_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_log_record_exporter_test.cc @@ -59,8 +59,8 @@ OtlpHttpClientOptions MakeOtlpHttpClientOptions(HttpRequestContentType content_t options.console_debug = true; options.http_headers.insert(std::make_pair("Custom-Header-Key", "Custom-Header-Value")); options.retry_policy_max_attempts = 0U; - options.retry_policy_initial_backoff = 0.0f; - options.retry_policy_max_backoff = 0.0f; + options.retry_policy_initial_backoff = std::chrono::duration::zero(); + options.retry_policy_max_backoff = std::chrono::duration::zero(); options.retry_policy_backoff_multiplier = 0.0f; OtlpHttpClientOptions otlp_http_client_options( options.url, false, /* ssl_insecure_skip_verify */ @@ -761,49 +761,49 @@ TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigRetryDefaultValues) std::unique_ptr exporter(new OtlpHttpLogRecordExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 5); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 1.0); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 5.0); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 1.0); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 5.0); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 1.5); } TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigRetryValuesFromEnv) { - setenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_ATTEMPTS", "123", 1); - setenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_INITIAL_BACKOFF", "4.5", 1); - setenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_BACKOFF", "6.7", 1); - setenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_BACKOFF_MULTIPLIER", "8.9", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_LOGS_RETRY_MAX_ATTEMPTS", "123", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_LOGS_RETRY_INITIAL_BACKOFF", "4.5", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_LOGS_RETRY_MAX_BACKOFF", "6.7", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_LOGS_RETRY_BACKOFF_MULTIPLIER", "8.9", 1); std::unique_ptr exporter(new OtlpHttpLogRecordExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 123); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 4.5); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 6.7); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 4.5); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 6.7); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 8.9); - unsetenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_ATTEMPTS"); - unsetenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_INITIAL_BACKOFF"); - unsetenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_MAX_BACKOFF"); - unsetenv("OTEL_EXPORTER_OTLP_LOGS_RETRY_BACKOFF_MULTIPLIER"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_LOGS_RETRY_MAX_ATTEMPTS"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_LOGS_RETRY_INITIAL_BACKOFF"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_LOGS_RETRY_MAX_BACKOFF"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_LOGS_RETRY_BACKOFF_MULTIPLIER"); } TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigRetryGenericValuesFromEnv) { - setenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS", "321", 1); - setenv("OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF", "5.4", 1); - setenv("OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF", "7.6", 1); - setenv("OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER", "9.8", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS", "321", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF", "5.4", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_BACKOFF", "7.6", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER", "9.8", 1); std::unique_ptr exporter(new OtlpHttpLogRecordExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 321); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 5.4); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 7.6); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 5.4); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 7.6); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 9.8); - unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"); - unsetenv("OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"); - unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"); - unsetenv("OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_BACKOFF"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"); } # endif // NO_GETENV diff --git a/exporters/otlp/test/otlp_http_metric_exporter_test.cc b/exporters/otlp/test/otlp_http_metric_exporter_test.cc index 1ce39416f1..f509962975 100644 --- a/exporters/otlp/test/otlp_http_metric_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_metric_exporter_test.cc @@ -81,8 +81,8 @@ OtlpHttpClientOptions MakeOtlpHttpClientOptions(HttpRequestContentType content_t options.console_debug = true; options.http_headers.insert(std::make_pair("Custom-Header-Key", "Custom-Header-Value")); options.retry_policy_max_attempts = 0U; - options.retry_policy_initial_backoff = 0.0f; - options.retry_policy_max_backoff = 0.0f; + options.retry_policy_initial_backoff = std::chrono::duration::zero(); + options.retry_policy_max_backoff = std::chrono::duration::zero(); options.retry_policy_backoff_multiplier = 0.0f; OtlpHttpClientOptions otlp_http_client_options( options.url, false, /* ssl_insecure_skip_verify */ @@ -1017,49 +1017,49 @@ TEST_F(OtlpHttpMetricExporterTestPeer, ConfigRetryDefaultValues) std::unique_ptr exporter(new OtlpHttpMetricExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 5); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 1.0); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 5.0); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 1.0); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 5.0); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 1.5); } TEST_F(OtlpHttpMetricExporterTestPeer, ConfigRetryValuesFromEnv) { - setenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_ATTEMPTS", "123", 1); - setenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_INITIAL_BACKOFF", "4.5", 1); - setenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_BACKOFF", "6.7", 1); - setenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_BACKOFF_MULTIPLIER", "8.9", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_METRICS_RETRY_MAX_ATTEMPTS", "123", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_METRICS_RETRY_INITIAL_BACKOFF", "4.5", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_METRICS_RETRY_MAX_BACKOFF", "6.7", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_METRICS_RETRY_BACKOFF_MULTIPLIER", "8.9", 1); std::unique_ptr exporter(new OtlpHttpMetricExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 123); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 4.5); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 6.7); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 4.5); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 6.7); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 8.9); - unsetenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_ATTEMPTS"); - unsetenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_INITIAL_BACKOFF"); - unsetenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_MAX_BACKOFF"); - unsetenv("OTEL_EXPORTER_OTLP_METRICS_RETRY_BACKOFF_MULTIPLIER"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_METRICS_RETRY_MAX_ATTEMPTS"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_METRICS_RETRY_INITIAL_BACKOFF"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_METRICS_RETRY_MAX_BACKOFF"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_METRICS_RETRY_BACKOFF_MULTIPLIER"); } TEST_F(OtlpHttpMetricExporterTestPeer, ConfigRetryGenericValuesFromEnv) { - setenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS", "321", 1); - setenv("OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF", "5.4", 1); - setenv("OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF", "7.6", 1); - setenv("OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER", "9.8", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS", "321", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF", "5.4", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_BACKOFF", "7.6", 1); + setenv("OTEL_CPP_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER", "9.8", 1); std::unique_ptr exporter(new OtlpHttpMetricExporter()); const auto options = GetOptions(exporter); ASSERT_EQ(options.retry_policy_max_attempts, 321); - ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff, 5.4); - ASSERT_FLOAT_EQ(options.retry_policy_max_backoff, 7.6); + ASSERT_FLOAT_EQ(options.retry_policy_initial_backoff.count(), 5.4); + ASSERT_FLOAT_EQ(options.retry_policy_max_backoff.count(), 7.6); ASSERT_FLOAT_EQ(options.retry_policy_backoff_multiplier, 9.8); - unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"); - unsetenv("OTEL_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"); - unsetenv("OTEL_EXPORTER_OTLP_RETRY_MAX_BACKOFF"); - unsetenv("OTEL_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_ATTEMPTS"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_RETRY_INITIAL_BACKOFF"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_RETRY_MAX_BACKOFF"); + unsetenv("OTEL_CPP_EXPORTER_OTLP_RETRY_BACKOFF_MULTIPLIER"); } #endif // NO_GETENV diff --git a/ext/include/opentelemetry/ext/http/client/http_client.h b/ext/include/opentelemetry/ext/http/client/http_client.h index 8dbdc682ee..36f4e2d0a6 100644 --- a/ext/include/opentelemetry/ext/http/client/http_client.h +++ b/ext/include/opentelemetry/ext/http/client/http_client.h @@ -233,8 +233,8 @@ struct RetryPolicy RetryPolicy() = default; RetryPolicy(std::uint32_t input_max_attempts, - SecondsDecimal::rep input_initial_backoff, - SecondsDecimal::rep input_max_backoff, + SecondsDecimal input_initial_backoff, + SecondsDecimal input_max_backoff, float input_backoff_multiplier) : max_attempts(input_max_attempts), initial_backoff(input_initial_backoff), diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index ad93df625e..2732a667c1 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -357,7 +357,8 @@ TEST_F(BasicCurlHttpTests, RetryPolicyEnabled) http_client::Body body; http_client::Headers headers; http_client::Compression compression = http_client::Compression::kNone; - http_client::RetryPolicy retry_policy = {5, 1.0f, 5.0f, 1.5f}; + http_client::RetryPolicy retry_policy = {5, std::chrono::duration{1.0f}, + std::chrono::duration{5.0f}, 1.5f}; curl::HttpOperation operation(http_client::Method::Post, "http://127.0.0.1:19000/retry/", no_ssl, &handler, headers, body, compression, false, @@ -374,7 +375,8 @@ TEST_F(BasicCurlHttpTests, RetryPolicyDisabled) http_client::Body body; http_client::Headers headers; http_client::Compression compression = http_client::Compression::kNone; - http_client::RetryPolicy no_retry_policy = {0, 0.0f, 0.0f, 0.0f}; + http_client::RetryPolicy no_retry_policy = {0, std::chrono::duration::zero(), + std::chrono::duration::zero(), 0.0f}; curl::HttpOperation operation(http_client::Method::Post, "http://127.0.0.1:19000/retry/", no_ssl, &handler, headers, body, compression, false, @@ -395,7 +397,8 @@ TEST_F(BasicCurlHttpTests, ExponentialBackoffRetry) http_client::Body body; http_client::Headers headers; http_client::Compression compression = http_client::Compression::kNone; - http_client::RetryPolicy retry_policy = {4, 1.0f, 5.0f, 2.0f}; + http_client::RetryPolicy retry_policy = {4, std::chrono::duration{1.0f}, + std::chrono::duration{5.0f}, 2.0f}; curl::HttpOperation operation(http_client::Method::Post, "http://127.0.0.1:19000/retry/", no_ssl, &handler, headers, body, compression, false, From da7c6363049ed7b6dc20583246a2b92d97c76070 Mon Sep 17 00:00:00 2001 From: Alex E Date: Tue, 7 Jan 2025 23:55:15 -0500 Subject: [PATCH 18/30] Change to strtoull to ensure portability on msvc --- sdk/src/common/env_variables.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/src/common/env_variables.cc b/sdk/src/common/env_variables.cc index 5ed139f821..0fc9646901 100644 --- a/sdk/src/common/env_variables.cc +++ b/sdk/src/common/env_variables.cc @@ -209,7 +209,7 @@ bool GetUintEnvironmentVariable(const char *env_var_name, std::uint32_t &value) const char *end = raw_value.c_str() + raw_value.length(); char *actual_end = nullptr; - const auto temp = std::strtoul(raw_value.c_str(), &actual_end, 10); + const auto temp = std::strtoull(raw_value.c_str(), &actual_end, 10); if (errno == ERANGE) { From cd816b63ab0a78b39595a56fa11fc297e0aaa52d Mon Sep 17 00:00:00 2001 From: Alex E Date: Wed, 8 Jan 2025 00:29:21 -0500 Subject: [PATCH 19/30] Include errno --- sdk/src/common/env_variables.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/src/common/env_variables.cc b/sdk/src/common/env_variables.cc index 0fc9646901..016f738d4e 100644 --- a/sdk/src/common/env_variables.cc +++ b/sdk/src/common/env_variables.cc @@ -11,6 +11,7 @@ #endif #include +#include #include #include #include From 169b4a6f9b8dce24002a6bb30d527f71bd3f003f Mon Sep 17 00:00:00 2001 From: Alex E Date: Wed, 8 Jan 2025 18:36:48 -0500 Subject: [PATCH 20/30] Bail out earlier from retry list --- ext/src/http/client/curl/http_client_curl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 43e1022d2b..687984d60a 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -818,7 +818,7 @@ bool HttpClient::doRetrySessions() } else { - ++retry_it; + break; } } From 2e3ec77370a2701f09098c3021620751294c9908 Mon Sep 17 00:00:00 2001 From: Alex E Date: Wed, 8 Jan 2025 20:07:50 -0500 Subject: [PATCH 21/30] Fixes for iwyu --- exporters/otlp/src/otlp_environment.cc | 1 + exporters/otlp/src/otlp_http_exporter.cc | 1 + exporters/otlp/src/otlp_http_log_record_exporter.cc | 1 + exporters/otlp/src/otlp_http_metric_exporter.cc | 1 + ext/src/http/client/curl/http_client_curl.cc | 2 ++ ext/src/http/client/curl/http_operation_curl.cc | 1 + ext/test/http/curl_http_test.cc | 7 +++++-- sdk/test/common/env_var_test.cc | 4 +++- 8 files changed, 15 insertions(+), 3 deletions(-) diff --git a/exporters/otlp/src/otlp_environment.cc b/exporters/otlp/src/otlp_environment.cc index d653992496..c6d5aaefee 100644 --- a/exporters/otlp/src/otlp_environment.cc +++ b/exporters/otlp/src/otlp_environment.cc @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 #include +#include #include #include #include diff --git a/exporters/otlp/src/otlp_http_exporter.cc b/exporters/otlp/src/otlp_http_exporter.cc index 997d0ca5eb..69fe9ec48f 100644 --- a/exporters/otlp/src/otlp_http_exporter.cc +++ b/exporters/otlp/src/otlp_http_exporter.cc @@ -14,6 +14,7 @@ #include "opentelemetry/exporters/otlp/otlp_http_exporter_options.h" #include "opentelemetry/exporters/otlp/otlp_recordable.h" #include "opentelemetry/exporters/otlp/otlp_recordable_utils.h" +#include "opentelemetry/ext/http/client/http_client.h" #include "opentelemetry/nostd/span.h" #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/sdk/common/exporter_utils.h" diff --git a/exporters/otlp/src/otlp_http_log_record_exporter.cc b/exporters/otlp/src/otlp_http_log_record_exporter.cc index bc4fc3c662..f87a488582 100644 --- a/exporters/otlp/src/otlp_http_log_record_exporter.cc +++ b/exporters/otlp/src/otlp_http_log_record_exporter.cc @@ -14,6 +14,7 @@ #include "opentelemetry/exporters/otlp/otlp_http_log_record_exporter_options.h" #include "opentelemetry/exporters/otlp/otlp_log_recordable.h" #include "opentelemetry/exporters/otlp/otlp_recordable_utils.h" +#include "opentelemetry/ext/http/client/http_client.h" #include "opentelemetry/nostd/span.h" #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/sdk/common/exporter_utils.h" diff --git a/exporters/otlp/src/otlp_http_metric_exporter.cc b/exporters/otlp/src/otlp_http_metric_exporter.cc index 23b1a3e04a..58300c0b06 100644 --- a/exporters/otlp/src/otlp_http_metric_exporter.cc +++ b/exporters/otlp/src/otlp_http_metric_exporter.cc @@ -14,6 +14,7 @@ #include "opentelemetry/exporters/otlp/otlp_http_metric_exporter.h" #include "opentelemetry/exporters/otlp/otlp_http_metric_exporter_options.h" #include "opentelemetry/exporters/otlp/otlp_metric_utils.h" +#include "opentelemetry/ext/http/client/http_client.h" #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/sdk/common/exporter_utils.h" #include "opentelemetry/sdk/common/global_log_handler.h" diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 1fb7033ae9..0dc55dcc01 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -9,7 +9,9 @@ #include #include #include +#include #include +#include #include #include #include diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index de04ebb946..73a3a35d08 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 05a0bf814b..adbedca8ce 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -1,10 +1,13 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 +#include #include -#include -#include +#include "gmock/gmock.h" +#include "gtest/gtest.h" + #include +#include #include #include #include diff --git a/sdk/test/common/env_var_test.cc b/sdk/test/common/env_var_test.cc index e2adf932a7..b4cc0d2e12 100644 --- a/sdk/test/common/env_var_test.cc +++ b/sdk/test/common/env_var_test.cc @@ -2,8 +2,10 @@ // SPDX-License-Identifier: Apache-2.0 #include -#include + #include +#include +#include #include #include "opentelemetry/sdk/common/env_variables.h" From cb60f09be712a6c489ba0e0c5dd21837c546a705 Mon Sep 17 00:00:00 2001 From: Alex E Date: Wed, 8 Jan 2025 22:42:58 -0500 Subject: [PATCH 22/30] Fixes for clang-tidy --- exporters/otlp/src/otlp_environment.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporters/otlp/src/otlp_environment.cc b/exporters/otlp/src/otlp_environment.cc index c6d5aaefee..a7bc95b321 100644 --- a/exporters/otlp/src/otlp_environment.cc +++ b/exporters/otlp/src/otlp_environment.cc @@ -96,7 +96,7 @@ static bool GetUintDualEnvVar(const char *signal_name, return exists; } -static float GetFloatDualEnvVar(const char *signal_name, const char *generic_name, float &value) +static bool GetFloatDualEnvVar(const char *signal_name, const char *generic_name, float &value) { bool exists; From f8e10c771865bc25d37c26a5b0de0ef7b6623a2f Mon Sep 17 00:00:00 2001 From: Alex E Date: Thu, 9 Jan 2025 07:05:33 -0500 Subject: [PATCH 23/30] Add to changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52f280eb22..42b7fcc60d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,9 @@ Increment the: * [EXPORTER] Fix scope attributes missing from otlp traces metrics [#3185](https://github.com/open-telemetry/opentelemetry-cpp/pull/3185) + [EXPORTER] Support handling retry-able errors for OTLP/HTTP + [#3223](https://github.com/open-telemetry/opentelemetry-cpp/pull/3223) + ## [1.18 2024-11-25] * [EXPORTER] Fix crash in ElasticsearchLogRecordExporter From 0e33217ed13d4984f122001a006cbc40ad59383a Mon Sep 17 00:00:00 2001 From: Alex E Date: Thu, 9 Jan 2025 21:10:22 -0500 Subject: [PATCH 24/30] Make it an opt-in feature --- CMakeLists.txt | 3 +++ api/CMakeLists.txt | 5 +++++ ci/do_ci.sh | 4 ++++ exporters/otlp/test/otlp_http_exporter_test.cc | 2 ++ ext/src/http/client/curl/http_operation_curl.cc | 4 ++++ ext/test/http/curl_http_test.cc | 2 ++ 6 files changed, 20 insertions(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 408ee43046..9448ff5728 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -212,6 +212,9 @@ if(NOT WITH_STL STREQUAL "OFF") endif() endif() +option(WITH_OTLP_RETRY_PREVIEW + "Whether to enable experimental retry functionality" OFF) + option(WITH_OTLP_GRPC_SSL_MTLS_PREVIEW "Whether to enable mTLS support fro gRPC" OFF) diff --git a/api/CMakeLists.txt b/api/CMakeLists.txt index 78e97ad029..65a123293d 100644 --- a/api/CMakeLists.txt +++ b/api/CMakeLists.txt @@ -116,6 +116,11 @@ target_compile_definitions( opentelemetry_api INTERFACE OPENTELEMETRY_ABI_VERSION_NO=${OPENTELEMETRY_ABI_VERSION_NO}) +if(WITH_OTLP_RETRY_PREVIEW) + target_compile_definitions(opentelemetry_api + INTERFACE ENABLE_OTLP_RETRY_PREVIEW) +endif() + if(WITH_OTLP_GRPC_SSL_MTLS_PREVIEW) target_compile_definitions(opentelemetry_api INTERFACE ENABLE_OTLP_GRPC_SSL_MTLS_PREVIEW) diff --git a/ci/do_ci.sh b/ci/do_ci.sh index 11dbe4ef80..0f6add0ff5 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -131,6 +131,7 @@ elif [[ "$1" == "cmake.maintainer.sync.test" ]]; then -DOTELCPP_MAINTAINER_MODE=ON \ -DWITH_NO_DEPRECATED_CODE=ON \ -DWITH_OTLP_HTTP_COMPRESSION=ON \ + -DWITH_OTLP_RETRY_PREVIEW=ON \ ${IWYU} \ "${SRC_DIR}" eval "$MAKE_COMMAND" @@ -153,6 +154,7 @@ elif [[ "$1" == "cmake.maintainer.async.test" ]]; then -DOTELCPP_MAINTAINER_MODE=ON \ -DWITH_NO_DEPRECATED_CODE=ON \ -DWITH_OTLP_HTTP_COMPRESSION=ON \ + -DWITH_OTLP_RETRY_PREVIEW=ON \ ${IWYU} \ "${SRC_DIR}" eval "$MAKE_COMMAND" @@ -176,6 +178,7 @@ elif [[ "$1" == "cmake.maintainer.cpp11.async.test" ]]; then -DOTELCPP_MAINTAINER_MODE=ON \ -DWITH_NO_DEPRECATED_CODE=ON \ -DWITH_OTLP_HTTP_COMPRESSION=ON \ + -DWITH_OTLP_RETRY_PREVIEW=ON \ "${SRC_DIR}" make -k -j $(nproc) make test @@ -199,6 +202,7 @@ elif [[ "$1" == "cmake.maintainer.abiv2.test" ]]; then -DWITH_ABI_VERSION_1=OFF \ -DWITH_ABI_VERSION_2=ON \ -DWITH_OTLP_HTTP_COMPRESSION=ON \ + -DWITH_OTLP_RETRY_PREVIEW=ON \ ${IWYU} \ "${SRC_DIR}" eval "$MAKE_COMMAND" diff --git a/exporters/otlp/test/otlp_http_exporter_test.cc b/exporters/otlp/test/otlp_http_exporter_test.cc index d72417422a..01ca140b7d 100644 --- a/exporters/otlp/test/otlp_http_exporter_test.cc +++ b/exporters/otlp/test/otlp_http_exporter_test.cc @@ -685,6 +685,7 @@ TEST_F(OtlpHttpExporterTestPeer, ConfigRetryGenericValuesFromEnv) } # endif // NO_GETENV +# ifdef ENABLE_OTLP_RETRY_PREVIEW using StatusCodeVector = std::vector; class OtlpHttpExporterRetryIntegrationTests @@ -787,6 +788,7 @@ TEST_P(OtlpHttpExporterRetryIntegrationTests, StatusCodes) ASSERT_EQ(expected_attempts, request_count); } +# endif // ENABLE_OTLP_RETRY_PREVIEW } // namespace otlp } // namespace exporter diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 73a3a35d08..8ea8764528 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -439,6 +439,7 @@ void HttpOperation::Cleanup() bool HttpOperation::IsRetryable() { +#ifdef ENABLE_OTLP_RETRY_PREVIEW static constexpr auto kRetryableStatusCodes = std::array{ 429, // Too Many Requests 502, // Bad Gateway @@ -451,6 +452,9 @@ bool HttpOperation::IsRetryable() return is_retryable && (last_curl_result_ == CURLE_OK) && (retry_attempts_ < retry_policy_.max_attempts); +#else + return false; +#endif // ENABLE_OTLP_RETRY_PREVIEW } std::chrono::system_clock::time_point HttpOperation::NextRetryTime() diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index adbedca8ce..34725d8821 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -354,6 +354,7 @@ TEST_F(BasicCurlHttpTests, CurlHttpOperations) delete handler; } +#ifdef ENABLE_OTLP_RETRY_PREVIEW TEST_F(BasicCurlHttpTests, RetryPolicyEnabled) { RetryEventHandler handler; @@ -436,6 +437,7 @@ TEST_F(BasicCurlHttpTests, ExponentialBackoffRetry) ASSERT_EQ(CURLE_OK, operation.Send()); ASSERT_FALSE(operation.IsRetryable()); } +#endif // ENABLE_OTLP_RETRY_PREVIEW TEST_F(BasicCurlHttpTests, SendGetRequestSync) { From e8bdb6653030c3cd2f5de452fec40707d791c1f9 Mon Sep 17 00:00:00 2001 From: Alex E Date: Thu, 9 Jan 2025 22:44:54 -0500 Subject: [PATCH 25/30] Guard includes for iwyu --- ext/src/http/client/curl/http_operation_curl.cc | 6 +++++- ext/test/http/curl_http_test.cc | 7 +++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/ext/src/http/client/curl/http_operation_curl.cc b/ext/src/http/client/curl/http_operation_curl.cc index 8ea8764528..1af00a0ff5 100644 --- a/ext/src/http/client/curl/http_operation_curl.cc +++ b/ext/src/http/client/curl/http_operation_curl.cc @@ -4,8 +4,12 @@ #include #include #include + +#ifdef ENABLE_OTLP_RETRY_PREVIEW +# include +#endif // ENABLE_OTLP_RETRY_PREVIEW + #include -#include #include #include #include diff --git a/ext/test/http/curl_http_test.cc b/ext/test/http/curl_http_test.cc index 34725d8821..e8299202b0 100644 --- a/ext/test/http/curl_http_test.cc +++ b/ext/test/http/curl_http_test.cc @@ -1,11 +1,14 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -#include #include -#include "gmock/gmock.h" #include "gtest/gtest.h" +#ifdef ENABLE_OTLP_RETRY_PREVIEW +# include +# include "gmock/gmock.h" +#endif // ENABLE_OTLP_RETRY_PREVIEW + #include #include #include From f8b6a091d64d4e482bc341aa28fc4cb806a5d598 Mon Sep 17 00:00:00 2001 From: Alex E Date: Sat, 11 Jan 2025 11:57:22 -0500 Subject: [PATCH 26/30] Slightly faster code, slightly clearer intent --- ext/src/http/client/curl/http_client_curl.cc | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 0dc55dcc01..97a42867ec 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -489,7 +489,7 @@ bool HttpClient::MaybeSpawnBackgroundThread() if (operation->IsRetryable()) { - self->pending_to_retry_sessions_.push_front(hold_session); + self->pending_to_retry_sessions_.push_back(hold_session); } } } @@ -797,26 +797,25 @@ bool HttpClient::doRetrySessions() auto has_data = false; // Assumptions: - // - This is a FIFO list so older sessions, pushed at the front, always end up at the tail + // - This is a FIFO list so older sessions, pushed at the back, always end up at the front // - Locking not required because only the background thread would be pushing to this container // - Retry policy is not changed once HTTP client is initialized, so same settings for everyone - // - Iterating backwards should result in removing items with minimal or no compacting required - for (auto retry_it = pending_to_retry_sessions_.crbegin(); - retry_it != pending_to_retry_sessions_.crend();) + for (auto retry_it = pending_to_retry_sessions_.cbegin(); + retry_it != pending_to_retry_sessions_.cend();) { const auto session = *retry_it; const auto operation = session ? session->GetOperation().get() : nullptr; if (!operation) { - retry_it = decltype(retry_it){pending_to_retry_sessions_.erase(std::next(retry_it).base())}; + retry_it = pending_to_retry_sessions_.erase(retry_it); } else if (operation->NextRetryTime() < now) { auto easy_handle = operation->GetCurlEasyHandle(); curl_multi_remove_handle(multi_handle_, easy_handle); curl_multi_add_handle(multi_handle_, easy_handle); - retry_it = decltype(retry_it){pending_to_retry_sessions_.erase(std::next(retry_it).base())}; + retry_it = pending_to_retry_sessions_.erase(retry_it); has_data = true; } else From 797fda18f4e63e39c5c1d8dabfc7ceabb18ea80e Mon Sep 17 00:00:00 2001 From: Alex E Date: Sat, 11 Jan 2025 13:20:26 -0500 Subject: [PATCH 27/30] Remove unused include --- ext/src/http/client/curl/http_client_curl.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 97a42867ec..b031a2e2fc 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -11,7 +11,6 @@ #include #include #include -#include #include #include #include From facc19d5597342019f115b9176c29f3de9332af6 Mon Sep 17 00:00:00 2001 From: Alex E Date: Thu, 16 Jan 2025 21:30:41 -0500 Subject: [PATCH 28/30] Prevent background thread from exiting when pending retries --- .../opentelemetry/ext/http/client/curl/http_client_curl.h | 2 +- ext/src/http/client/curl/http_client_curl.cc | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h index 511b6d66ab..77bd6bf89c 100644 --- a/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h +++ b/ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h @@ -348,7 +348,7 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient bool doAddSessions(); bool doAbortSessions(); bool doRemoveSessions(); - bool doRetrySessions(); + bool doRetrySessions(bool report_all); void resetMultiHandle(); std::mutex multi_handle_m_; diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 98989637ee..854f52a458 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -546,7 +546,7 @@ bool HttpClient::MaybeSpawnBackgroundThread() } // Check if pending easy handles can be retried - if (self->doRetrySessions()) + if (self->doRetrySessions(false)) { still_running = 1; } @@ -602,7 +602,7 @@ bool HttpClient::MaybeSpawnBackgroundThread() } // Check if pending easy handles can be retried - if (self->doRetrySessions()) + if (self->doRetrySessions(true)) { still_running = 1; } @@ -830,7 +830,7 @@ bool HttpClient::doRemoveSessions() return has_data; } -bool HttpClient::doRetrySessions() +bool HttpClient::doRetrySessions(bool report_all) { const auto now = std::chrono::system_clock::now(); auto has_data = false; @@ -863,7 +863,7 @@ bool HttpClient::doRetrySessions() } } - return has_data; + return report_all ? !pending_to_retry_sessions_.empty() : has_data; } void HttpClient::resetMultiHandle() From 63d7d7d5edd4a36cdaa5b770620e1841dba5b099 Mon Sep 17 00:00:00 2001 From: Alex E Date: Fri, 17 Jan 2025 07:08:49 -0500 Subject: [PATCH 29/30] Address nit and minor logic flaw --- ext/src/http/client/curl/http_client_curl.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 854f52a458..e2c2528ea8 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -832,6 +832,7 @@ bool HttpClient::doRemoveSessions() bool HttpClient::doRetrySessions(bool report_all) { +#ifdef ENABLE_OTLP_RETRY_PREVIEW const auto now = std::chrono::system_clock::now(); auto has_data = false; @@ -863,7 +864,11 @@ bool HttpClient::doRetrySessions(bool report_all) } } - return report_all ? !pending_to_retry_sessions_.empty() : has_data; + report_all = report_all && !pending_to_retry_sessions_.empty(); + return has_data || report_all; +#else + return false; +#endif // ENABLE_OTLP_RETRY_PREVIEW } void HttpClient::resetMultiHandle() From 905e7aeda9b60e9a7276be4753ef4352d95433c0 Mon Sep 17 00:00:00 2001 From: Alex E Date: Fri, 17 Jan 2025 08:25:17 -0500 Subject: [PATCH 30/30] Fix clang tidy warning --- ext/src/http/client/curl/http_client_curl.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index e2c2528ea8..a65410d058 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -830,9 +830,9 @@ bool HttpClient::doRemoveSessions() return has_data; } +#ifdef ENABLE_OTLP_RETRY_PREVIEW bool HttpClient::doRetrySessions(bool report_all) { -#ifdef ENABLE_OTLP_RETRY_PREVIEW const auto now = std::chrono::system_clock::now(); auto has_data = false; @@ -866,10 +866,13 @@ bool HttpClient::doRetrySessions(bool report_all) report_all = report_all && !pending_to_retry_sessions_.empty(); return has_data || report_all; +} #else +bool HttpClient::doRetrySessions(bool /* report_all */) +{ return false; -#endif // ENABLE_OTLP_RETRY_PREVIEW } +#endif // ENABLE_OTLP_RETRY_PREVIEW void HttpClient::resetMultiHandle() {