[CODE HEALTH] clean up nostd::variant access in otlp exporters

CHANGELOG.md

@@ -15,6 +15,9 @@ Increment the:
 
 ## [Unreleased]
 
+* [CODE HEALTH] clean up nostd::variant access in otlp exporters
+  [#3973](https://github.com/open-telemetry/opentelemetry-cpp/pull/3973)
+
 * [SDK] Add `TracerProvider::UpdateTracerConfigurator()` and example
   [#4065](https://github.com/open-telemetry/opentelemetry-cpp/issues/4065)
 

CMakeLists.txt

@@ -529,6 +529,7 @@ message(STATUS "---------------------------------------------")
 message(STATUS "WITH_NLOHMANN_JSON: ${USE_NLOHMANN_JSON}")
 message(STATUS "WITH_CURL_LOGGING: ${WITH_CURL_LOGGING}")
 message(STATUS "WITH_OTLP_HTTP_COMPRESSION: ${WITH_OTLP_HTTP_COMPRESSION}")
+message(STATUS "WITH_OTLP_UTF8_VALIDITY: ${WITH_OTLP_UTF8_VALIDITY}")
 message(STATUS "---------------------------------------------")
 message(STATUS "examples and test options")
 message(STATUS "---------------------------------------------")

exporters/otlp/BUILD

@@ -633,6 +633,20 @@ cc_library(
     ],
 )
 
+cc_test(
+    name = "otlp_populate_attribute_utils_test",
+    srcs = ["test/otlp_populate_attribute_utils_test.cc"],
+    tags = [
+        "otlp",
+        "test",
+    ],
+    deps = [
+        ":otlp_recordable",
+        "//sdk/src/metrics",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
 cc_test(
     name = "otlp_recordable_test",
     srcs = ["test/otlp_recordable_test.cc"],

exporters/otlp/CMakeLists.txt

@@ -871,6 +871,21 @@ if(WITH_OTLP_FILE)
 endif()
 
 if(BUILD_TESTING)
+  add_executable(otlp_populate_attribute_utils_test
+                 test/otlp_populate_attribute_utils_test.cc)
+  target_link_libraries(
+    otlp_populate_attribute_utils_test ${GTEST_BOTH_LIBRARIES}
+    ${CMAKE_THREAD_LIBS_INIT} opentelemetry_otlp_recordable
+    protobuf::libprotobuf)
+  gtest_add_tests(
+    TARGET otlp_populate_attribute_utils_test
+    TEST_PREFIX exporter.otlp.
+    TEST_LIST otlp_populate_attribute_utils_test)
+  if(WITH_OTLP_UTF8_VALIDITY AND TARGET utf8_range::utf8_validity)
+    target_compile_definitions(otlp_populate_attribute_utils_test
+                               PRIVATE ENABLE_OTLP_UTF8_VALIDITY)
+  endif()
+
   add_executable(otlp_recordable_test test/otlp_recordable_test.cc)
   target_link_libraries(
     otlp_recordable_test ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}

exporters/otlp/src/otlp_http_client.cc

@@ -749,13 +749,21 @@ sdk::common::ExportResult OtlpHttpClient::Export(
     std::function<bool(opentelemetry::sdk::common::ExportResult)> &&result_callback,
     std::size_t max_running_requests) noexcept
 {
-  auto session = createSession(message, std::move(result_callback));
-  if (opentelemetry::nostd::holds_alternative<sdk::common::ExportResult>(session))
   {
-    return opentelemetry::nostd::get<sdk::common::ExportResult>(session);
-  }
+    auto created = createSession(message, std::move(result_callback));
+    if (const auto *result = opentelemetry::nostd::get_if<sdk::common::ExportResult>(&created))
+    {
+      return *result;
+    }
 
-  addSession(std::move(opentelemetry::nostd::get<HttpSessionData>(session)));
+    // created contains a valid HttpSessionData object
+    auto *session_data = opentelemetry::nostd::get_if<HttpSessionData>(&created);
+    if (session_data == nullptr)
+    {
+      return sdk::common::ExportResult::kFailure;
+    }
+    addSession(std::move(*session_data));
+  }  // created (and the moved-from HttpSessionData) released here
 
   // Wait for the response to be received
   if (options_.console_debug)

exporters/otlp/src/otlp_metric_utils.cc

@@ -1,7 +1,7 @@
 // Copyright The OpenTelemetry Authors
 // SPDX-License-Identifier: Apache-2.0
 
-#include <stdint.h>
+#include <cstdint>
 #include <memory>
 #include <utility>
 #include <vector>
@@ -11,6 +11,7 @@
 #include "opentelemetry/exporters/otlp/otlp_populate_attribute_utils.h"
 #include "opentelemetry/exporters/otlp/otlp_preferred_temporality.h"
 #include "opentelemetry/nostd/variant.h"
+#include "opentelemetry/sdk/common/global_log_handler.h"
 #include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h"
 #include "opentelemetry/sdk/metrics/data/circular_buffer.h"
 #include "opentelemetry/sdk/metrics/data/metric_data.h"
@@ -36,6 +37,18 @@ namespace otlp
 {
 namespace metric_sdk = opentelemetry::sdk::metrics;
 
+namespace
+{
+
+struct NumberDataPointValueSetter
+{
+  proto::metrics::v1::NumberDataPoint *point;
+  void operator()(int64_t v) const noexcept { point->set_as_int(v); }
+  void operator()(double v) const noexcept { point->set_as_double(v); }
+};
+
+}  // namespace
+
 proto::metrics::v1::AggregationTemporality OtlpMetricUtils::GetProtoAggregationTemporality(
     const opentelemetry::sdk::metrics::AggregationTemporality &aggregation_temporality) noexcept
 {
@@ -87,23 +100,24 @@ void OtlpMetricUtils::ConvertSumMetric(const metric_sdk::MetricData &metric_data
       (metric_data.instrument_descriptor.type_ == metric_sdk::InstrumentType::kObservableCounter));
   auto start_ts = metric_data.start_ts.time_since_epoch().count();
   auto ts       = metric_data.end_ts.time_since_epoch().count();
-  for (auto &point_data_with_attributes : metric_data.point_data_attr_)
+  for (const auto &point_data_with_attributes : metric_data.point_data_attr_)
   {
     proto::metrics::v1::NumberDataPoint *proto_sum_point_data = sum->add_data_points();
     proto_sum_point_data->set_start_time_unix_nano(start_ts);
     proto_sum_point_data->set_time_unix_nano(ts);
-    auto sum_data = nostd::get<sdk::metrics::SumPointData>(point_data_with_attributes.point_data);
-
-    if ((nostd::holds_alternative<int64_t>(sum_data.value_)))
-    {
-      proto_sum_point_data->set_as_int(nostd::get<int64_t>(sum_data.value_));
-    }
-    else
+    const auto *maybe_sum_data =
+        nostd::get_if<sdk::metrics::SumPointData>(&point_data_with_attributes.point_data);
+    if (maybe_sum_data == nullptr)
     {
-      proto_sum_point_data->set_as_double(nostd::get<double>(sum_data.value_));
+      OTEL_INTERNAL_LOG_ERROR(
+          "[OTLP Metrics] ConvertSumMetric: point data type mismatch, skipping data point");
+      continue;
     }
+    const auto &sum_data = *maybe_sum_data;
+
+    nostd::visit(NumberDataPointValueSetter{proto_sum_point_data}, sum_data.value_);
     // set attributes
-    for (auto &kv_attr : point_data_with_attributes.attributes)
+    for (const auto &kv_attr : point_data_with_attributes.attributes)
     {
       OtlpPopulateAttributeUtils::PopulateAttribute(proto_sum_point_data->add_attributes(),
                                                     kv_attr.first, kv_attr.second, false);
@@ -119,63 +133,48 @@ void OtlpMetricUtils::ConvertHistogramMetric(
       GetProtoAggregationTemporality(metric_data.aggregation_temporality));
   auto start_ts = metric_data.start_ts.time_since_epoch().count();
   auto ts       = metric_data.end_ts.time_since_epoch().count();
-  for (auto &point_data_with_attributes : metric_data.point_data_attr_)
+  for (const auto &point_data_with_attributes : metric_data.point_data_attr_)
   {
     proto::metrics::v1::HistogramDataPoint *proto_histogram_point_data =
         histogram->add_data_points();
     proto_histogram_point_data->set_start_time_unix_nano(start_ts);
     proto_histogram_point_data->set_time_unix_nano(ts);
-    auto histogram_data =
-        nostd::get<sdk::metrics::HistogramPointData>(point_data_with_attributes.point_data);
-    // sum
-    if ((nostd::holds_alternative<int64_t>(histogram_data.sum_)))
+    const auto *maybe_histogram_data =
+        nostd::get_if<sdk::metrics::HistogramPointData>(&point_data_with_attributes.point_data);
+    if (maybe_histogram_data == nullptr)
     {
-      // Use static_cast to avoid C4244 in MSVC
-      proto_histogram_point_data->set_sum(
-          static_cast<double>(nostd::get<int64_t>(histogram_data.sum_)));
-    }
-    else
-    {
-      proto_histogram_point_data->set_sum(nostd::get<double>(histogram_data.sum_));
+      OTEL_INTERNAL_LOG_ERROR(
+          "[OTLP Metrics] ConvertHistogramMetric: point data type mismatch, skipping data point");
+      continue;
     }
+    const auto &histogram_data = *maybe_histogram_data;
+    // sum
+    // Use static_cast to avoid C4244 in MSVC
+    proto_histogram_point_data->set_sum(
+        nostd::visit([](auto v) { return static_cast<double>(v); }, histogram_data.sum_));
     // count
     proto_histogram_point_data->set_count(histogram_data.count_);
     if (histogram_data.record_min_max_)
     {
-      if (nostd::holds_alternative<int64_t>(histogram_data.min_))
-      {
-        // Use static_cast to avoid C4244 in MSVC
-        proto_histogram_point_data->set_min(
-            static_cast<double>(nostd::get<int64_t>(histogram_data.min_)));
-      }
-      else
-      {
-        proto_histogram_point_data->set_min(nostd::get<double>(histogram_data.min_));
-      }
-      if (nostd::holds_alternative<int64_t>(histogram_data.max_))
-      {
-        // Use static_cast to avoid C4244 in MSVC
-        proto_histogram_point_data->set_max(
-            static_cast<double>(nostd::get<int64_t>(histogram_data.max_)));
-      }
-      else
-      {
-        proto_histogram_point_data->set_max(nostd::get<double>(histogram_data.max_));
-      }
+      // Use static_cast to avoid C4244 in MSVC
+      proto_histogram_point_data->set_min(
+          nostd::visit([](auto v) { return static_cast<double>(v); }, histogram_data.min_));
+      proto_histogram_point_data->set_max(
+          nostd::visit([](auto v) { return static_cast<double>(v); }, histogram_data.max_));
     }
     // buckets
 
-    for (auto bound : histogram_data.boundaries_)
+    for (const auto &bound : histogram_data.boundaries_)
     {
       proto_histogram_point_data->add_explicit_bounds(bound);
     }
     // bucket counts
-    for (auto bucket_value : histogram_data.counts_)
+    for (const auto &bucket_value : histogram_data.counts_)
     {
       proto_histogram_point_data->add_bucket_counts(bucket_value);
     }
     // attributes
-    for (auto &kv_attr : point_data_with_attributes.attributes)
+    for (const auto &kv_attr : point_data_with_attributes.attributes)
     {
       OtlpPopulateAttributeUtils::PopulateAttribute(proto_histogram_point_data->add_attributes(),
                                                     kv_attr.first, kv_attr.second, false);
@@ -191,18 +190,23 @@ void OtlpMetricUtils::ConvertExponentialHistogramMetric(
       GetProtoAggregationTemporality(metric_data.aggregation_temporality));
   auto start_ts = metric_data.start_ts.time_since_epoch().count();
   auto ts       = metric_data.end_ts.time_since_epoch().count();
-  for (auto &point_data_with_attributes : metric_data.point_data_attr_)
+  for (const auto &point_data_with_attributes : metric_data.point_data_attr_)
   {
     proto::metrics::v1::ExponentialHistogramDataPoint *proto_histogram_point_data =
         histogram->add_data_points();
     proto_histogram_point_data->set_start_time_unix_nano(start_ts);
     proto_histogram_point_data->set_time_unix_nano(ts);
-    auto histogram_data = nostd::get<sdk::metrics::Base2ExponentialHistogramPointData>(
-        point_data_with_attributes.point_data);
-    if (histogram_data.positive_buckets_ == nullptr && histogram_data.negative_buckets_ == nullptr)
+    const auto *maybe_histogram_data =
+        nostd::get_if<sdk::metrics::Base2ExponentialHistogramPointData>(
+            &point_data_with_attributes.point_data);
+    if (maybe_histogram_data == nullptr)
     {
+      OTEL_INTERNAL_LOG_ERROR(
+          "[OTLP Metrics] ConvertExponentialHistogramMetric: point data type mismatch, skipping "
+          "data point");
       continue;
     }
+    const auto &histogram_data = *maybe_histogram_data;
     // sum
     proto_histogram_point_data->set_sum(histogram_data.sum_);
     proto_histogram_point_data->set_count(histogram_data.count_);
@@ -212,7 +216,7 @@ void OtlpMetricUtils::ConvertExponentialHistogramMetric(
       proto_histogram_point_data->set_max(histogram_data.max_);
     }
     // negative buckets
-    if (!histogram_data.negative_buckets_->Empty())
+    if (histogram_data.negative_buckets_ != nullptr && !histogram_data.negative_buckets_->Empty())
     {
       auto negative_buckets = proto_histogram_point_data->mutable_negative();
       negative_buckets->set_offset(histogram_data.negative_buckets_->StartIndex());
@@ -224,7 +228,7 @@ void OtlpMetricUtils::ConvertExponentialHistogramMetric(
       }
     }
     // positive buckets
-    if (!histogram_data.positive_buckets_->Empty())
+    if (histogram_data.positive_buckets_ != nullptr && !histogram_data.positive_buckets_->Empty())
     {
       auto positive_buckets = proto_histogram_point_data->mutable_positive();
       positive_buckets->set_offset(histogram_data.positive_buckets_->StartIndex());
@@ -239,7 +243,7 @@ void OtlpMetricUtils::ConvertExponentialHistogramMetric(
     proto_histogram_point_data->set_zero_count(histogram_data.zero_count_);
 
     // attributes
-    for (auto &kv_attr : point_data_with_attributes.attributes)
+    for (const auto &kv_attr : point_data_with_attributes.attributes)
     {
       OtlpPopulateAttributeUtils::PopulateAttribute(proto_histogram_point_data->add_attributes(),
                                                     kv_attr.first, kv_attr.second, false);
@@ -252,24 +256,24 @@ void OtlpMetricUtils::ConvertGaugeMetric(const opentelemetry::sdk::metrics::Metr
 {
   auto start_ts = metric_data.start_ts.time_since_epoch().count();
   auto ts       = metric_data.end_ts.time_since_epoch().count();
-  for (auto &point_data_with_attributes : metric_data.point_data_attr_)
+  for (const auto &point_data_with_attributes : metric_data.point_data_attr_)
   {
     proto::metrics::v1::NumberDataPoint *proto_gauge_point_data = gauge->add_data_points();
     proto_gauge_point_data->set_start_time_unix_nano(start_ts);
     proto_gauge_point_data->set_time_unix_nano(ts);
-    auto gauge_data =
-        nostd::get<sdk::metrics::LastValuePointData>(point_data_with_attributes.point_data);
-
-    if ((nostd::holds_alternative<int64_t>(gauge_data.value_)))
+    const auto *maybe_gauge_data =
+        nostd::get_if<sdk::metrics::LastValuePointData>(&point_data_with_attributes.point_data);
+    if (maybe_gauge_data == nullptr)
     {
-      proto_gauge_point_data->set_as_int(nostd::get<int64_t>(gauge_data.value_));
-    }
-    else
-    {
-      proto_gauge_point_data->set_as_double(nostd::get<double>(gauge_data.value_));
+      OTEL_INTERNAL_LOG_ERROR(
+          "[OTLP Metrics] ConvertGaugeMetric: point data type mismatch, skipping data point");
+      continue;
     }
+    const auto &gauge_data = *maybe_gauge_data;
+
+    nostd::visit(NumberDataPointValueSetter{proto_gauge_point_data}, gauge_data.value_);
     // set attributes
-    for (auto &kv_attr : point_data_with_attributes.attributes)
+    for (const auto &kv_attr : point_data_with_attributes.attributes)
     {
       OtlpPopulateAttributeUtils::PopulateAttribute(proto_gauge_point_data->add_attributes(),
                                                     kv_attr.first, kv_attr.second, false);
@@ -321,6 +325,8 @@ void OtlpMetricUtils::PopulateResourceMetrics(
   {
     if (scope_metrics.scope_ == nullptr)
     {
+      OTEL_INTERNAL_LOG_ERROR(
+          "[OTLP Metrics] PopulateResourceMetrics: scope is null, skipping scope metrics");
       continue;
     }
     auto scope_lib_metrics                         = resource_metrics->add_scope_metrics();