From 200f6abd4564a06e629de9e5724ff0565348a69d Mon Sep 17 00:00:00 2001 From: Punya Biswal Date: Sun, 1 Sep 2024 15:13:56 -0400 Subject: [PATCH 1/2] Fix overflow in timeout logic Also use steady clock consistently. Prior to this change, the test would fail under ASAN: bazel test --config=asan --test_output=errors //sdk/test/metrics:meter_provider_sdk_test INFO: Analyzed target //sdk/test/metrics:meter_provider_sdk_test (0 packages loaded, 0 targets configured). FAIL: //sdk/test/metrics:meter_provider_sdk_test (see /private/var/tmp/_bazel_punya/e3bd968ba61238cdeb1a5537fc3dbf7d/execroot/_main/bazel-out/darwin_arm64-fastbuild-asan/testlogs/sdk/test/metrics/meter_provider_sdk_test/test.log) INFO: From Testing //sdk/test/metrics:meter_provider_sdk_test: ==================== Test output for //sdk/test/metrics:meter_provider_sdk_test: Running main() from gmock_main.cc [==========] Running 1 test from 1 test suite. [----------] Global test environment set-up. [----------] 1 test from MeterProvider [ RUN ] MeterProvider.GetMeter [Warning] File: sdk/src/metrics/meter_provider.cc:65 [MeterProvider::GetMeter] Library name is empty. [Warning] File: sdk/src/metrics/meter_provider.cc:65 [MeterProvider::GetMeter] Library name is empty. /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__chrono/duration.h:102:59: runtime error: signed integer overflow: 9221646818050376183 * 1000 cannot be represented in type '_Ct' (aka 'long long') SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/v1/__chrono/duration.h:102:59 in ================================================================================ INFO: Found 1 test target... Target //sdk/test/metrics:meter_provider_sdk_test up-to-date: bazel-bin/sdk/test/metrics/meter_provider_sdk_test INFO: Elapsed time: 2.251s, Critical Path: 2.13s INFO: 5 processes: 5 darwin-sandbox. INFO: Build completed, 1 test FAILED, 5 total actions //sdk/test/metrics:meter_provider_sdk_test FAILED in 0.6s /private/var/tmp/_bazel_punya/e3bd968ba61238cdeb1a5537fc3dbf7d/execroot/_main/bazel-out/darwin_arm64-fastbuild-asan/testlogs/sdk/test/metrics/meter_provider_sdk_test/test.log Executed 1 out of 1 test: 1 fails locally. Fix overflow in periodic_exporting_metric_reader --- .../periodic_exporting_metric_reader.cc | 2 +- sdk/src/metrics/meter_context.cc | 34 +++++++------------ 2 files changed, 13 insertions(+), 23 deletions(-) diff --git a/sdk/src/metrics/export/periodic_exporting_metric_reader.cc b/sdk/src/metrics/export/periodic_exporting_metric_reader.cc index 1ebfc2a1b6..42fd6003ae 100644 --- a/sdk/src/metrics/export/periodic_exporting_metric_reader.cc +++ b/sdk/src/metrics/export/periodic_exporting_metric_reader.cc @@ -218,7 +218,7 @@ bool PeriodicExportingMetricReader::OnForceFlush(std::chrono::microseconds timeo // - If original `timeout` is `zero`, use that in exporter::forceflush // - Else if remaining `timeout_steady` more than zero, use that in exporter::forceflush // - Else don't invoke exporter::forceflush ( as remaining time is zero or less) - if (timeout <= std::chrono::steady_clock::duration::zero()) + if (timeout <= std::chrono::milliseconds::duration::zero()) { result = exporter_->ForceFlush(std::chrono::duration_cast(timeout)); diff --git a/sdk/src/metrics/meter_context.cc b/sdk/src/metrics/meter_context.cc index 40b9da42b2..42efce76fb 100644 --- a/sdk/src/metrics/meter_context.cc +++ b/sdk/src/metrics/meter_context.cc @@ -168,46 +168,36 @@ bool MeterContext::ForceFlush(std::chrono::microseconds timeout) noexcept bool result = true; // Simultaneous flush not allowed. const std::lock_guard locked(forceflush_lock_); - // Convert to nanos to prevent overflow - auto timeout_ns = (std::chrono::nanoseconds::max)(); - if (std::chrono::duration_cast(timeout_ns) > timeout) - { - timeout_ns = std::chrono::duration_cast(timeout); - } - - auto current_time = std::chrono::system_clock::now(); - std::chrono::system_clock::time_point expire_time; - auto overflow_checker = (std::chrono::system_clock::time_point::max)(); - // check if the expected expire time doesn't overflow. - if (overflow_checker - current_time > timeout_ns) + auto time_remaining = (std::chrono::steady_clock::duration::max)(); + if (std::chrono::duration_cast(time_remaining) > timeout) { - expire_time = - current_time + std::chrono::duration_cast(timeout_ns); + time_remaining = timeout; } - else + + auto current_time = std::chrono::steady_clock::now(); + auto expire_time = (std::chrono::steady_clock::time_point::max)(); + if (expire_time - current_time > time_remaining) { - // overflow happens, reset expire time to max. - expire_time = overflow_checker; + expire_time = current_time + time_remaining; } for (auto &collector : collectors_) { if (!std::static_pointer_cast(collector)->ForceFlush( - std::chrono::duration_cast(timeout_ns))) + std::chrono::duration_cast(time_remaining))) { result = false; } - current_time = std::chrono::system_clock::now(); - + current_time = std::chrono::steady_clock::now(); if (expire_time >= current_time) { - timeout_ns = std::chrono::duration_cast(expire_time - current_time); + time_remaining = expire_time - current_time; } else { - timeout_ns = std::chrono::nanoseconds::zero(); + time_remaining = std::chrono::steady_clock::duration::zero(); } } if (!result) From 2bfc306a9ac5a9cfcaa1a70357877d8b89d9c3fc Mon Sep 17 00:00:00 2001 From: Punya Biswal Date: Sun, 1 Sep 2024 13:01:58 -0400 Subject: [PATCH 2/2] Add missing tests to Bazel build by globbing test files Previously, we were missing * instrument_metadata_validator_test * observable_registry_test * cardinality_limit_test * periodic_exporting_metric_reader_test And there were no checks in place to prevent things from getting worse. Remove unnecessary exception checks in attributes_hashmap_test, which simplifies the build and CI script. Resolve symbol collision using anonymous namespaces. --- ci/do_ci.sh | 4 +- sdk/test/metrics/BUILD | 303 +----------------- sdk/test/metrics/async_instruments_test.cc | 3 + sdk/test/metrics/attributes_hashmap_test.cc | 10 +- sdk/test/metrics/meter_test.cc | 2 +- .../periodic_exporting_metric_reader_test.cc | 2 +- 6 files changed, 14 insertions(+), 310 deletions(-) diff --git a/ci/do_ci.sh b/ci/do_ci.sh index 6d339c8d4e..4cd97a7f43 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -484,8 +484,8 @@ elif [[ "$1" == "bazel.noexcept" ]]; then # there are some exceptions and error handling code from the Prometheus Client # as well as Opentracing shim (due to some third party code in its Opentracing dependency) # that make this test always fail. Ignore these packages in the noexcept test here. - bazel $BAZEL_STARTUP_OPTIONS build --copt=-fno-exceptions $BAZEL_OPTIONS_ASYNC -- //... -//exporters/prometheus/... -//examples/prometheus/... -//sdk/test/metrics:attributes_hashmap_test -//opentracing-shim/... - bazel $BAZEL_STARTUP_OPTIONS test --copt=-fno-exceptions $BAZEL_TEST_OPTIONS_ASYNC -- //... -//exporters/prometheus/... -//examples/prometheus/... -//sdk/test/metrics:attributes_hashmap_test -//opentracing-shim/... + bazel $BAZEL_STARTUP_OPTIONS build --copt=-fno-exceptions $BAZEL_OPTIONS_ASYNC -- //... -//exporters/prometheus/... -//examples/prometheus/... -//opentracing-shim/... + bazel $BAZEL_STARTUP_OPTIONS test --copt=-fno-exceptions $BAZEL_TEST_OPTIONS_ASYNC -- //... -//exporters/prometheus/... -//examples/prometheus/... -//opentracing-shim/... exit 0 elif [[ "$1" == "bazel.nortti" ]]; then # there are some exceptions and error handling code from the Prometheus Client diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD index 3cf379494d..70d0cc063a 100644 --- a/sdk/test/metrics/BUILD +++ b/sdk/test/metrics/BUILD @@ -19,307 +19,8 @@ cc_library( ) cc_test( - name = "meter_test", - srcs = [ - "meter_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "meter_provider_sdk_test", - srcs = [ - "meter_provider_sdk_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "//sdk/src/resource", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "metric_reader_test", - srcs = [ - "metric_reader_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "//sdk/src/resource", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "histogram_test", - srcs = [ - "histogram_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "//sdk/src/resource", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "view_registry_test", - srcs = [ - "view_registry_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "//sdk/src/resource", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "aggregation_test", - srcs = [ - "aggregation_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "//sdk/src/resource", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "sync_metric_storage_counter_test", - srcs = [ - "sync_metric_storage_counter_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "//sdk/src/resource", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "sync_metric_storage_up_down_counter_test", - srcs = [ - "sync_metric_storage_up_down_counter_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "//sdk/src/resource", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "sync_metric_storage_histogram_test", - srcs = [ - "sync_metric_storage_histogram_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "//sdk/src/resource", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "sync_instruments_test", - srcs = [ - "sync_instruments_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "//sdk/src/resource", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "async_instruments_test", - srcs = [ - "async_instruments_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "async_metric_storage_test", - srcs = [ - "async_metric_storage_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "//sdk/src/resource", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "observer_result_test", - srcs = [ - "observer_result_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "//sdk/src/resource", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "multi_metric_storage_test", - srcs = [ - "multi_metric_storage_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "//sdk/src/resource", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "attributes_processor_test", - srcs = [ - "attributes_processor_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "attributes_hashmap_test", - srcs = [ - "attributes_hashmap_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "circular_buffer_counter_test", - srcs = [ - "circular_buffer_counter_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "//sdk/src/metrics", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "base2_exponential_histogram_indexer_test", - srcs = [ - "base2_exponential_histogram_indexer_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "histogram_aggregation_test", - srcs = [ - "histogram_aggregation_test.cc", - ], - tags = [ - "metrics", - "test", - ], - deps = [ - "metrics_common_test_utils", - "@com_google_googletest//:gtest_main", - ], -) - -cc_test( - name = "sum_aggregation_test", - srcs = [ - "sum_aggregation_test.cc", - ], + name = "all_tests", + srcs = glob(["*_test.cc"]), tags = [ "metrics", "test", diff --git a/sdk/test/metrics/async_instruments_test.cc b/sdk/test/metrics/async_instruments_test.cc index b7ad885bbd..01a421e046 100644 --- a/sdk/test/metrics/async_instruments_test.cc +++ b/sdk/test/metrics/async_instruments_test.cc @@ -12,10 +12,13 @@ using namespace opentelemetry::sdk::metrics; using M = std::map; +namespace +{ // NOLINTNEXTLINE void asyc_generate_measurements(opentelemetry::metrics::ObserverResult /* observer */, void * /* state */) {} +} // namespace TEST(AsyncInstruments, ObservableInstrument) { diff --git a/sdk/test/metrics/attributes_hashmap_test.cc b/sdk/test/metrics/attributes_hashmap_test.cc index 9ae8f5cfd5..7d77e6166a 100644 --- a/sdk/test/metrics/attributes_hashmap_test.cc +++ b/sdk/test/metrics/attributes_hashmap_test.cc @@ -26,14 +26,14 @@ TEST(AttributesHashMap, BasicTests) std::unique_ptr aggregation1( new DropAggregation()); // = std::unique_ptr(new DropAggregation); hash_map.Set(m1, std::move(aggregation1), hash); - EXPECT_NO_THROW(hash_map.Get(hash)->Aggregate(static_cast(1))); + hash_map.Get(hash)->Aggregate(static_cast(1)); EXPECT_EQ(hash_map.Size(), 1); EXPECT_EQ(hash_map.Has(hash), true); // Set same key again auto aggregation2 = std::unique_ptr(new DropAggregation()); hash_map.Set(m1, std::move(aggregation2), hash); - EXPECT_NO_THROW(hash_map.Get(hash)->Aggregate(static_cast(1))); + hash_map.Get(hash)->Aggregate(static_cast(1)); EXPECT_EQ(hash_map.Size(), 1); EXPECT_EQ(hash_map.Has(hash), true); @@ -44,7 +44,7 @@ TEST(AttributesHashMap, BasicTests) hash_map.Set(m3, std::move(aggregation3), hash3); EXPECT_EQ(hash_map.Has(hash), true); EXPECT_EQ(hash_map.Has(hash3), true); - EXPECT_NO_THROW(hash_map.Get(hash3)->Aggregate(static_cast(1))); + hash_map.Get(hash3)->Aggregate(static_cast(1)); EXPECT_EQ(hash_map.Size(), 2); // GetOrSetDefault @@ -54,8 +54,8 @@ TEST(AttributesHashMap, BasicTests) }; MetricAttributes m4 = {{"k1", "v1"}, {"k2", "v2"}, {"k3", "v3"}}; auto hash4 = opentelemetry::sdk::common::GetHashForAttributeMap(m4); - EXPECT_NO_THROW(hash_map.GetOrSetDefault(m4, create_default_aggregation, hash4) - ->Aggregate(static_cast(1))); + hash_map.GetOrSetDefault(m4, create_default_aggregation, hash4) + ->Aggregate(static_cast(1)); EXPECT_EQ(hash_map.Size(), 3); // Set attributes with different order - shouldn't create a new entry. diff --git a/sdk/test/metrics/meter_test.cc b/sdk/test/metrics/meter_test.cc index b5ed86f55a..cd66737465 100644 --- a/sdk/test/metrics/meter_test.cc +++ b/sdk/test/metrics/meter_test.cc @@ -28,7 +28,6 @@ nostd::shared_ptr InitMeter(MetricReader **metricReaderPtr, auto meter = provider->GetMeter(meter_name); return meter; } -} // namespace void asyc_generate_measurements(opentelemetry::metrics::ObserverResult observer, void * /* state */) { @@ -36,6 +35,7 @@ void asyc_generate_measurements(opentelemetry::metrics::ObserverResult observer, nostd::get>>(observer); observer_long->Observe(10); } +} // namespace TEST(MeterTest, BasicAsyncTests) { diff --git a/sdk/test/metrics/periodic_exporting_metric_reader_test.cc b/sdk/test/metrics/periodic_exporting_metric_reader_test.cc index 277a20582f..2eec5e8c61 100644 --- a/sdk/test/metrics/periodic_exporting_metric_reader_test.cc +++ b/sdk/test/metrics/periodic_exporting_metric_reader_test.cc @@ -83,7 +83,7 @@ TEST(PeriodicExporingMetricReader, BasicTests) MockMetricProducer producer; reader->SetMetricProducer(&producer); std::this_thread::sleep_for(std::chrono::milliseconds(2000)); - EXPECT_NO_THROW(reader->ForceFlush()); + reader->ForceFlush(); reader->Shutdown(); EXPECT_EQ(static_cast(exporter_ptr)->GetDataCount(), static_cast(&producer)->GetDataCount());