Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metrics SDK: Filtering metrics attributes #1191

Merged
merged 19 commits into from
Feb 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions sdk/include/opentelemetry/sdk/common/attribute_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

#pragma once

#include <map>
#include <string>
#include <unordered_map>
#include <vector>
Expand Down Expand Up @@ -143,6 +144,56 @@ class AttributeMap : public std::unordered_map<std::string, OwnedAttributeValue>
private:
AttributeConverter converter_;
};

/**
* Class for storing attributes.
*/
class OrderedAttributeMap : public std::map<std::string, OwnedAttributeValue>
{
public:
// Contruct empty attribute map
OrderedAttributeMap() : std::map<std::string, OwnedAttributeValue>(){};

// Contruct attribute map and populate with attributes
OrderedAttributeMap(const opentelemetry::common::KeyValueIterable &attributes)
: OrderedAttributeMap()
{
attributes.ForEachKeyValue(
[&](nostd::string_view key, opentelemetry::common::AttributeValue value) noexcept {
SetAttribute(key, value);
return true;
});
}

// Construct map from initializer list by applying `SetAttribute` transform for every attribute
OrderedAttributeMap(
std::initializer_list<std::pair<nostd::string_view, opentelemetry::common::AttributeValue>>
attributes)
: OrderedAttributeMap()
{
for (auto &kv : attributes)
{
SetAttribute(kv.first, kv.second);
}
}

// Returns a reference to this map
const std::map<std::string, OwnedAttributeValue> &GetAttributes() const noexcept
{
return (*this);
}

// Convert non-owning key-value to owning std::string(key) and OwnedAttributeValue(value)
void SetAttribute(nostd::string_view key,
const opentelemetry::common::AttributeValue &value) noexcept
{
(*this)[std::string(key)] = nostd::visit(converter_, value);
}

private:
AttributeConverter converter_;
};

} // namespace common
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
62 changes: 62 additions & 0 deletions sdk/include/opentelemetry/sdk/common/attributemap_hash.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

#pragma once

#include <iostream>
#include <string>
#include "opentelemetry/sdk/common/attribute_utils.h"

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace common
{

template <class T>
inline void GetHashForAttributeValue(size_t &seed, const T arg)
{
std::hash<T> hasher;
// reference -
// https://www.boost.org/doc/libs/1_37_0/doc/html/hash/reference.html#boost.hash_combine
seed ^= hasher(arg) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
}

template <class T>
inline void GetHashForAttributeValue(size_t &seed, const std::vector<T> &arg)
{
for (auto v : arg)
{
GetHashForAttributeValue<T>(seed, v);
}
}

struct GetHashForAttributeValueVisitor
{
GetHashForAttributeValueVisitor(size_t &seed) : seed_(seed) {}
template <class T>
void operator()(T &v)
{
GetHashForAttributeValue(seed_, v);
}
size_t &seed_;
};

// Calculate hash of keys and values of attribute map
inline size_t GetHashForAttributeMap(const OrderedAttributeMap &attribute_map)
{
size_t seed = 0UL;
for (auto &kv : attribute_map)
{
std::hash<std::string> hasher;
// reference -
// https://www.boost.org/doc/libs/1_37_0/doc/html/hash/reference.html#boost.hash_combine
seed ^= hasher(kv.first) + 0x9e3779b9 + (seed << 6) + (seed >> 2);
nostd::visit(GetHashForAttributeValueVisitor(seed), kv.second);
}
return seed;
}

} // namespace common
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,27 @@ namespace sdk
{
namespace metrics
{
using MetricAttributes = opentelemetry::sdk::common::AttributeMap;
using MetricAttributes = opentelemetry::sdk::common::OrderedAttributeMap;

/**
* The AttributesProcessor is responsible for customizing which
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this can be made internal/private to the SDK for now.

The specification only allows very limited manipulation on attributes (e.g. remove certain attributes from the measurement), this generic solution could work but it comes with perf cost (e.g. the virtual AttributesProcessor::process call).

Doesn't have to be a blocker though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I do suspect some perf cost associated with virtual and attributes copy. I have added a benchmark test to get cost statistics and would be helpful if have to consider performance improvement.

Also, I realized that we need to calculate the hash on the attribute map to store metrics data. Have added functionality to calculate the hash based on std::hash and boost::hash_combine`. As hash should be the same irrespective of the order of key/values in the AttributeMap, I have changed the internal storage for AttributeMap from unordered_map to map. This way we can avoid sorting of keys for every measurement.

Let me see how can we make this class internal/private to SDK, as C++ doesn't have a direct way of doing it. We can make AttributeProcessor::process() as private, and other SDK classes as its friend to it but would like to see a better approach if possible.

* attribute(s) are to be reported as metrics dimension(s).
*/

class AttributesProcessor
{
public:
// Process the metric instrument attributes.
// @returns The processed attributes
virtual MetricAttributes process(
const opentelemetry::common::KeyValueIterable &attributes) noexcept = 0;
};

/**
* DefaultAttributesProcessor returns copy of input instrument attributes without
* any modification.
*/

class DefaultAttributesProcessor : public AttributesProcessor
{
MetricAttributes process(
Expand All @@ -28,6 +40,39 @@ class DefaultAttributesProcessor : public AttributesProcessor
}
};

/**
* FilteringAttributesProcessor filters by allowed attribute names and drops any names
* that are not in the allow list.
*/

class FilteringAttributesProcessor : public AttributesProcessor
{
public:
FilteringAttributesProcessor(
const std::unordered_map<std::string, bool> allowed_attribute_keys = {})
: allowed_attribute_keys_(std::move(allowed_attribute_keys))
{}

MetricAttributes process(
const opentelemetry::common::KeyValueIterable &attributes) noexcept override
{
MetricAttributes result;
attributes.ForEachKeyValue(
[&](nostd::string_view key, opentelemetry::common::AttributeValue value) noexcept {
if (allowed_attribute_keys_.find(key.data()) != allowed_attribute_keys_.end())
{
result.SetAttribute(key, value);
return true;
}
return true;
});
return result;
}

private:
std::unordered_map<std::string, bool> allowed_attribute_keys_;
};

} // namespace metrics
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
Expand Down
23 changes: 23 additions & 0 deletions sdk/test/common/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -127,3 +127,26 @@ cc_test(
"@com_google_googletest//:gtest_main",
],
)

