diff --git a/CHANGELOG.md b/CHANGELOG.md index f09553ce70..38a077e7d8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h b/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h index ce7f0a1191..312b641c3e 100644 --- a/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h +++ b/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h @@ -58,7 +58,6 @@ class PrometheusExporterUtils static void SetData(std::vector values, const opentelemetry::sdk::metrics::PointAttributes &labels, ::prometheus::MetricType type, - std::chrono::nanoseconds time, ::prometheus::MetricFamily *metric_family); /** @@ -70,14 +69,12 @@ class PrometheusExporterUtils const std::vector &boundaries, const std::vector &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); /** @@ -107,9 +104,6 @@ class PrometheusExporterUtils const std::vector &boundaries, const std::vector &counts, ::prometheus::ClientMetric *metric); - - // For testing - friend class SanitizeNameTester; }; } // namespace metrics } // namespace exporter diff --git a/exporters/prometheus/src/exporter_utils.cc b/exporters/prometheus/src/exporter_utils.cc index 966d665df6..149d93290b 100644 --- a/exporters/prometheus/src/exporter_utils.cc +++ b/exporters/prometheus/src/exporter_utils.cc @@ -44,7 +44,6 @@ std::vector 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); @@ -72,7 +71,7 @@ std::vector PrometheusExporterUtils::TranslateT sum = nostd::get(histogram_point_data.sum_); } SetData(std::vector{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) { @@ -82,14 +81,14 @@ std::vector PrometheusExporterUtils::TranslateT auto last_value_point_data = nostd::get(point_data_attr.point_data); std::vector 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(point_data_attr.point_data)) { auto sum_point_data = nostd::get(point_data_attr.point_data); std::vector 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 { @@ -105,7 +104,7 @@ std::vector PrometheusExporterUtils::TranslateT auto sum_point_data = nostd::get(point_data_attr.point_data); std::vector 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 { @@ -228,12 +227,11 @@ template void PrometheusExporterUtils::SetData(std::vector 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); } @@ -246,34 +244,51 @@ void PrometheusExporterUtils::SetData(std::vector values, const std::vector &boundaries, const std::vector &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."); } } } diff --git a/exporters/prometheus/test/exporter_utils_test.cc b/exporters/prometheus/test/exporter_utils_test.cc index 2222787787..228b63dbcf 100644 --- a/exporters/prometheus/test/exporter_utils_test.cc +++ b/exporters/prometheus/test/exporter_utils_test.cc @@ -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 void assert_basic(prometheus_client::MetricFamily &metric, const std::string &sanitized_name, @@ -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); @@ -147,14 +138,71 @@ TEST(PrometheusExporterUtils, TranslateToPrometheusHistogramNormal) assert_histogram(metric, std::list{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 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 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 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 &expected) + { + std::vector 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