From a45081a83b4ec728f8db42b0aa950ca2338ef756 Mon Sep 17 00:00:00 2001 From: Punya Biswal Date: Wed, 27 Sep 2023 03:04:27 -0400 Subject: [PATCH 1/3] [exporters/prometheus] Sanitize labels according to Prometheus spec (#2330) Fixes #2289 --- CHANGELOG.md | 2 + .../exporters/prometheus/exporter_utils.h | 9 -- exporters/prometheus/src/exporter_utils.cc | 129 +++++++++++------- .../prometheus/test/exporter_utils_test.cc | 44 ++++-- 4 files changed, 116 insertions(+), 68 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38a077e7d8..7901a35732 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,8 @@ Increment the: [#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) +* [EXPORTER] Replace colons with underscores when converting to Prometheus label + [#2324](https://github.com/open-telemetry/opentelemetry-cpp/pull/2330) ## [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 312b641c3e..07d2c396c6 100644 --- a/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h +++ b/exporters/prometheus/include/opentelemetry/exporters/prometheus/exporter_utils.h @@ -32,15 +32,6 @@ class PrometheusExporterUtils const sdk::metrics::ResourceMetrics &data); private: - /** - * Sanitize the given metric name or label according to Prometheus rule. - * - * This function is needed because names in OpenTelemetry can contain - * alphanumeric characters, '_', '.', and '-', whereas in Prometheus the - * name should only contain alphanumeric characters and '_'. - */ - static std::string SanitizeNames(std::string name); - static opentelemetry::sdk::metrics::AggregationType getAggregationType( const opentelemetry::sdk::metrics::PointType &point_type); diff --git a/exporters/prometheus/src/exporter_utils.cc b/exporters/prometheus/src/exporter_utils.cc index 149d93290b..a840b087ff 100644 --- a/exporters/prometheus/src/exporter_utils.cc +++ b/exporters/prometheus/src/exporter_utils.cc @@ -20,6 +20,85 @@ namespace exporter { namespace metrics { +namespace +{ +/** + * Sanitize the given metric name by replacing invalid characters with _, + * ensuring that multiple consecutive _ characters are collapsed to a single _. + * + * @param valid a callable with the signature `(int pos, char ch) -> bool` that + * returns whether `ch` is valid at position `pos` in the string + * @param name the string to sanitize + */ +template +inline std::string Sanitize(std::string name, const T &valid) +{ + static_assert(std::is_convertible>::value, + "valid should be a callable with the signature " + "(int, char) -> bool"); + + constexpr const auto replacement = '_'; + constexpr const auto replacement_dup = '='; + + bool has_dup = false; + for (int i = 0; i < (int)name.size(); ++i) + { + if (valid(i, name[i]) && name[i] != replacement) + { + continue; + } + if (i > 0 && (name[i - 1] == replacement || name[i - 1] == replacement_dup)) + { + has_dup = true; + name[i] = replacement_dup; + } + else + { + name[i] = replacement; + } + } + if (has_dup) + { + auto end = std::remove(name.begin(), name.end(), replacement_dup); + return std::string{name.begin(), end}; + } + return name; +} + +/** + * Sanitize the given metric label key according to Prometheus rule. + * Prometheus metric label keys are required to match the following regex: + * [a-zA-Z_]([a-zA-Z0-9_])* + * and multiple consecutive _ characters must be collapsed to a single _. + */ +std::string SanitizeLabel(std::string label_key) +{ + return Sanitize(label_key, [](int i, char c) { + return (c >= 'a' && c <= 'z') || // + (c >= 'A' && c <= 'Z') || // + c == '_' || // + (c >= '0' && c <= '9' && i > 0); + }); +} + +/** + * Sanitize the given metric name according to Prometheus rule. + * Prometheus metric names are required to match the following regex: + * [a-zA-Z_:]([a-zA-Z0-9_:])* + * and multiple consecutive _ characters must be collapsed to a single _. + */ +std::string SanitizeName(std::string name) +{ + return Sanitize(name, [](int i, char c) { + return (c >= 'a' && c <= 'z') || // + (c >= 'A' && c <= 'Z') || // + c == '_' || // + c == ':' || // + (c >= '0' && c <= '9' && i > 0); + }); +} +} // namespace + /** * Helper function to convert OpenTelemetry metrics data collection * to Prometheus metrics data collection @@ -40,7 +119,7 @@ std::vector PrometheusExporterUtils::TranslateT { auto origin_name = metric_data.instrument_descriptor.name_; auto unit = metric_data.instrument_descriptor.unit_; - auto sanitized = SanitizeNames(origin_name); + auto sanitized = SanitizeName(origin_name); prometheus_client::MetricFamily metric_family; metric_family.name = sanitized + "_" + unit; metric_family.help = metric_data.instrument_descriptor.description_; @@ -120,52 +199,6 @@ std::vector PrometheusExporterUtils::TranslateT return output; } -/** - * Sanitize the given metric name or label according to Prometheus rule. - * - * This function is needed because names in OpenTelemetry can contain - * alphanumeric characters, '_', '.', and '-', whereas in Prometheus the - * name should only contain alphanumeric characters and '_'. - */ -std::string PrometheusExporterUtils::SanitizeNames(std::string name) -{ - constexpr const auto replacement = '_'; - constexpr const auto replacement_dup = '='; - - auto valid = [](int i, char c) { - if ((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == ':' || - (c >= '0' && c <= '9' && i > 0)) - { - return true; - } - return false; - }; - - bool has_dup = false; - for (int i = 0; i < (int)name.size(); ++i) - { - if (valid(i, name[i])) - { - continue; - } - if (i > 0 && (name[i - 1] == replacement || name[i - 1] == replacement_dup)) - { - has_dup = true; - name[i] = replacement_dup; - } - else - { - name[i] = replacement; - } - } - if (has_dup) - { - auto end = std::remove(name.begin(), name.end(), replacement_dup); - return std::string{name.begin(), end}; - } - return name; -} - metric_sdk::AggregationType PrometheusExporterUtils::getAggregationType( const metric_sdk::PointType &point_type) { @@ -271,7 +304,7 @@ void PrometheusExporterUtils::SetMetricBasic(prometheus_client::ClientMetric &me std::string previous_key; for (auto const &label : labels) { - auto sanitized = SanitizeNames(label.first); + auto sanitized = SanitizeLabel(label.first); int comparison = previous_key.compare(sanitized); if (metric.label.empty() || comparison < 0) // new key { diff --git a/exporters/prometheus/test/exporter_utils_test.cc b/exporters/prometheus/test/exporter_utils_test.cc index 228b63dbcf..c2a6273397 100644 --- a/exporters/prometheus/test/exporter_utils_test.cc +++ b/exporters/prometheus/test/exporter_utils_test.cc @@ -138,33 +138,55 @@ TEST(PrometheusExporterUtils, TranslateToPrometheusHistogramNormal) assert_histogram(metric, std::list{10.1, 20.2, 30.2}, {200, 300, 400, 500}); } -class SanitizeNameTest : public ::testing::Test +class SanitizeTest : 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) + void CheckSanitizeName(const std::string &original, const std::string &sanitized) { - metric_sdk::InstrumentDescriptor instrument_descriptor_{ + 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_, {}, {}, {}, {{{}, {}}}}}}}}); + {{instrumentation_scope_.get(), {{instrument_descriptor, {}, {}, {}, {{{}, {}}}}}}}}); EXPECT_EQ(result.begin()->name, sanitized + "_unit"); } + + void CheckSanitizeLabel(const std::string &original, const std::string &sanitized) + { + metric_sdk::InstrumentDescriptor instrument_descriptor{ + "name", "description", "unit", metric_sdk::InstrumentType::kCounter, + metric_sdk::InstrumentValueType::kDouble}; + std::vector result = PrometheusExporterUtils::TranslateToPrometheus( + {&resource_, + {{instrumentation_scope_.get(), + {{instrument_descriptor, {}, {}, {}, {{{{original, "value"}}, {}}}}}}}}); + EXPECT_EQ(result.begin()->metric.begin()->label.begin()->name, sanitized); + } }; -TEST_F(SanitizeNameTest, All) +TEST_F(SanitizeTest, Name) +{ + CheckSanitizeName("name", "name"); + CheckSanitizeName("name?", "name_"); + CheckSanitizeName("name???", "name_"); + CheckSanitizeName("name?__", "name_"); + CheckSanitizeName("name?__name", "name_name"); + CheckSanitizeName("name?__name:", "name_name:"); +} + +TEST_F(SanitizeTest, Label) { - CheckSanitation("name", "name"); - CheckSanitation("name?", "name_"); - CheckSanitation("name???", "name_"); - CheckSanitation("name?__", "name_"); - CheckSanitation("name?__name", "name_name"); - CheckSanitation("name?__name:", "name_name:"); + CheckSanitizeLabel("name", "name"); + CheckSanitizeLabel("name?", "name_"); + CheckSanitizeLabel("name???", "name_"); + CheckSanitizeLabel("name?__", "name_"); + CheckSanitizeLabel("name?__name", "name_name"); + CheckSanitizeLabel("name?__name:", "name_name_"); } class AttributeCollisionTest : public ::testing::Test From 368ee792107961c6ad7c140864af276955f5dc58 Mon Sep 17 00:00:00 2001 From: WenTao Ou Date: Thu, 28 Sep 2023 15:02:55 +0800 Subject: [PATCH 2/3] Fix deadlock when shuting down http client. (#2337) --- ext/src/http/client/curl/http_client_curl.cc | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/ext/src/http/client/curl/http_client_curl.cc b/ext/src/http/client/curl/http_client_curl.cc index 94bbf9fa7b..287059a1c6 100644 --- a/ext/src/http/client/curl/http_client_curl.cc +++ b/ext/src/http/client/curl/http_client_curl.cc @@ -244,6 +244,7 @@ void HttpClient::CleanupSession(uint64_t session_id) } } + bool need_wakeup_background_thread = false; { std::lock_guard lock_guard{session_ids_m_}; pending_to_add_session_ids_.erase(session_id); @@ -259,10 +260,15 @@ void HttpClient::CleanupSession(uint64_t session_id) { // If this session is already running, give it to the background thread for cleanup. pending_to_abort_sessions_[session_id] = std::move(session); - wakeupBackgroundThread(); + need_wakeup_background_thread = true; } } } + + if (need_wakeup_background_thread) + { + wakeupBackgroundThread(); + } } void HttpClient::MaybeSpawnBackgroundThread() @@ -519,7 +525,8 @@ bool HttpClient::doRemoveSessions() std::lock_guard session_id_lock_guard{session_ids_m_}; pending_to_remove_session_handles_.swap(pending_to_remove_session_handles); pending_to_remove_sessions_.swap(pending_to_remove_sessions); - + } + { // If user callback do not call CancelSession or FinishSession, We still need to remove it // from sessions_ std::lock_guard session_lock_guard{sessions_m_}; From 5e96b80bc964eeb61ddfd62a61d7b746fe75a97a Mon Sep 17 00:00:00 2001 From: Matthijs Brobbel Date: Thu, 28 Sep 2023 09:46:17 +0200 Subject: [PATCH 3/3] [Exporter] Group spans by resource and instrumentation scope in OTLP export requests (#2335) --- .../exporters/otlp/otlp_recordable.h | 3 + exporters/otlp/src/otlp_recordable.cc | 11 +++ exporters/otlp/src/otlp_recordable_utils.cc | 49 +++++++++-- exporters/otlp/test/otlp_recordable_test.cc | 88 +++++++++++++++++++ 4 files changed, 144 insertions(+), 7 deletions(-) diff --git a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_recordable.h b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_recordable.h index 29bc6011f9..bc505a959b 100644 --- a/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_recordable.h +++ b/exporters/otlp/include/opentelemetry/exporters/otlp/otlp_recordable.h @@ -27,7 +27,10 @@ class OtlpRecordable final : public opentelemetry::sdk::trace::Recordable /** Dynamically converts the resource of this span into a proto. */ proto::resource::v1::Resource ProtoResource() const noexcept; + const opentelemetry::sdk::resource::Resource *GetResource() const noexcept; const std::string GetResourceSchemaURL() const noexcept; + const opentelemetry::sdk::instrumentationscope::InstrumentationScope *GetInstrumentationScope() + const noexcept; const std::string GetInstrumentationLibrarySchemaURL() const noexcept; proto::common::v1::InstrumentationScope GetProtoInstrumentationScope() const noexcept; diff --git a/exporters/otlp/src/otlp_recordable.cc b/exporters/otlp/src/otlp_recordable.cc index e0b181a452..a3ab4327f2 100644 --- a/exporters/otlp/src/otlp_recordable.cc +++ b/exporters/otlp/src/otlp_recordable.cc @@ -40,6 +40,11 @@ proto::resource::v1::Resource OtlpRecordable::ProtoResource() const noexcept return proto; } +const opentelemetry::sdk::resource::Resource *OtlpRecordable::GetResource() const noexcept +{ + return resource_; +} + const std::string OtlpRecordable::GetResourceSchemaURL() const noexcept { std::string schema_url; @@ -51,6 +56,12 @@ const std::string OtlpRecordable::GetResourceSchemaURL() const noexcept return schema_url; } +const opentelemetry::sdk::instrumentationscope::InstrumentationScope * +OtlpRecordable::GetInstrumentationScope() const noexcept +{ + return instrumentation_scope_; +} + const std::string OtlpRecordable::GetInstrumentationLibrarySchemaURL() const noexcept { std::string schema_url; diff --git a/exporters/otlp/src/otlp_recordable_utils.cc b/exporters/otlp/src/otlp_recordable_utils.cc index 1b1da94e08..59e4aadf4e 100644 --- a/exporters/otlp/src/otlp_recordable_utils.cc +++ b/exporters/otlp/src/otlp_recordable_utils.cc @@ -56,19 +56,54 @@ void OtlpRecordableUtils::PopulateRequest( return; } + using spans_by_scope = + std::unordered_map>>; + std::unordered_map spans_index; + + // Collect spans per resource and instrumentation scope for (auto &recordable : spans) { auto rec = std::unique_ptr(static_cast(recordable.release())); - auto resource_span = request->add_resource_spans(); - auto scope_spans = resource_span->add_scope_spans(); + auto resource = rec->GetResource(); + auto instrumentation = rec->GetInstrumentationScope(); - *scope_spans->add_spans() = std::move(rec->span()); - *scope_spans->mutable_scope() = rec->GetProtoInstrumentationScope(); + spans_index[resource][instrumentation].emplace_back(std::move(rec)); + } - scope_spans->set_schema_url(rec->GetInstrumentationLibrarySchemaURL()); + // Add all resource spans + for (auto &input_resource_spans : spans_index) + { + // Add the resource + auto resource_spans = request->add_resource_spans(); + if (input_resource_spans.first) + { + proto::resource::v1::Resource resource_proto; + OtlpPopulateAttributeUtils::PopulateAttribute(&resource_proto, *input_resource_spans.first); + *resource_spans->mutable_resource() = resource_proto; + resource_spans->set_schema_url(input_resource_spans.first->GetSchemaURL()); + } - *resource_span->mutable_resource() = rec->ProtoResource(); - resource_span->set_schema_url(rec->GetResourceSchemaURL()); + // Add all scope spans + for (auto &input_scope_spans : input_resource_spans.second) + { + // Add the instrumentation scope + auto scope_spans = resource_spans->add_scope_spans(); + if (input_scope_spans.first) + { + proto::common::v1::InstrumentationScope instrumentation_scope_proto; + instrumentation_scope_proto.set_name(input_scope_spans.first->GetName()); + instrumentation_scope_proto.set_version(input_scope_spans.first->GetVersion()); + *scope_spans->mutable_scope() = instrumentation_scope_proto; + scope_spans->set_schema_url(input_scope_spans.first->GetSchemaURL()); + } + + // Add all spans to this scope spans + for (auto &input_span : input_scope_spans.second) + { + *scope_spans->add_spans() = std::move(input_span->span()); + } + } } } diff --git a/exporters/otlp/test/otlp_recordable_test.cc b/exporters/otlp/test/otlp_recordable_test.cc index 4db58844f7..9f3ba62b16 100644 --- a/exporters/otlp/test/otlp_recordable_test.cc +++ b/exporters/otlp/test/otlp_recordable_test.cc @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 #include "opentelemetry/exporters/otlp/otlp_recordable.h" +#include "opentelemetry/exporters/otlp/otlp_recordable_utils.h" #include "opentelemetry/sdk/resource/resource.h" #if defined(__GNUC__) @@ -285,6 +286,93 @@ TEST(OtlpRecordable, SetArrayAttribute) } } +// Test otlp resource populate request util +TEST(OtlpRecordable, PopulateRequest) +{ + auto rec1 = std::unique_ptr(new OtlpRecordable); + auto resource1 = resource::Resource::Create({{"service.name", "one"}}); + rec1->SetResource(resource1); + auto inst_lib1 = trace_sdk::InstrumentationScope::Create("one", "1"); + rec1->SetInstrumentationScope(*inst_lib1); + + auto rec2 = std::unique_ptr(new OtlpRecordable); + auto resource2 = resource::Resource::Create({{"service.name", "two"}}); + rec2->SetResource(resource2); + auto inst_lib2 = trace_sdk::InstrumentationScope::Create("two", "2"); + rec2->SetInstrumentationScope(*inst_lib2); + + // This has the same resource as rec2, but a different scope + auto rec3 = std::unique_ptr(new OtlpRecordable); + rec3->SetResource(resource2); + auto inst_lib3 = trace_sdk::InstrumentationScope::Create("three", "3"); + rec3->SetInstrumentationScope(*inst_lib3); + + proto::collector::trace::v1::ExportTraceServiceRequest req; + std::vector> spans; + spans.push_back(std::move(rec1)); + spans.push_back(std::move(rec2)); + spans.push_back(std::move(rec3)); + const nostd::span, 3> spans_span(spans.data(), 3); + OtlpRecordableUtils::PopulateRequest(spans_span, &req); + + EXPECT_EQ(req.resource_spans().size(), 2); + for (auto resource_spans : req.resource_spans()) + { + auto service_name = resource_spans.resource().attributes(0).value().string_value(); + auto scope_spans_size = resource_spans.scope_spans().size(); + if (service_name == "one") + { + EXPECT_EQ(scope_spans_size, 1); + EXPECT_EQ(resource_spans.scope_spans(0).scope().name(), "one"); + } + if (service_name == "two") + { + EXPECT_EQ(scope_spans_size, 2); + } + } +} + +// Test otlp resource populate request util with missing data +TEST(OtlpRecordable, PopulateRequestMissing) +{ + // Missing scope + auto rec1 = std::unique_ptr(new OtlpRecordable); + auto resource1 = resource::Resource::Create({{"service.name", "one"}}); + rec1->SetResource(resource1); + + // Missing resource + auto rec2 = std::unique_ptr(new OtlpRecordable); + auto inst_lib2 = trace_sdk::InstrumentationScope::Create("two", "2"); + rec2->SetInstrumentationScope(*inst_lib2); + + proto::collector::trace::v1::ExportTraceServiceRequest req; + std::vector> spans; + spans.push_back(std::move(rec1)); + spans.push_back(std::move(rec2)); + const nostd::span, 2> spans_span(spans.data(), 2); + OtlpRecordableUtils::PopulateRequest(spans_span, &req); + + EXPECT_EQ(req.resource_spans().size(), 2); + for (auto resource_spans : req.resource_spans()) + { + // Both should have scope spans + EXPECT_EQ(resource_spans.scope_spans().size(), 1); + auto scope = resource_spans.scope_spans(0).scope(); + // Select the one with missing scope + if (scope.name() == "") + { + // Version is also empty + EXPECT_EQ(scope.version(), ""); + } + else + { + // The other has a name and version + EXPECT_EQ(scope.name(), "two"); + EXPECT_EQ(scope.version(), "2"); + } + } +} + template struct EmptyArrayAttributeTest : public testing::Test {