cc_test(
name = "attributemap_hash_test",
srcs = [
"attributemap_hash_test.cc",
],
tags = ["test"],
deps = [
"//api",
"//sdk:headers",
"@com_google_googletest//:gtest_main",
],
)

otel_cc_benchmark(
name = "attributemap_hash_benchmark",
srcs = ["attributemap_hash_benchmark.cc"],
tags = ["test"],
deps = [
"//api",
"//sdk:headers",
],
)
5 changes: 5 additions & 0 deletions sdk/test/common/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ foreach(
circular_buffer_range_test
circular_buffer_test
attribute_utils_test
attributemap_hash_test
global_log_handle_test)

add_executable(${testname} "${testname}.cc")
Expand All @@ -30,3 +31,7 @@ target_link_libraries(random_benchmark benchmark::benchmark
add_executable(circular_buffer_benchmark circular_buffer_benchmark.cc)
target_link_libraries(circular_buffer_benchmark benchmark::benchmark
${CMAKE_THREAD_LIBS_INIT} opentelemetry_api)

add_executable(attributemap_hash_benchmark attributemap_hash_benchmark.cc)
target_link_libraries(attributemap_hash_benchmark benchmark::benchmark
${CMAKE_THREAD_LIBS_INIT} opentelemetry_common)
32 changes: 28 additions & 4 deletions sdk/test/common/attribute_utils_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,15 @@

TEST(AttributeMapTest, DefaultConstruction)
{
opentelemetry::sdk::common::AttributeMap map;
EXPECT_EQ(map.GetAttributes().size(), 0);

opentelemetry::sdk::common::AttributeMap attribute_map;
EXPECT_EQ(attribute_map.GetAttributes().size(), 0);
}

TEST(OrderedAttributeMapTest, DefaultConstruction)
{
opentelemetry::sdk::common::OrderedAttributeMap attribute_map;
EXPECT_EQ(attribute_map.GetAttributes().size(), 0);
}

TEST(AttributeMapTest, AttributesConstruction)
Expand All @@ -20,10 +27,27 @@ TEST(AttributeMapTest, AttributesConstruction)
{keys[0], values[0]}, {keys[1], values[1]}, {keys[2], values[2]}};

opentelemetry::common::KeyValueIterableView<std::map<std::string, int>> iterable(attributes);
opentelemetry::sdk::common::AttributeMap map(iterable);
opentelemetry::sdk::common::AttributeMap attribute_map(iterable);

