review comments

open-telemetry · Feb 16, 2022 · 96a1ef5 · 96a1ef5
1 parent 20d5440
commit 96a1ef5
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 14 deletions.
diff --git a/sdk/include/opentelemetry/sdk/metrics/instruments.h b/sdk/include/opentelemetry/sdk/metrics/instruments.h
@@ -54,7 +54,7 @@ struct InstrumentDescriptor
 
 using MetricAttributes = opentelemetry::sdk::common::OrderedAttributeMap;
 
-// TBD -> Once MetricCollector is imoplemeted
+// TBD -> Remove once MetricCollector is imoplemeted
 class MetricCollector
 {
 public:

diff --git a/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h b/sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h
@@ -57,7 +57,6 @@ class AttributesHashMap
    * If not present, it uses the provided callback to generate
    * value and store in the hash
    */
-
   Aggregation *GetOrSetDefault(const MetricAttributes &attributes,
                                std::function<std::unique_ptr<Aggregation>()> aggregation_callback)
   {
@@ -75,20 +74,15 @@ class AttributesHashMap
   /**
    * Set the value for given key, overwriting the value if already present
    */
-
   void Set(const MetricAttributes &attributes, std::unique_ptr<Aggregation> value)
   {
     std::lock_guard<opentelemetry::common::SpinLockMutex> guard(GetLock());
-    if (hash_map_.find(attributes) == hash_map_.end())
-    {
-      hash_map_[attributes] = std::move(value);
-    }
+    hash_map_[attributes] = std::move(value);
   }
 
   /**
    * Iterate the hash to yield key and value stored in hash.
    */
-
   bool GetAllEnteries(
       nostd::function_ref<bool(const MetricAttributes &, Aggregation &)> callback) const
   {
@@ -106,7 +100,6 @@ class AttributesHashMap
   /**
    * Return the size of hash.
    */
-
   size_t Size()
   {
     std::lock_guard<opentelemetry::common::SpinLockMutex> guard(GetLock());

diff --git a/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h b/sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h
@@ -13,6 +13,8 @@
 #  include "opentelemetry/sdk/metrics/view/view.h"
 #  include "opentelemetry/sdk/resource/resource.h"
 
+#  include <memory>
+
 OPENTELEMETRY_BEGIN_NAMESPACE
 namespace sdk
 {
@@ -26,10 +28,10 @@ class SyncMetricStorage : public MetricStorage, public WritableMetricStorage
   SyncMetricStorage(InstrumentDescriptor instrument_descriptor,
                     const AggregationType aggregation_type,
                     AttributesProcessor *attributes_processor = new DefaultAttributesProcessor())
-      : aggregation_type_{aggregation_type},
-        instrument_descriptor_(instrument_descriptor),
-        attributes_processor_{attributes_processor},
-        attributes_hashmap_(std::move(std::unique_ptr<AttributesHashMap>(new AttributesHashMap)))
+      : instrument_descriptor_(instrument_descriptor),
+        aggregation_type_{aggregation_type},
+        attributes_hashmap_(new AttributesHashMap()),
+        attributes_processor_{attributes_processor}
   {
     create_default_aggregation_ = [&]() -> std::unique_ptr<Aggregation> {
       return std::move(this->create_aggregation());
@@ -102,8 +104,8 @@ class SyncMetricStorage : public MetricStorage, public WritableMetricStorage
   InstrumentDescriptor instrument_descriptor_;
   AggregationType aggregation_type_;
   std::unique_ptr<AttributesHashMap> attributes_hashmap_;
-  std::function<std::unique_ptr<Aggregation>()> create_default_aggregation_;
   AttributesProcessor *attributes_processor_;
+  std::function<std::unique_ptr<Aggregation>()> create_default_aggregation_;
 
   std::unique_ptr<Aggregation> create_aggregation()
   {

diff --git a/sdk/test/metrics/BUILD b/sdk/test/metrics/BUILD
@@ -79,6 +79,21 @@ cc_test(
     ],
 )
 
+cc_test(
+    name = "attributes_hashmap_test",
+    srcs = [
+        "attributes_hashmap_test.cc",
+    ],
+    tags = [
+        "metrics",
+        "test",
+    ],
+    deps = [
+        "//sdk/src/metrics",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
 otel_cc_benchmark(
     name = "attributes_processor_benchmark",
     srcs = [
@@ -92,3 +107,17 @@ otel_cc_benchmark(
         "//sdk/src/metrics",
     ],
 )
+
+otel_cc_benchmark(
+    name = "attributes_hashmap_benchmark",
+    srcs = [
+        "attributes_hashmap_benchmark.cc",
+    ],
+    tags = [
+        "metrics",
+        "test",
+    ],
+    deps = [
+        "//sdk/src/metrics",
+    ],
+)