-
Notifications
You must be signed in to change notification settings - Fork 500
Implementing configurable aggregation cardinality limit #3624
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
Changes from 22 commits
f3c38a3
0d80b39
491ef3c
671e4ca
203fd46
3af667c
277f5ee
db29ab1
14aa407
5f153a2
b446e6f
9b34954
5186013
969ad85
8a5b7a1
34f1d13
6c91fbb
2c9305c
100959c
f6719f2
d0c4b95
d4aaf6a
c7b4a64
f3ea8f0
d8d4fdd
fcebda2
121d6c6
9c6fb5e
f81f8bd
93b9914
274614c
7dde674
dc5661c
df2c451
49eac75
02fc44e
e29dc0f
a42ae43
24ed437
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,9 @@ | |
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
#include <gtest/gtest.h> | ||
#include <stddef.h> | ||
#include <initializer_list> | ||
#include <map> | ||
#include <string> | ||
#include <unordered_map> | ||
#include <utility> | ||
|
@@ -26,6 +28,7 @@ | |
#include "opentelemetry/sdk/metrics/meter_provider.h" | ||
#include "opentelemetry/sdk/metrics/metric_reader.h" | ||
#include "opentelemetry/sdk/metrics/push_metric_exporter.h" | ||
#include "opentelemetry/sdk/metrics/state/attributes_hashmap.h" | ||
#include "opentelemetry/sdk/metrics/view/attributes_processor.h" | ||
#include "opentelemetry/sdk/metrics/view/instrument_selector.h" | ||
#include "opentelemetry/sdk/metrics/view/meter_selector.h" | ||
|
@@ -142,6 +145,85 @@ TEST(HistogramToSumFilterAttributes, Double) | |
}); | ||
} | ||
|
||
TEST(HistogramToSumFilterAttributesWithCardinaityLimit, Double) | ||
ThomsonTan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
{ | ||
MeterProvider mp; | ||
auto m = mp.GetMeter("meter1", "version1", "schema1"); | ||
std::string instrument_unit = "ms"; | ||
std::string instrument_name = "historgram1"; | ||
ThomsonTan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
std::string instrument_desc = "histogram metrics"; | ||
size_t cardinality_limit = 10000; | ||
|
||
std::unordered_map<std::string, bool> allowedattr; | ||
allowedattr["attr1"] = true; | ||
std::unique_ptr<opentelemetry::sdk::metrics::AttributesProcessor> attrproc{ | ||
new opentelemetry::sdk::metrics::FilteringAttributesProcessor(allowedattr)}; | ||
|
||
std::shared_ptr<opentelemetry::sdk::metrics::AggregationConfig> dummy_aggregation_config{ | ||
new opentelemetry::sdk::metrics::AggregationConfig(cardinality_limit)}; | ||
std::unique_ptr<MockMetricExporter> exporter(new MockMetricExporter()); | ||
std::shared_ptr<MetricReader> reader{new MockMetricReader(std::move(exporter))}; | ||
mp.AddMetricReader(reader); | ||
|
||
std::unique_ptr<View> view{new View("view1", "view1_description", AggregationType::kSum, | ||
dummy_aggregation_config, std::move(attrproc))}; | ||
std::unique_ptr<InstrumentSelector> instrument_selector{ | ||
new InstrumentSelector(InstrumentType::kHistogram, instrument_name, instrument_unit)}; | ||
std::unique_ptr<MeterSelector> meter_selector{new MeterSelector("meter1", "version1", "schema1")}; | ||
mp.AddView(std::move(instrument_selector), std::move(meter_selector), std::move(view)); | ||
|
||
auto h = m->CreateDoubleHistogram(instrument_name, instrument_desc, instrument_unit); | ||
size_t total_metrics_times = 5; | ||
|
||
size_t agg_repeat_count = 5; | ||
for (size_t repeat = 0; repeat < agg_repeat_count; repeat++) | ||
{ | ||
|
||
for (size_t times = 0; times < total_metrics_times; times++) | ||
{ | ||
for (size_t i = 0; i < 2 * cardinality_limit; i++) | ||
{ | ||
std::unordered_map<std::string, std::string> attr = {{"attr1", std::to_string(i)}, | ||
{"attr2", "val2"}}; | ||
h->Record(1, attr, opentelemetry::context::Context{}); | ||
} | ||
} | ||
|
||
reader->Collect([&](ResourceMetrics &rm) { | ||
for (const ScopeMetrics &smd : rm.scope_metric_data_) | ||
{ | ||
for (const MetricData &md : smd.metric_data_) | ||
{ | ||
// Something weird about attributes hashmap. If cardinality is setup to n, it emits n-1 | ||
// including overflow. Just making the logic generic here to succeed for n or n-1 total | ||
// cardinality. | ||
ThomsonTan marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be contradicting the earlier comment
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, n-1 is possible for now which is a bug in the metrics SDK, filed an issue (see the other reply) and will fix it in separate PR. |
||
EXPECT_GE(cardinality_limit, md.point_data_attr_.size()); | ||
EXPECT_LT(cardinality_limit / 2, md.point_data_attr_.size()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this range check done. can't we get exact ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems there is an issue when the metric collection process which could return 1 less aggregated metric than the configured cardinality. Filed below issue to track it. |
||
for (size_t i = 0; i < md.point_data_attr_.size(); i++) | ||
{ | ||
EXPECT_EQ(1, md.point_data_attr_[i].attributes.size()); | ||
if (md.point_data_attr_[i].attributes.end() != | ||
md.point_data_attr_[i].attributes.find("attr1")) | ||
{ | ||
EXPECT_EQ(total_metrics_times * (repeat + 1), | ||
opentelemetry::nostd::get<double>(opentelemetry::nostd::get<SumPointData>( | ||
md.point_data_attr_[i].point_data) | ||
.value_)); | ||
} | ||
else | ||
{ | ||
EXPECT_NE(md.point_data_attr_[i].attributes.end(), | ||
md.point_data_attr_[i].attributes.find( | ||
sdk::metrics::kAttributesLimitOverflowKey)); | ||
} | ||
} | ||
} | ||
} | ||
return true; | ||
}); | ||
} | ||
} | ||
|
||
TEST(CounterToSum, Double) | ||
{ | ||
MeterProvider mp; | ||
|
@@ -244,6 +326,85 @@ TEST(CounterToSumFilterAttributes, Double) | |
}); | ||
} | ||
|
||
TEST(CounterToSumFilterAttributesWithCardinalityLimit, Double) | ||
{ | ||
MeterProvider mp; | ||
auto m = mp.GetMeter("meter1", "version1", "schema1"); | ||
std::string instrument_unit = "ms"; | ||
std::string instrument_name = "counter1"; | ||
std::string instrument_desc = "counter metrics"; | ||
size_t cardinality_limit = 10000; | ||
|
||
std::unordered_map<std::string, bool> allowedattr; | ||
allowedattr["attr1"] = true; | ||
std::unique_ptr<opentelemetry::sdk::metrics::AttributesProcessor> attrproc{ | ||
new opentelemetry::sdk::metrics::FilteringAttributesProcessor(allowedattr)}; | ||
|
||
std::shared_ptr<opentelemetry::sdk::metrics::AggregationConfig> dummy_aggregation_config{ | ||
new opentelemetry::sdk::metrics::AggregationConfig(cardinality_limit)}; | ||
std::unique_ptr<MockMetricExporter> exporter(new MockMetricExporter()); | ||
std::shared_ptr<MetricReader> reader{new MockMetricReader(std::move(exporter))}; | ||
mp.AddMetricReader(reader); | ||
|
||
std::unique_ptr<View> view{new View("view1", "view1_description", AggregationType::kSum, | ||
dummy_aggregation_config, std::move(attrproc))}; | ||
std::unique_ptr<InstrumentSelector> instrument_selector{ | ||
new InstrumentSelector(InstrumentType::kCounter, instrument_name, instrument_unit)}; | ||
std::unique_ptr<MeterSelector> meter_selector{new MeterSelector("meter1", "version1", "schema1")}; | ||
mp.AddView(std::move(instrument_selector), std::move(meter_selector), std::move(view)); | ||
|
||
auto c = m->CreateDoubleCounter(instrument_name, instrument_desc, instrument_unit); | ||
|
||
size_t agg_repeat_count = 5; | ||
for (size_t repeat = 0; repeat < agg_repeat_count; repeat++) | ||
{ | ||
size_t total_metrics_times = 5; | ||
|
||
for (size_t times = 0; times < total_metrics_times; times++) | ||
{ | ||
for (size_t i = 0; i < 2 * cardinality_limit; i++) | ||
{ | ||
std::unordered_map<std::string, std::string> attr = {{"attr1", std::to_string(i)}, | ||
{"attr2", "val2"}}; | ||
c->Add(1, attr, opentelemetry::context::Context{}); | ||
} | ||
} | ||
|
||
reader->Collect([&](ResourceMetrics &rm) { | ||
for (const ScopeMetrics &smd : rm.scope_metric_data_) | ||
{ | ||
for (const MetricData &md : smd.metric_data_) | ||
{ | ||
// Something weird about attributes hashmap. If cardinality is setup to n, it emits n-1 | ||
// including overflow. Just making the logic generic here to succeed for n or n-1 total | ||
// cardinality. | ||
ThomsonTan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
EXPECT_GE(cardinality_limit, md.point_data_attr_.size()); | ||
EXPECT_LT(cardinality_limit / 2, md.point_data_attr_.size()); | ||
for (size_t i = 0; i < md.point_data_attr_.size(); i++) | ||
{ | ||
EXPECT_EQ(1, md.point_data_attr_[i].attributes.size()); | ||
if (md.point_data_attr_[i].attributes.find("attr1") != | ||
md.point_data_attr_[i].attributes.end()) | ||
{ | ||
EXPECT_EQ(total_metrics_times * (repeat + 1), | ||
opentelemetry::nostd::get<double>(opentelemetry::nostd::get<SumPointData>( | ||
md.point_data_attr_[i].point_data) | ||
.value_)); | ||
} | ||
else | ||
{ | ||
EXPECT_NE(md.point_data_attr_[i].attributes.end(), | ||
md.point_data_attr_[i].attributes.find( | ||
sdk::metrics::kAttributesLimitOverflowKey)); | ||
} | ||
} | ||
} | ||
} | ||
return true; | ||
}); | ||
} | ||
} | ||
|
||
class UpDownCounterToSumFixture : public ::testing::TestWithParam<bool> | ||
{}; | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.