for (int i = 0; i < kNumAttributes; i++)
{
EXPECT_EQ(opentelemetry::nostd::get<int>(attribute_map.GetAttributes().at(keys[i])), values[i]);
}
}

TEST(OrderedAttributeMapTest, AttributesConstruction)
{
const int kNumAttributes = 3;
std::string keys[kNumAttributes] = {"attr1", "attr2", "attr3"};
int values[kNumAttributes] = {15, 24, 37};
std::map<std::string, int> attributes = {
{keys[0], values[0]}, {keys[1], values[1]}, {keys[2], values[2]}};

opentelemetry::common::KeyValueIterableView<std::map<std::string, int>> iterable(attributes);
opentelemetry::sdk::common::OrderedAttributeMap attribute_map(iterable);

for (int i = 0; i < kNumAttributes; i++)
{
EXPECT_EQ(opentelemetry::nostd::get<int>(map.GetAttributes().at(keys[i])), values[i]);
EXPECT_EQ(opentelemetry::nostd::get<int>(attribute_map.GetAttributes().at(keys[i])), values[i]);
}
}
22 changes: 22 additions & 0 deletions sdk/test/common/attributemap_hash_benchmark.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

#include <benchmark/benchmark.h>
#include "opentelemetry/sdk/common/attributemap_hash.h"

using namespace opentelemetry::sdk::common;
namespace
{
void BM_AttributeMapHash(benchmark::State &state)
{
OrderedAttributeMap map1 = {{"k1", "v1"}, {"k2", "v2"}, {"k3", "v3"}, {"k4", "v4"},
{"k5", true}, {"k6", 12}, {"k7", 12.209}};
while (state.KeepRunning())
{
benchmark::DoNotOptimize(GetHashForAttributeMap(map1));
}
}
BENCHMARK(BM_AttributeMapHash);

} // namespace
BENCHMARK_MAIN();
32 changes: 32 additions & 0 deletions sdk/test/common/attributemap_hash_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

#include "opentelemetry/sdk/common/attributemap_hash.h"
#include <gtest/gtest.h>

using namespace opentelemetry::sdk::common;
TEST(AttributeMapHashTest, BasicTests)
{
{
OrderedAttributeMap map1 = {{"k1", "v1"}, {"k2", "v2"}, {"k3", "v3"}};
OrderedAttributeMap map2 = {{"k1", "v1"}, {"k2", "v2"}, {"k3", "v3"}, {"k4", "v4"}};
OrderedAttributeMap map3 = {{"k3", "v3"}, {"k1", "v1"}, {"k2", "v2"}};

EXPECT_TRUE(GetHashForAttributeMap(map1) != 0);
EXPECT_TRUE(GetHashForAttributeMap(map1) == GetHashForAttributeMap(map1));
EXPECT_TRUE(GetHashForAttributeMap(map1) != GetHashForAttributeMap(map2));
EXPECT_TRUE(GetHashForAttributeMap(map1) == GetHashForAttributeMap(map3));
}

{
OrderedAttributeMap map1 = {{"k1", 10}, {"k2", true}, {"k3", 12.22}};
OrderedAttributeMap map2 = {{"k3", 12.22}, {"k1", 10}, {"k2", true}};
EXPECT_TRUE(GetHashForAttributeMap(map1) == GetHashForAttributeMap(map2));
EXPECT_TRUE(GetHashForAttributeMap(map1) != 0);
}

{
OrderedAttributeMap map1 = {};
EXPECT_TRUE(GetHashForAttributeMap(map1) == 0);
}
}
35 changes: 35 additions & 0 deletions sdk/test/metrics/BUILD
Original file line number Diff line number Diff line change
@@ -1,11 +1,46 @@
load("//bazel:otel_cc_benchmark.bzl", "otel_cc_benchmark")

cc_test(
name = "meter_provider_sdk_test",
srcs = [
"meter_provider_sdk_test.cc",
],
tags = [
"metrics",
"test",
],
deps = [
"//sdk/src/metrics",
"//sdk/src/resource",
"@com_google_googletest//:gtest_main",
],
)

cc_test(
name = "attributes_processor_test",
srcs = [
"attributes_processor_test.cc",
],
tags = [
"metrics",
"test",
],
deps = [
"//sdk/src/metrics",
"@com_google_googletest//:gtest_main",
],
)

otel_cc_benchmark(
name = "attributes_processor_benchmark",
srcs = [
"attributes_processor_benchmark.cc",
],
tags = [
"metrics",
"test",
],
deps = [
"//sdk/src/metrics",
],
)
Loading