From 4a32aea4efda06a494018ddb1999b09f2a53edfe Mon Sep 17 00:00:00 2001 From: Snehil Chopra Date: Mon, 27 Jul 2020 23:09:54 +0000 Subject: [PATCH 1/4] Extended SetAttribute() method to include all integer types --- .../trace/gcp_exporter/internal/recordable.cc | 34 +++++++++++++++++-- .../gcp_exporter/internal/recordable_test.cc | 29 +++++++++++++--- exporters/trace/gcp_exporter/recordable.h | 22 ++++++++---- 3 files changed, 72 insertions(+), 13 deletions(-) diff --git a/exporters/trace/gcp_exporter/internal/recordable.cc b/exporters/trace/gcp_exporter/internal/recordable.cc index e70d182..9de8200 100644 --- a/exporters/trace/gcp_exporter/internal/recordable.cc +++ b/exporters/trace/gcp_exporter/internal/recordable.cc @@ -1,5 +1,4 @@ #include "exporters/trace/gcp_exporter/recordable.h" -#include OPENTELEMETRY_BEGIN_NAMESPACE @@ -34,7 +33,7 @@ void Recordable::SetIds(trace::TraceId trace_id, void Recordable::SetAttribute(nostd::string_view key, - const common::AttributeValue &&value) noexcept + const common::AttributeValue &value) noexcept { // Get the protobuf span's map auto map = span_.mutable_attributes()->mutable_attribute_map(); @@ -43,6 +42,22 @@ void Recordable::SetAttribute(nostd::string_view key, { (*map)[std::string(key)].set_bool_value(nostd::get(value)); } + else if (nostd::holds_alternative(value)) + { + (*map)[std::string(key)].set_int_value(nostd::get(value)); + } + else if (nostd::holds_alternative(value)) + { + (*map)[std::string(key)].set_int_value(nostd::get(value)); + } + else if (nostd::holds_alternative(value)) + { + (*map)[std::string(key)].set_int_value(nostd::get(value)); + } + else if (nostd::holds_alternative(value)) + { + (*map)[std::string(key)].set_int_value(nostd::get(value)); + } else if (nostd::holds_alternative(value)) { (*map)[std::string(key)].set_int_value(nostd::get(value)); @@ -56,9 +71,22 @@ void Recordable::SetAttribute(nostd::string_view key, } -void Recordable::AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) noexcept +void Recordable::AddEvent(nostd::string_view name, + core::SystemTimestamp timestamp, + const opentelemetry::trace::KeyValueIterable &attributes) noexcept { (void)name; + (void)timestamp; + (void)attributes; +} + + +void Recordable::AddLink( + opentelemetry::trace::SpanContext span_context, + const opentelemetry::trace::KeyValueIterable &attributes) noexcept +{ + (void)span_context; + (void)attributes; } diff --git a/exporters/trace/gcp_exporter/internal/recordable_test.cc b/exporters/trace/gcp_exporter/internal/recordable_test.cc index 84eac54..425e75b 100644 --- a/exporters/trace/gcp_exporter/internal/recordable_test.cc +++ b/exporters/trace/gcp_exporter/internal/recordable_test.cc @@ -16,12 +16,30 @@ TEST(Recordable, TestSetAttribute) const common::AttributeValue bool_value = true; rec.SetAttribute(bool_key, std::move(bool_value)); - // Set 'integer' type + // Set 'int' type const nostd::string_view int_key = "int_key"; - const int64_t seven = 7; - const common::AttributeValue int_value = seven; + const int seven_int = 7; + const common::AttributeValue int_value = seven_int; rec.SetAttribute(int_key, std::move(int_value)); + // Set 'int64_t' type + const nostd::string_view int64_t_key = "int64_t_key"; + const int64_t seven_int64_t = 7; + const common::AttributeValue int64_t_value = seven_int64_t; + rec.SetAttribute(int64_t_key, std::move(int64_t_value)); + + // Set 'uint64_t' type + const nostd::string_view uint64_t_key = "uint64_t_key"; + const uint64_t seven_uint64_t = 7; + const common::AttributeValue uint64_t_value = seven_uint64_t; + rec.SetAttribute(uint64_t_key, std::move(uint64_t_value)); + + // Set 'unsigned int' type + const nostd::string_view unsigned_int_key = "unsigned_int_key"; + const int64_t seven_unsigned_int = 7; + const common::AttributeValue unsigned_int_value = seven_unsigned_int; + rec.SetAttribute(unsigned_int_key, std::move(unsigned_int_value)); + // Set 'string' type const nostd::string_view string_key = "string_key"; const common::AttributeValue string_value = "test"; @@ -30,7 +48,10 @@ TEST(Recordable, TestSetAttribute) auto attr_map = rec.span().attributes().attribute_map(); EXPECT_TRUE(attr_map["bool_key"].bool_value()); - EXPECT_EQ(seven, attr_map["int_key"].int_value()); + EXPECT_EQ(seven_int, attr_map["int_key"].int_value()); + EXPECT_EQ(seven_uint64_t, attr_map["uint64_t_key"].int_value()); + EXPECT_EQ(seven_int64_t, attr_map["int64_t_key"].int_value()); + EXPECT_EQ(seven_unsigned_int, attr_map["unsigned_int_key"].int_value()); EXPECT_EQ("test", attr_map["string_key"].string_value().value()); } diff --git a/exporters/trace/gcp_exporter/recordable.h b/exporters/trace/gcp_exporter/recordable.h index 0b00c16..60cd57a 100644 --- a/exporters/trace/gcp_exporter/recordable.h +++ b/exporters/trace/gcp_exporter/recordable.h @@ -22,16 +22,26 @@ class Recordable final : public sdk::trace::Recordable public: const google::devtools::cloudtrace::v2::Span &span() const noexcept { return span_; } - void SetIds(trace::TraceId trace_id, - trace::SpanId span_id, - trace::SpanId parent_span_id) noexcept override; + void SetIds(opentelemetry::trace::TraceId trace_id, + opentelemetry::trace::SpanId span_id, + opentelemetry::trace::SpanId parent_span_id) noexcept override; void SetAttribute(nostd::string_view key, - const opentelemetry::common::AttributeValue &&value) noexcept override; + const opentelemetry::common::AttributeValue &value) noexcept override; - void AddEvent(nostd::string_view name, core::SystemTimestamp timestamp) noexcept override; + void AddEvent( + nostd::string_view name, + core::SystemTimestamp timestamp = core::SystemTimestamp(std::chrono::system_clock::now()), + const opentelemetry::trace::KeyValueIterable &attributes = + opentelemetry::trace::KeyValueIterableView>({})) noexcept override; - void SetStatus(trace::CanonicalCode code, nostd::string_view description) noexcept override; + void AddLink( + opentelemetry::trace::SpanContext span_context, + const opentelemetry::trace::KeyValueIterable &attributes = + opentelemetry::trace::KeyValueIterableView>({})) noexcept override; + + void SetStatus(opentelemetry::trace::CanonicalCode code, + nostd::string_view description) noexcept override; void SetName(nostd::string_view name) noexcept override; From 1f3796dc1d2b29a5ba7473aff9ff38047a9a2b06 Mon Sep 17 00:00:00 2001 From: Snehil Chopra Date: Tue, 28 Jul 2020 00:19:17 +0000 Subject: [PATCH 2/4] Addressed review comments --- .../trace/gcp_exporter/internal/recordable.cc | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/exporters/trace/gcp_exporter/internal/recordable.cc b/exporters/trace/gcp_exporter/internal/recordable.cc index 9de8200..c88d89a 100644 --- a/exporters/trace/gcp_exporter/internal/recordable.cc +++ b/exporters/trace/gcp_exporter/internal/recordable.cc @@ -40,33 +40,33 @@ void Recordable::SetAttribute(nostd::string_view key, if(nostd::holds_alternative(value)) { - (*map)[std::string(key)].set_bool_value(nostd::get(value)); + (*map)[key.data()].set_bool_value(nostd::get(value)); } else if (nostd::holds_alternative(value)) { - (*map)[std::string(key)].set_int_value(nostd::get(value)); + (*map)[key.data()].set_int_value(nostd::get(value)); } else if (nostd::holds_alternative(value)) { - (*map)[std::string(key)].set_int_value(nostd::get(value)); + (*map)[key.data()].set_int_value(nostd::get(value)); } else if (nostd::holds_alternative(value)) { - (*map)[std::string(key)].set_int_value(nostd::get(value)); + (*map)[key.data()].set_int_value(nostd::get(value)); } else if (nostd::holds_alternative(value)) { - (*map)[std::string(key)].set_int_value(nostd::get(value)); + (*map)[key.data()].set_int_value(nostd::get(value)); } else if (nostd::holds_alternative(value)) { - (*map)[std::string(key)].set_int_value(nostd::get(value)); + (*map)[key.data()].set_int_value(nostd::get(value)); } else if (nostd::holds_alternative(value)) { // TODO: Truncate string to 128 bytes std::string value_str = std::string(nostd::get(value)); - (*map)[std::string(key)].mutable_string_value()->set_value(value_str); + (*map)[key.data()].mutable_string_value()->set_value(value_str); } } From 6ccf2cc34568a44f9f5eee592135ef5cd86c84b2 Mon Sep 17 00:00:00 2001 From: Snehil Chopra Date: Tue, 4 Aug 2020 20:54:33 +0000 Subject: [PATCH 3/4] Changed to templated test for int types --- .../gcp_exporter/internal/recordable_test.cc | 53 +++++++++---------- exporters/trace/gcp_exporter/recordable.h | 8 ++- 2 files changed, 27 insertions(+), 34 deletions(-) diff --git a/exporters/trace/gcp_exporter/internal/recordable_test.cc b/exporters/trace/gcp_exporter/internal/recordable_test.cc index 425e75b..597c581 100644 --- a/exporters/trace/gcp_exporter/internal/recordable_test.cc +++ b/exporters/trace/gcp_exporter/internal/recordable_test.cc @@ -7,7 +7,7 @@ namespace exporter namespace gcp { -TEST(Recordable, TestSetAttribute) +TEST(Recordable, TestSetNonIntAttribute) { Recordable rec; @@ -16,30 +16,6 @@ TEST(Recordable, TestSetAttribute) const common::AttributeValue bool_value = true; rec.SetAttribute(bool_key, std::move(bool_value)); - // Set 'int' type - const nostd::string_view int_key = "int_key"; - const int seven_int = 7; - const common::AttributeValue int_value = seven_int; - rec.SetAttribute(int_key, std::move(int_value)); - - // Set 'int64_t' type - const nostd::string_view int64_t_key = "int64_t_key"; - const int64_t seven_int64_t = 7; - const common::AttributeValue int64_t_value = seven_int64_t; - rec.SetAttribute(int64_t_key, std::move(int64_t_value)); - - // Set 'uint64_t' type - const nostd::string_view uint64_t_key = "uint64_t_key"; - const uint64_t seven_uint64_t = 7; - const common::AttributeValue uint64_t_value = seven_uint64_t; - rec.SetAttribute(uint64_t_key, std::move(uint64_t_value)); - - // Set 'unsigned int' type - const nostd::string_view unsigned_int_key = "unsigned_int_key"; - const int64_t seven_unsigned_int = 7; - const common::AttributeValue unsigned_int_value = seven_unsigned_int; - rec.SetAttribute(unsigned_int_key, std::move(unsigned_int_value)); - // Set 'string' type const nostd::string_view string_key = "string_key"; const common::AttributeValue string_value = "test"; @@ -48,13 +24,32 @@ TEST(Recordable, TestSetAttribute) auto attr_map = rec.span().attributes().attribute_map(); EXPECT_TRUE(attr_map["bool_key"].bool_value()); - EXPECT_EQ(seven_int, attr_map["int_key"].int_value()); - EXPECT_EQ(seven_uint64_t, attr_map["uint64_t_key"].int_value()); - EXPECT_EQ(seven_int64_t, attr_map["int64_t_key"].int_value()); - EXPECT_EQ(seven_unsigned_int, attr_map["unsigned_int_key"].int_value()); EXPECT_EQ("test", attr_map["string_key"].string_value().value()); } +template +struct IntAttributeTest : public testing::Test +{ + using IntParamType = T; +}; + +using IntTypes = testing::Types; +TYPED_TEST_CASE(IntAttributeTest, IntTypes); + +TYPED_TEST(IntAttributeTest, SetIntSingleAttribute) +{ + using IntType = typename TestFixture::IntParamType; + IntType i = 2; + common::AttributeValue int_val(i); + + Recordable rec; + rec.SetAttribute("int_key", int_val); + + auto attr_map = rec.span().attributes().attribute_map(); + + EXPECT_EQ(nostd::get(int_val), attr_map["int_key"].int_value()); +} + TEST(Recordable, TestSetIds) { setenv("GOOGLE_CLOUD_PROJECT_ID", "test_project", 1); diff --git a/exporters/trace/gcp_exporter/recordable.h b/exporters/trace/gcp_exporter/recordable.h index 60cd57a..62954e0 100644 --- a/exporters/trace/gcp_exporter/recordable.h +++ b/exporters/trace/gcp_exporter/recordable.h @@ -31,14 +31,12 @@ class Recordable final : public sdk::trace::Recordable void AddEvent( nostd::string_view name, - core::SystemTimestamp timestamp = core::SystemTimestamp(std::chrono::system_clock::now()), - const opentelemetry::trace::KeyValueIterable &attributes = - opentelemetry::trace::KeyValueIterableView>({})) noexcept override; + core::SystemTimestamp timestamp, + const opentelemetry::trace::KeyValueIterable &attributes) noexcept override; void AddLink( opentelemetry::trace::SpanContext span_context, - const opentelemetry::trace::KeyValueIterable &attributes = - opentelemetry::trace::KeyValueIterableView>({})) noexcept override; + const opentelemetry::trace::KeyValueIterable &attributes) noexcept override; void SetStatus(opentelemetry::trace::CanonicalCode code, nostd::string_view description) noexcept override; From 26f70f02ee2a39b9994302a3e7dace9746982283 Mon Sep 17 00:00:00 2001 From: Snehil Chopra Date: Wed, 5 Aug 2020 18:00:07 +0000 Subject: [PATCH 4/4] Addressed reviews --- exporters/trace/gcp_exporter/internal/recordable.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exporters/trace/gcp_exporter/internal/recordable.cc b/exporters/trace/gcp_exporter/internal/recordable.cc index c88d89a..2b3dc13 100644 --- a/exporters/trace/gcp_exporter/internal/recordable.cc +++ b/exporters/trace/gcp_exporter/internal/recordable.cc @@ -36,7 +36,7 @@ void Recordable::SetAttribute(nostd::string_view key, const common::AttributeValue &value) noexcept { // Get the protobuf span's map - auto map = span_.mutable_attributes()->mutable_attribute_map(); + auto* map = span_.mutable_attributes()->mutable_attribute_map(); if(nostd::holds_alternative(value)) {