Skip to content

Commit

Permalink
Merge branch 'main' into fix-1
Browse files Browse the repository at this point in the history
  • Loading branch information
@marcalff
marcalff authored Sep 25, 2023
2 parents b034a35 + d49ba52 commit c49184f
Show file tree
Hide file tree
Showing 4 changed files with 109 additions and 48 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ Increment the:

* [DEPRECATION] Deprecate ZPAGES
[#2291](https://github.com/open-telemetry/opentelemetry-cpp/pull/2291)
* [EXPORTER] Remove explicit timestamps from metric points exported by Prometheus
[#2324](https://github.com/open-telemetry/opentelemetry-cpp/pull/2324)
* [EXPORTER] Handle attribute key collisions caused by sanitation
[#2324](https://github.com/open-telemetry/opentelemetry-cpp/pull/2326)

## [1.11.0] 2023-08-21

Expand Down
6 changes: 0 additions & 6 deletions exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ class PrometheusExporterUtils
static void SetData(std::vector<T> values,
const opentelemetry::sdk::metrics::PointAttributes &labels,
::prometheus::MetricType type,
std::chrono::nanoseconds time,
::prometheus::MetricFamily *metric_family);

/**
Expand All @@ -70,14 +69,12 @@ class PrometheusExporterUtils
const std::vector<double> &boundaries,
const std::vector<uint64_t> &counts,
const opentelemetry::sdk::metrics::PointAttributes &labels,
std::chrono::nanoseconds time,
::prometheus::MetricFamily *metric_family);

/**
* Set time and labels to metric data
*/
static void SetMetricBasic(::prometheus::ClientMetric &metric,
std::chrono::nanoseconds time,
const opentelemetry::sdk::metrics::PointAttributes &labels);

/**
Expand Down Expand Up @@ -107,9 +104,6 @@ class PrometheusExporterUtils
const std::vector<double> &boundaries,
const std::vector<uint64_t> &counts,
::prometheus::ClientMetric *metric);

// For testing
friend class SanitizeNameTester;
};
} // namespace metrics
} // namespace exporter
Expand Down
55 changes: 35 additions & 20 deletions exporters/prometheus/src/exporter_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ std::vector<prometheus_client::MetricFamily> PrometheusExporterUtils::TranslateT
prometheus_client::MetricFamily metric_family;
metric_family.name = sanitized + "_" + unit;
metric_family.help = metric_data.instrument_descriptor.description_;
auto time = metric_data.end_ts.time_since_epoch();
for (const auto &point_data_attr : metric_data.point_data_attr_)
{
auto kind = getAggregationType(point_data_attr.point_data);
Expand Down Expand Up @@ -72,7 +71,7 @@ std::vector<prometheus_client::MetricFamily> PrometheusExporterUtils::TranslateT
sum = nostd::get<int64_t>(histogram_point_data.sum_);
}
SetData(std::vector<double>{sum, (double)histogram_point_data.count_}, boundaries, counts,
point_data_attr.attributes, time, &metric_family);
point_data_attr.attributes, &metric_family);
}
else if (type == prometheus_client::MetricType::Gauge)
{
Expand All @@ -82,14 +81,14 @@ std::vector<prometheus_client::MetricFamily> PrometheusExporterUtils::TranslateT
auto last_value_point_data =
nostd::get<sdk::metrics::LastValuePointData>(point_data_attr.point_data);
std::vector<metric_sdk::ValueType> values{last_value_point_data.value_};
SetData(values, point_data_attr.attributes, type, time, &metric_family);
SetData(values, point_data_attr.attributes, type, &metric_family);
}
else if (nostd::holds_alternative<sdk::metrics::SumPointData>(point_data_attr.point_data))
{
auto sum_point_data =
nostd::get<sdk::metrics::SumPointData>(point_data_attr.point_data);
std::vector<metric_sdk::ValueType> values{sum_point_data.value_};
SetData(values, point_data_attr.attributes, type, time, &metric_family);
SetData(values, point_data_attr.attributes, type, &metric_family);
}
else
{
Expand All @@ -105,7 +104,7 @@ std::vector<prometheus_client::MetricFamily> PrometheusExporterUtils::TranslateT
auto sum_point_data =
nostd::get<sdk::metrics::SumPointData>(point_data_attr.point_data);
std::vector<metric_sdk::ValueType> values{sum_point_data.value_};
SetData(values, point_data_attr.attributes, type, time, &metric_family);
SetData(values, point_data_attr.attributes, type, &metric_family);
}
else
{
Expand Down Expand Up @@ -228,12 +227,11 @@ template <typename T>
void PrometheusExporterUtils::SetData(std::vector<T> values,
const metric_sdk::PointAttributes &labels,
prometheus_client::MetricType type,
std::chrono::nanoseconds time,
prometheus_client::MetricFamily *metric_family)
{
metric_family->metric.emplace_back();
prometheus_client::ClientMetric &metric = metric_family->metric.back();
SetMetricBasic(metric, time, labels);
SetMetricBasic(metric, labels);
SetValue(values, type, &metric);
}

Expand All @@ -246,34 +244,51 @@ void PrometheusExporterUtils::SetData(std::vector<T> values,
const std::vector<double> &boundaries,
const std::vector<uint64_t> &counts,
const metric_sdk::PointAttributes &labels,
std::chrono::nanoseconds time,
prometheus_client::MetricFamily *metric_family)
{
metric_family->metric.emplace_back();
prometheus_client::ClientMetric &metric = metric_family->metric.back();
SetMetricBasic(metric, time, labels);
SetMetricBasic(metric, labels);
SetValue(values, boundaries, counts, &metric);
}

/**
* Set time and labels to metric data
* Set labels to metric data
*/
void PrometheusExporterUtils::SetMetricBasic(prometheus_client::ClientMetric &metric,
std::chrono::nanoseconds time,
const metric_sdk::PointAttributes &labels)
{
metric.timestamp_ms = time.count() / 1000000;
if (labels.empty())
{
return;
}

// auto label_pairs = ParseLabel(labels);
if (!labels.empty())
// Concatenate values for keys that collide after sanitation.
// Note that attribute keys are sorted, but sanitized keys can be out-of-order.
// We could sort the sanitized keys again, but this seems too expensive to do
// in this hot code path. Instead, we ignore out-of-order keys and emit a warning.
metric.label.reserve(labels.size());
std::string previous_key;
for (auto const &label : labels)
{
metric.label.resize(labels.size());
size_t i = 0;
for (auto const &label : labels)
auto sanitized = SanitizeNames(label.first);
int comparison = previous_key.compare(sanitized);
if (metric.label.empty() || comparison < 0) // new key
{
previous_key = sanitized;
metric.label.push_back({sanitized, AttributeValueToString(label.second)});
}
else if (comparison == 0) // key collision after sanitation
{
metric.label.back().value += ";" + AttributeValueToString(label.second);
}
else // order inversion introduced by sanitation
{
auto sanitized = SanitizeNames(label.first);
metric.label[i].name = sanitized;
metric.label[i++].value = AttributeValueToString(label.second);
OTEL_INTERNAL_LOG_WARN(
"[Prometheus Exporter] SetMetricBase - "
"the sort order of labels has changed because of sanitization: '"
<< label.first << "' became '" << sanitized << "' which is less than '" << previous_key
<< "'. Ignoring this label.");
}
}
}
Expand Down
92 changes: 70 additions & 22 deletions exporters/prometheus/test/exporter_utils_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,6 @@ namespace prometheus_client = ::prometheus;

OPENTELEMETRY_BEGIN_NAMESPACE

namespace exporter
{
namespace metrics
{
class SanitizeNameTester
{
public:
static std::string sanitize(std::string name)
{
return PrometheusExporterUtils::SanitizeNames(name);
}
};
} // namespace metrics
} // namespace exporter

template <typename T>
void assert_basic(prometheus_client::MetricFamily &metric,
const std::string &sanitized_name,
Expand All @@ -42,6 +27,12 @@ void assert_basic(prometheus_client::MetricFamily &metric,
ASSERT_EQ(metric.help, description); // description not changed
ASSERT_EQ(metric.type, type); // type translated

// Prometheus metric data points should not have explicit timestamps
for (const prometheus::ClientMetric &cm : metric.metric)
{
ASSERT_EQ(cm.timestamp_ms, 0);
}

auto metric_data = metric.metric[0];
ASSERT_EQ(metric_data.label.size(), label_num);

Expand Down Expand Up @@ -147,14 +138,71 @@ TEST(PrometheusExporterUtils, TranslateToPrometheusHistogramNormal)
assert_histogram(metric, std::list<double>{10.1, 20.2, 30.2}, {200, 300, 400, 500});
}

TEST(PrometheusExporterUtils, SanitizeName)
class SanitizeNameTest : public ::testing::Test
{
Resource resource_ = Resource::Create({});
nostd::unique_ptr<InstrumentationScope> instrumentation_scope_ =
InstrumentationScope::Create("library_name", "1.2.0");

protected:
void CheckSanitation(const std::string &original, const std::string &sanitized)
{
metric_sdk::InstrumentDescriptor instrument_descriptor_{
original, "description", "unit", metric_sdk::InstrumentType::kCounter,
metric_sdk::InstrumentValueType::kDouble};
std::vector<prometheus::MetricFamily> result = PrometheusExporterUtils::TranslateToPrometheus(
{&resource_,
{{instrumentation_scope_.get(), {{instrument_descriptor_, {}, {}, {}, {{{}, {}}}}}}}});
EXPECT_EQ(result.begin()->name, sanitized + "_unit");
}
};

TEST_F(SanitizeNameTest, All)
{
CheckSanitation("name", "name");
CheckSanitation("name?", "name_");
CheckSanitation("name???", "name_");
CheckSanitation("name?__", "name_");
CheckSanitation("name?__name", "name_name");
CheckSanitation("name?__name:", "name_name:");
}

class AttributeCollisionTest : public ::testing::Test
{
Resource resource_ = Resource::Create(ResourceAttributes{});
nostd::unique_ptr<InstrumentationScope> instrumentation_scope_ =
InstrumentationScope::Create("library_name", "1.2.0");
metric_sdk::InstrumentDescriptor instrument_descriptor_{"library_name", "description", "unit",
metric_sdk::InstrumentType::kCounter,
metric_sdk::InstrumentValueType::kDouble};

protected:
void CheckTranslation(const metric_sdk::PointAttributes &attrs,
const std::vector<prometheus::ClientMetric::Label> &expected)
{
std::vector<prometheus::MetricFamily> result = PrometheusExporterUtils::TranslateToPrometheus(
{&resource_,
{{instrumentation_scope_.get(), {{instrument_descriptor_, {}, {}, {}, {{attrs, {}}}}}}}});
EXPECT_EQ(result.begin()->metric.begin()->label, expected);
}
};

TEST_F(AttributeCollisionTest, SeparatesDistinctKeys)
{
CheckTranslation({{"foo.a", "value1"}, {"foo.b", "value2"}},
{{"foo_a", "value1"}, {"foo_b", "value2"}});
}

TEST_F(AttributeCollisionTest, JoinsCollidingKeys)
{
CheckTranslation({{"foo.a", "value1"}, {"foo_a", "value2"}}, //
{{"foo_a", "value1;value2"}});
}

TEST_F(AttributeCollisionTest, DropsInvertedKeys)
{
ASSERT_EQ(exporter::metrics::SanitizeNameTester::sanitize("name"), "name");
ASSERT_EQ(exporter::metrics::SanitizeNameTester::sanitize("name?"), "name_");
ASSERT_EQ(exporter::metrics::SanitizeNameTester::sanitize("name???"), "name_");
ASSERT_EQ(exporter::metrics::SanitizeNameTester::sanitize("name?__"), "name_");
ASSERT_EQ(exporter::metrics::SanitizeNameTester::sanitize("name?__name"), "name_name");
ASSERT_EQ(exporter::metrics::SanitizeNameTester::sanitize("name?__name:"), "name_name:");
CheckTranslation({{"foo.a", "value1"}, {"foo.b", "value2"}, {"foo__a", "value3"}},
{{"foo_a", "value1"}, {"foo_b", "value2"}});
}

OPENTELEMETRY_END_NAMESPACE

0 comments on commit c49184f

Please sign in to comment.