From 60cd0f279d409ba599195cb7cc9681cc21358128 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 30 Aug 2022 16:48:58 -0700 Subject: [PATCH 1/2] fix --- .../metrics/state/temporal_metric_storage.cc | 37 +++++++------- sdk/test/metrics/sync_metric_storage_test.cc | 48 +++++++++++++++++++ 2 files changed, 68 insertions(+), 17 deletions(-) diff --git a/sdk/src/metrics/state/temporal_metric_storage.cc b/sdk/src/metrics/state/temporal_metric_storage.cc index 3eea56917b..eb29286d8d 100644 --- a/sdk/src/metrics/state/temporal_metric_storage.cc +++ b/sdk/src/metrics/state/temporal_metric_storage.cc @@ -34,9 +34,12 @@ bool TemporalMetricStorage::buildMetrics(CollectorHandle *collector, opentelemetry::common::SystemTimestamp last_collection_ts = sdk_start_ts; AggregationTemporality aggregation_temporarily = collector->GetAggregationTemporality(instrument_descriptor_.type_); - for (auto &col : collectors) + if (delta_metrics->Size()) { - unreported_metrics_[col.get()].push_back(delta_metrics); + for (auto &col : collectors) + { + unreported_metrics_[col.get()].push_back(delta_metrics); + } } // Get the unreported metrics for the `collector` from `unreported metrics stash` @@ -88,20 +91,20 @@ bool TemporalMetricStorage::buildMetrics(CollectorHandle *collector, if (aggregation_temporarily == AggregationTemporality::kCumulative) { // merge current delta to previous cumulative - last_aggr_hashmap->GetAllEnteries([&merged_metrics, this](const MetricAttributes &attributes, - Aggregation &aggregation) { - auto agg = merged_metrics->Get(attributes); - if (agg) - { - merged_metrics->Set(attributes, agg->Merge(aggregation)); - } - else - { - merged_metrics->Set( - attributes, DefaultAggregation::CreateAggregation(instrument_descriptor_, nullptr)); - } - return true; - }); + last_aggr_hashmap->GetAllEnteries( + [&merged_metrics, this](const MetricAttributes &attributes, Aggregation &aggregation) { + auto agg = merged_metrics->Get(attributes); + if (agg) + { + merged_metrics->Set(attributes, agg->Merge(aggregation)); + } + else + { + auto def_agg = DefaultAggregation::CreateAggregation(instrument_descriptor_, nullptr); + merged_metrics->Set(attributes, def_agg->Merge(aggregation)); + } + return true; + }); } last_reported_metrics_[collector] = LastReportedMetrics{std::move(merged_metrics), collection_ts}; @@ -137,4 +140,4 @@ bool TemporalMetricStorage::buildMetrics(CollectorHandle *collector, } // namespace sdk OPENTELEMETRY_END_NAMESPACE -#endif \ No newline at end of file +#endif diff --git a/sdk/test/metrics/sync_metric_storage_test.cc b/sdk/test/metrics/sync_metric_storage_test.cc index 6c167c7a29..59e54dd60f 100644 --- a/sdk/test/metrics/sync_metric_storage_test.cc +++ b/sdk/test/metrics/sync_metric_storage_test.cc @@ -105,6 +105,30 @@ TEST_P(WritableMetricStorageTestFixture, LongSumAggregation) expected_total_put_requests = 0; } + // collect one more time. + collection_ts = std::chrono::system_clock::now(); + count_attributes = 0; + storage.Collect( + collector.get(), collectors, sdk_start_ts, collection_ts, [&](const MetricData data) { + for (auto data_attr : data.point_data_attr_) + { + auto data = opentelemetry::nostd::get(data_attr.point_data); + if (opentelemetry::nostd::get( + data_attr.attributes.find("RequestType")->second) == "GET") + { + EXPECT_EQ(opentelemetry::nostd::get(data.value_), expected_total_get_requests); + count_attributes++; + } + else if (opentelemetry::nostd::get( + data_attr.attributes.find("RequestType")->second) == "PUT") + { + EXPECT_EQ(opentelemetry::nostd::get(data.value_), expected_total_put_requests); + count_attributes++; + } + } + return true; + }); + storage.RecordLong(50l, KeyValueIterableView>(attributes_get), opentelemetry::context::Context{}); expected_total_get_requests += 50; @@ -214,6 +238,30 @@ TEST_P(WritableMetricStorageTestFixture, DoubleSumAggregation) expected_total_put_requests = 0; } + // collect one more time. + collection_ts = std::chrono::system_clock::now(); + count_attributes = 0; + storage.Collect( + collector.get(), collectors, sdk_start_ts, collection_ts, [&](const MetricData data) { + for (auto data_attr : data.point_data_attr_) + { + auto data = opentelemetry::nostd::get(data_attr.point_data); + if (opentelemetry::nostd::get( + data_attr.attributes.find("RequestType")->second) == "GET") + { + EXPECT_EQ(opentelemetry::nostd::get(data.value_), expected_total_get_requests); + count_attributes++; + } + else if (opentelemetry::nostd::get( + data_attr.attributes.find("RequestType")->second) == "PUT") + { + EXPECT_EQ(opentelemetry::nostd::get(data.value_), expected_total_put_requests); + count_attributes++; + } + } + return true; + }); + storage.RecordDouble(50.0, KeyValueIterableView>(attributes_get), opentelemetry::context::Context{}); From 4ea14c885b7c127b34fa6a7bae2d8c9c10214a33 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 6 Sep 2022 14:14:26 -0700 Subject: [PATCH 2/2] fix --- sdk/test/metrics/sync_metric_storage_test.cc | 22 +++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/sdk/test/metrics/sync_metric_storage_test.cc b/sdk/test/metrics/sync_metric_storage_test.cc index 59e54dd60f..6bfd09d181 100644 --- a/sdk/test/metrics/sync_metric_storage_test.cc +++ b/sdk/test/metrics/sync_metric_storage_test.cc @@ -96,7 +96,7 @@ TEST_P(WritableMetricStorageTestFixture, LongSumAggregation) } return true; }); - + EXPECT_EQ(count_attributes, 2); // GET and PUT // In case of delta temporarily, subsequent collection would contain new data points, so resetting // the counts if (temporality == AggregationTemporality::kDelta) @@ -116,18 +116,22 @@ TEST_P(WritableMetricStorageTestFixture, LongSumAggregation) if (opentelemetry::nostd::get( data_attr.attributes.find("RequestType")->second) == "GET") { - EXPECT_EQ(opentelemetry::nostd::get(data.value_), expected_total_get_requests); count_attributes++; + EXPECT_EQ(opentelemetry::nostd::get(data.value_), expected_total_get_requests); } else if (opentelemetry::nostd::get( data_attr.attributes.find("RequestType")->second) == "PUT") { - EXPECT_EQ(opentelemetry::nostd::get(data.value_), expected_total_put_requests); count_attributes++; + EXPECT_EQ(opentelemetry::nostd::get(data.value_), expected_total_put_requests); } } return true; }); + if (temporality == AggregationTemporality::kCumulative) + { + EXPECT_EQ(count_attributes, 2); // GET AND PUT + } storage.RecordLong(50l, KeyValueIterableView>(attributes_get), opentelemetry::context::Context{}); @@ -158,7 +162,9 @@ TEST_P(WritableMetricStorageTestFixture, LongSumAggregation) } return true; }); + EXPECT_EQ(count_attributes, 2); // GET and PUT } + INSTANTIATE_TEST_SUITE_P(WritableMetricStorageTestLong, WritableMetricStorageTestFixture, ::testing::Values(AggregationTemporality::kCumulative, @@ -229,6 +235,7 @@ TEST_P(WritableMetricStorageTestFixture, DoubleSumAggregation) } return true; }); + EXPECT_EQ(count_attributes, 2); // GET and PUT // In case of delta temporarily, subsequent collection would contain new data points, so resetting // the counts @@ -249,18 +256,22 @@ TEST_P(WritableMetricStorageTestFixture, DoubleSumAggregation) if (opentelemetry::nostd::get( data_attr.attributes.find("RequestType")->second) == "GET") { - EXPECT_EQ(opentelemetry::nostd::get(data.value_), expected_total_get_requests); count_attributes++; + EXPECT_EQ(opentelemetry::nostd::get(data.value_), expected_total_get_requests); } else if (opentelemetry::nostd::get( data_attr.attributes.find("RequestType")->second) == "PUT") { - EXPECT_EQ(opentelemetry::nostd::get(data.value_), expected_total_put_requests); count_attributes++; + EXPECT_EQ(opentelemetry::nostd::get(data.value_), expected_total_put_requests); } } return true; }); + if (temporality == AggregationTemporality::kCumulative) + { + EXPECT_EQ(count_attributes, 2); // GET AND PUT + } storage.RecordDouble(50.0, KeyValueIterableView>(attributes_get), @@ -293,6 +304,7 @@ TEST_P(WritableMetricStorageTestFixture, DoubleSumAggregation) } return true; }); + EXPECT_EQ(count_attributes, 2); // GET and PUT } INSTANTIATE_TEST_SUITE_P(WritableMetricStorageTestDouble, WritableMetricStorageTestFixture,