From 05c057505716e980373e2f412430e0fba0a968c7 Mon Sep 17 00:00:00 2001 From: Lalit Kumar Bhasin Date: Tue, 25 Oct 2022 23:37:02 -0700 Subject: [PATCH 01/10] validate instrument --- api/include/opentelemetry/common/macros.h | 9 +++ .../metrics/instrument_metadata_validator.h | 38 ++++++++++ sdk/include/opentelemetry/sdk/metrics/meter.h | 20 +++++ .../sdk/metrics/view/predicate.h | 9 +-- sdk/src/metrics/CMakeLists.txt | 1 + .../metrics/instrument_metadata_validator.cc | 71 ++++++++++++++++++ sdk/src/metrics/meter.cc | 54 +++++++++++++ sdk/test/metrics/CMakeLists.txt | 3 +- .../instrument_metadata_validator_test.cc | 75 +++++++++++++++++++ 9 files changed, 271 insertions(+), 9 deletions(-) create mode 100644 sdk/include/opentelemetry/sdk/metrics/instrument_metadata_validator.h create mode 100644 sdk/src/metrics/instrument_metadata_validator.cc create mode 100644 sdk/test/metrics/instrument_metadata_validator_test.cc diff --git a/api/include/opentelemetry/common/macros.h b/api/include/opentelemetry/common/macros.h index 8f88fc8f85..9770909294 100644 --- a/api/include/opentelemetry/common/macros.h +++ b/api/include/opentelemetry/common/macros.h @@ -90,6 +90,15 @@ # define OPENTELEMETRY_DEPRECATED_MESSAGE(msg) #endif +// Regex support +#if (__GNUC__ == 4 && (__GNUC_MINOR__ == 8 || __GNUC_MINOR__ == 9)) +# define HAVE_WORKING_REGEX 0 +# include "opentelemetry/sdk/common/global_log_handler.h" +#else +# include +# define HAVE_WORKING_REGEX 1 +#endif + /* clang-format off */ /** diff --git a/sdk/include/opentelemetry/sdk/metrics/instrument_metadata_validator.h b/sdk/include/opentelemetry/sdk/metrics/instrument_metadata_validator.h new file mode 100644 index 0000000000..93782370f2 --- /dev/null +++ b/sdk/include/opentelemetry/sdk/metrics/instrument_metadata_validator.h @@ -0,0 +1,38 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#pragma once +#ifndef ENABLE_METRICS_PREVIEW +# include +# include "opentelemetry/common/macros.h" +# include "opentelemetry/nostd/string_view.h" +# include "opentelemetry/version.h" + +# if HAVE_WORKING_REGEX +# include +# endif + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ +class InstrumentMetaDataValidator +{ +public: + InstrumentMetaDataValidator(); + bool ValidateName(nostd::string_view name) const; + bool ValidateUnit(nostd::string_view unit) const; + bool ValidateDescription(nostd::string_view description) const; + +private: +# if HAVE_WORKING_REGEX + const std::regex name_reg_key_; + const std::regex unit_reg_key_; +# endif +}; + +} // namespace metrics +} // namespace sdk +OPENTELEMETRY_END_NAMESPACE +#endif \ No newline at end of file diff --git a/sdk/include/opentelemetry/sdk/metrics/meter.h b/sdk/include/opentelemetry/sdk/metrics/meter.h index c9d9fe1566..4d286e4bf6 100644 --- a/sdk/include/opentelemetry/sdk/metrics/meter.h +++ b/sdk/include/opentelemetry/sdk/metrics/meter.h @@ -5,7 +5,9 @@ #ifndef ENABLE_METRICS_PREVIEW # include # include "opentelemetry/metrics/meter.h" +# include "opentelemetry/metrics/noop.h" # include "opentelemetry/sdk/instrumentationscope/instrumentation_scope.h" +# include "opentelemetry/sdk/metrics/instrument_metadata_validator.h" # include "opentelemetry/sdk/metrics/instruments.h" # include "opentelemetry/sdk/metrics/meter_context.h" # include "opentelemetry/sdk/metrics/state/async_metric_storage.h" @@ -26,6 +28,15 @@ class SyncWritableMetricStorage; class AsyncWritableMetricsStorge; class ObservableRegistry; +inline bool ValidateInstrument(nostd::string_view name, + nostd::string_view description, + nostd::string_view unit) +{ + const static InstrumentMetaDataValidator instrument_validator; + return instrument_validator.ValidateName(name) && instrument_validator.ValidateUnit(unit) && + instrument_validator.ValidateDescription(description); +} + class Meter final : public opentelemetry::metrics::Meter { public: @@ -121,6 +132,15 @@ class Meter final : public opentelemetry::metrics::Meter std::unique_ptr RegisterAsyncMetricStorage( InstrumentDescriptor &instrument_descriptor); opentelemetry::common::SpinLockMutex storage_lock_; + const InstrumentMetaDataValidator instrument_metadata_validator; + + static nostd::shared_ptr + GetNoopObservableInsrument() + { + static nostd::shared_ptr noop_instrument( + new opentelemetry::metrics::NoopObservableInstrument("", "", "")); + return noop_instrument; + } }; } // namespace metrics } // namespace sdk diff --git a/sdk/include/opentelemetry/sdk/metrics/view/predicate.h b/sdk/include/opentelemetry/sdk/metrics/view/predicate.h index 2fe5fbf155..82d1a23465 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/predicate.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/predicate.h @@ -4,14 +4,7 @@ #pragma once #ifndef ENABLE_METRICS_PREVIEW # include -# if (__GNUC__ == 4 && (__GNUC_MINOR__ == 8 || __GNUC_MINOR__ == 9)) -# define HAVE_WORKING_REGEX 0 -# include "opentelemetry/sdk/common/global_log_handler.h" -# else -# include -# define HAVE_WORKING_REGEX 1 -# endif - +# include "opentelemetry/common/macros.h" # include "opentelemetry/nostd/string_view.h" OPENTELEMETRY_BEGIN_NAMESPACE diff --git a/sdk/src/metrics/CMakeLists.txt b/sdk/src/metrics/CMakeLists.txt index 80c1bff335..7f9214e324 100644 --- a/sdk/src/metrics/CMakeLists.txt +++ b/sdk/src/metrics/CMakeLists.txt @@ -5,6 +5,7 @@ add_library( meter.cc meter_context.cc metric_reader.cc + instrument_metadata_validator.cc export/periodic_exporting_metric_reader.cc state/metric_collector.cc state/observable_registry.cc diff --git a/sdk/src/metrics/instrument_metadata_validator.cc b/sdk/src/metrics/instrument_metadata_validator.cc new file mode 100644 index 0000000000..06da496dd3 --- /dev/null +++ b/sdk/src/metrics/instrument_metadata_validator.cc @@ -0,0 +1,71 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#ifndef ENABLE_METRICS_PREVIEW +# include "opentelemetry/sdk/metrics/instrument_metadata_validator.h" +# include "opentelemetry/common/macros.h" +# include "opentelemetry/nostd/string_view.h" + +# include +# include + +OPENTELEMETRY_BEGIN_NAMESPACE +namespace sdk +{ +namespace metrics +{ +const size_t kMaxSize = 63; +// instrument-name = ALPHA 0*62 ("_" / "." / "-" / ALPHA / DIGIT) +const std::string kInstrumentNamePattern = "[a-zA-Z][-_.a-zA-Z0-9]{0,62}"; +// +const std::string kInstrumentUnitPattern = "[\x01-\x7F]{0,63}"; +// instrument-unit = It can have a maximum length of 63 ASCII chars + +InstrumentMetaDataValidator::InstrumentMetaDataValidator() + : name_reg_key_{kInstrumentNamePattern}, unit_reg_key_{kInstrumentUnitPattern} +{} + +bool InstrumentMetaDataValidator::ValidateName(nostd::string_view name) const +{ +# if HAVE_WORKING_REGEX + return std::regex_match(name.data(), name_reg_key_); +# endif + // size atmost 63 chars + if (name.size() > kMaxSize) + { + return false; + } + // first char should be alphanumeric + if (!isalpha(name[0])) + { + return false; + } + // subsequent chars should be either of alphabets, underscore, minus, dot + return !std::any_of(std::next(name.begin()), name.end(), + [](char c) { return !isalpha(c) && c != '-' && c != '_' && c != '.'; }); +} + +bool InstrumentMetaDataValidator::ValidateUnit(nostd::string_view unit) const +{ +# if HAVE_WORKING_REGEX + return std::regex_match(unit.data(), unit_reg_key_); +# endif + // length atmost 63 chars + if (unit.size() > kMaxSize) + { + return false; + } + // all should be ascii chars. + return !std::any_of(unit.begin(), unit.end(), + [](char c) { return static_cast(c) > 127; }); +} + +bool InstrumentMetaDataValidator::ValidateDescription(nostd::string_view description) const +{ + return true; +} + +} // namespace metrics +} // namespace sdk +OPENTELEMETRY_END_NAMESPACE +#endif \ No newline at end of file diff --git a/sdk/src/metrics/meter.cc b/sdk/src/metrics/meter.cc index bff0cf5535..ff7f2263c9 100644 --- a/sdk/src/metrics/meter.cc +++ b/sdk/src/metrics/meter.cc @@ -39,6 +39,11 @@ nostd::unique_ptr> Meter::CreateUInt64Counter( nostd::string_view description, nostd::string_view unit) noexcept { + if (!ValidateInstrument(name, description, unit)) + { + return nostd::unique_ptr>( + new metrics::NoopCounter(name, description, unit)); + } InstrumentDescriptor instrument_descriptor = { std::string{name.data(), name.size()}, std::string{description.data(), description.size()}, std::string{unit.data(), unit.size()}, InstrumentType::kCounter, InstrumentValueType::kLong}; @@ -52,6 +57,11 @@ nostd::unique_ptr> Meter::CreateDoubleCounter( nostd::string_view description, nostd::string_view unit) noexcept { + if (!ValidateInstrument(name, description, unit)) + { + return nostd::unique_ptr>( + new metrics::NoopCounter(name, description, unit)); + } InstrumentDescriptor instrument_descriptor = { std::string{name.data(), name.size()}, std::string{description.data(), description.size()}, std::string{unit.data(), unit.size()}, InstrumentType::kCounter, @@ -66,6 +76,10 @@ nostd::shared_ptr Meter::CreateInt nostd::string_view description, nostd::string_view unit) noexcept { + if (!ValidateInstrument(name, description, unit)) + { + return GetNoopObservableInsrument(); + } InstrumentDescriptor instrument_descriptor = { std::string{name.data(), name.size()}, std::string{description.data(), description.size()}, std::string{unit.data(), unit.size()}, InstrumentType::kObservableCounter, @@ -80,6 +94,10 @@ Meter::CreateDoubleObservableCounter(nostd::string_view name, nostd::string_view description, nostd::string_view unit) noexcept { + if (!ValidateInstrument(name, description, unit)) + { + return GetNoopObservableInsrument(); + } InstrumentDescriptor instrument_descriptor = { std::string{name.data(), name.size()}, std::string{description.data(), description.size()}, std::string{unit.data(), unit.size()}, InstrumentType::kObservableCounter, @@ -94,6 +112,11 @@ nostd::unique_ptr> Meter::CreateUInt64Histogram( nostd::string_view description, nostd::string_view unit) noexcept { + if (!ValidateInstrument(name, description, unit)) + { + return nostd::unique_ptr>( + new metrics::NoopHistogram(name, description, unit)); + } InstrumentDescriptor instrument_descriptor = { std::string{name.data(), name.size()}, std::string{description.data(), description.size()}, std::string{unit.data(), unit.size()}, InstrumentType::kHistogram, @@ -108,6 +131,11 @@ nostd::unique_ptr> Meter::CreateDoubleHistogram( nostd::string_view description, nostd::string_view unit) noexcept { + if (!ValidateInstrument(name, description, unit)) + { + return nostd::unique_ptr>( + new metrics::NoopHistogram(name, description, unit)); + } InstrumentDescriptor instrument_descriptor = { std::string{name.data(), name.size()}, std::string{description.data(), description.size()}, std::string{unit.data(), unit.size()}, InstrumentType::kHistogram, @@ -122,6 +150,10 @@ nostd::shared_ptr Meter::CreateInt nostd::string_view description, nostd::string_view unit) noexcept { + if (!ValidateInstrument(name, description, unit)) + { + return GetNoopObservableInsrument(); + } InstrumentDescriptor instrument_descriptor = { std::string{name.data(), name.size()}, std::string{description.data(), description.size()}, std::string{unit.data(), unit.size()}, InstrumentType::kObservableGauge, @@ -136,6 +168,10 @@ nostd::shared_ptr Meter::CreateDou nostd::string_view description, nostd::string_view unit) noexcept { + if (!ValidateInstrument(name, description, unit)) + { + return GetNoopObservableInsrument(); + } InstrumentDescriptor instrument_descriptor = { std::string{name.data(), name.size()}, std::string{description.data(), description.size()}, std::string{unit.data(), unit.size()}, InstrumentType::kObservableGauge, @@ -150,6 +186,11 @@ nostd::unique_ptr> Meter::CreateInt64UpDownCount nostd::string_view description, nostd::string_view unit) noexcept { + if (!ValidateInstrument(name, description, unit)) + { + return nostd::unique_ptr>( + new metrics::NoopUpDownCounter(name, description, unit)); + } InstrumentDescriptor instrument_descriptor = { std::string{name.data(), name.size()}, std::string{description.data(), description.size()}, std::string{unit.data(), unit.size()}, InstrumentType::kUpDownCounter, @@ -164,6 +205,11 @@ nostd::unique_ptr> Meter::CreateDoubleUpDownCount nostd::string_view description, nostd::string_view unit) noexcept { + if (!ValidateInstrument(name, description, unit)) + { + return nostd::unique_ptr>( + new metrics::NoopUpDownCounter(name, description, unit)); + } InstrumentDescriptor instrument_descriptor = { std::string{name.data(), name.size()}, std::string{description.data(), description.size()}, std::string{unit.data(), unit.size()}, InstrumentType::kUpDownCounter, @@ -178,6 +224,10 @@ Meter::CreateInt64ObservableUpDownCounter(nostd::string_view name, nostd::string_view description, nostd::string_view unit) noexcept { + if (!ValidateInstrument(name, description, unit)) + { + return GetNoopObservableInsrument(); + } InstrumentDescriptor instrument_descriptor = { std::string{name.data(), name.size()}, std::string{description.data(), description.size()}, std::string{unit.data(), unit.size()}, InstrumentType::kObservableUpDownCounter, @@ -192,6 +242,10 @@ Meter::CreateDoubleObservableUpDownCounter(nostd::string_view name, nostd::string_view description, nostd::string_view unit) noexcept { + if (!ValidateInstrument(name, description, unit)) + { + return GetNoopObservableInsrument(); + } InstrumentDescriptor instrument_descriptor = { std::string{name.data(), name.size()}, std::string{description.data(), description.size()}, std::string{unit.data(), unit.size()}, InstrumentType::kObservableUpDownCounter, diff --git a/sdk/test/metrics/CMakeLists.txt b/sdk/test/metrics/CMakeLists.txt index 2ff8b5f30a..3e5e8f488a 100644 --- a/sdk/test/metrics/CMakeLists.txt +++ b/sdk/test/metrics/CMakeLists.txt @@ -16,7 +16,8 @@ foreach( async_instruments_test metric_reader_test observable_registry_test - periodic_exporting_metric_reader_test) + periodic_exporting_metric_reader_test + instrument_metadata_validator_test) add_executable(${testname} "${testname}.cc") target_link_libraries( ${testname} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT} diff --git a/sdk/test/metrics/instrument_metadata_validator_test.cc b/sdk/test/metrics/instrument_metadata_validator_test.cc new file mode 100644 index 0000000000..df7e7be594 --- /dev/null +++ b/sdk/test/metrics/instrument_metadata_validator_test.cc @@ -0,0 +1,75 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#ifndef ENABLE_METRICS_PREVIEW +# include "opentelemetry/sdk/metrics/instrument_metadata_validator.h" +# include + +static std::string CreateVeryLargeString(size_t multiple) +{ + std::string result = "START"; + std::string repeater = "0123456789"; + for (size_t i = 0; i < multiple; i++) + { + result += repeater; + } + return result; +} + +TEST(InstrumentMetadataValidator, TestName) +{ + opentelemetry::sdk::metrics::InstrumentMetaDataValidator validator; + std::vector invalid_names = { + "", // empty string + "1sdf", // string starting with number + "123€AAA€BBB", // unicode characters + "/\\sdsd", // string starting with special character + "***sSSs", // string starting with special character + CreateVeryLargeString(5) + "ABCERTYGJ", // total 64 charactes + CreateVeryLargeString(7), // string much bigger than 63 chars + }; + for (auto const &str : invalid_names) + { + EXPECT_FALSE(validator.ValidateName(str)); + } + + std::vector valid_names = { + "T", // single char string + "s123", // starting with char, followed by numbers + "dsdsdsd_-.", // string , and valid nonalphanumeric + "d1234_-sDSDs.sdsd344", // combination of all valid characters + CreateVeryLargeString(5) + "ABCERTYG", // total 63 charactes + }; + for (auto const &str : valid_names) + { + EXPECT_TRUE(validator.ValidateName(str)); + } +} + +TEST(InstrumentMetadataValidator, TestUnit) +{ + opentelemetry::sdk::metrics::InstrumentMetaDataValidator validator; + std::vector invalid_units = { + CreateVeryLargeString(5) + "ABCERTYGJ", // total 64 charactes + CreateVeryLargeString(7), // string bigger than 63 chars + "123€AAA€BBB", // unicode string + }; + for (auto const &str : invalid_units) + { + EXPECT_FALSE(validator.ValidateUnit(str)); + } + + std::vector valid_units = { + "T", // single char + "s123", // starting with char, followed by numbers + "dsdsdsd_-.", // string , and valid nonalphanumeric + "d1234_-sdsds.sdsd344", // combination of all valid characters + CreateVeryLargeString(5) + "ABCERTYG", // total 63 charactes + "ASDDSDF", // uppercase + }; + for (auto const &str : valid_units) + { + EXPECT_TRUE(validator.ValidateName(str)); + } +} +#endif From 6e0dda609c72e89d06b7d4e797db7efc89360a20 Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 26 Oct 2022 00:00:18 -0700 Subject: [PATCH 02/10] fix --- sdk/include/opentelemetry/sdk/metrics/meter.h | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/meter.h b/sdk/include/opentelemetry/sdk/metrics/meter.h index 4d286e4bf6..80f9d5bc67 100644 --- a/sdk/include/opentelemetry/sdk/metrics/meter.h +++ b/sdk/include/opentelemetry/sdk/metrics/meter.h @@ -28,15 +28,6 @@ class SyncWritableMetricStorage; class AsyncWritableMetricsStorge; class ObservableRegistry; -inline bool ValidateInstrument(nostd::string_view name, - nostd::string_view description, - nostd::string_view unit) -{ - const static InstrumentMetaDataValidator instrument_validator; - return instrument_validator.ValidateName(name) && instrument_validator.ValidateUnit(unit) && - instrument_validator.ValidateDescription(description); -} - class Meter final : public opentelemetry::metrics::Meter { public: @@ -141,6 +132,14 @@ class Meter final : public opentelemetry::metrics::Meter new opentelemetry::metrics::NoopObservableInstrument("", "", "")); return noop_instrument; } + static bool ValidateInstrument(nostd::string_view name, + nostd::string_view description, + nostd::string_view unit) + { + const static InstrumentMetaDataValidator instrument_validator; + return instrument_validator.ValidateName(name) && instrument_validator.ValidateUnit(unit) && + instrument_validator.ValidateDescription(description); + } }; } // namespace metrics } // namespace sdk From f06d87776814ce88f55717a59b0307f499355086 Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 26 Oct 2022 00:17:33 -0700 Subject: [PATCH 03/10] fix build --- api/include/opentelemetry/common/macros.h | 1 - .../sdk/metrics/view/predicate.h | 1 + .../metrics/instrument_metadata_validator.cc | 2 +- sdk/src/metrics/meter.cc | 27 +++++++++++++++++++ 4 files changed, 29 insertions(+), 2 deletions(-) diff --git a/api/include/opentelemetry/common/macros.h b/api/include/opentelemetry/common/macros.h index 9770909294..4741677a74 100644 --- a/api/include/opentelemetry/common/macros.h +++ b/api/include/opentelemetry/common/macros.h @@ -93,7 +93,6 @@ // Regex support #if (__GNUC__ == 4 && (__GNUC_MINOR__ == 8 || __GNUC_MINOR__ == 9)) # define HAVE_WORKING_REGEX 0 -# include "opentelemetry/sdk/common/global_log_handler.h" #else # include # define HAVE_WORKING_REGEX 1 diff --git a/sdk/include/opentelemetry/sdk/metrics/view/predicate.h b/sdk/include/opentelemetry/sdk/metrics/view/predicate.h index 82d1a23465..b60a338b7f 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/predicate.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/predicate.h @@ -6,6 +6,7 @@ # include # include "opentelemetry/common/macros.h" # include "opentelemetry/nostd/string_view.h" +# include "opentelemetry/sdk/common/global_log_handler.h" OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk diff --git a/sdk/src/metrics/instrument_metadata_validator.cc b/sdk/src/metrics/instrument_metadata_validator.cc index 06da496dd3..a201388fa3 100644 --- a/sdk/src/metrics/instrument_metadata_validator.cc +++ b/sdk/src/metrics/instrument_metadata_validator.cc @@ -60,7 +60,7 @@ bool InstrumentMetaDataValidator::ValidateUnit(nostd::string_view unit) const [](char c) { return static_cast(c) > 127; }); } -bool InstrumentMetaDataValidator::ValidateDescription(nostd::string_view description) const +bool InstrumentMetaDataValidator::ValidateDescription(nostd::string_view /*description*/) const { return true; } diff --git a/sdk/src/metrics/meter.cc b/sdk/src/metrics/meter.cc index ff7f2263c9..a4223ae88c 100644 --- a/sdk/src/metrics/meter.cc +++ b/sdk/src/metrics/meter.cc @@ -12,6 +12,7 @@ # include "opentelemetry/sdk/metrics/state/observable_registry.h" # include "opentelemetry/sdk/metrics/state/sync_metric_storage.h" +# include "opentelemetry/sdk/common/global_log_handler.h" # include "opentelemetry/sdk/metrics/sync_instruments.h" # include "opentelemetry/sdk_config.h" @@ -41,6 +42,8 @@ nostd::unique_ptr> Meter::CreateUInt64Counter( { if (!ValidateInstrument(name, description, unit)) { + OTEL_INTERNAL_LOG_ERROR("Meter::CreateUInt64Counter - failed. Invalid parameters." + << name << " " << description << " " << unit); return nostd::unique_ptr>( new metrics::NoopCounter(name, description, unit)); } @@ -59,6 +62,8 @@ nostd::unique_ptr> Meter::CreateDoubleCounter( { if (!ValidateInstrument(name, description, unit)) { + OTEL_INTERNAL_LOG_ERROR("Meter::CreateDoubleCounter - failed. Invalid parameters." + << name << " " << description << " " << unit); return nostd::unique_ptr>( new metrics::NoopCounter(name, description, unit)); } @@ -78,6 +83,8 @@ nostd::shared_ptr Meter::CreateInt { if (!ValidateInstrument(name, description, unit)) { + OTEL_INTERNAL_LOG_ERROR("Meter::CreateInt64ObservableCounter - failed. Invalid parameters." + << name << " " << description << " " << unit); return GetNoopObservableInsrument(); } InstrumentDescriptor instrument_descriptor = { @@ -96,6 +103,8 @@ Meter::CreateDoubleObservableCounter(nostd::string_view name, { if (!ValidateInstrument(name, description, unit)) { + OTEL_INTERNAL_LOG_ERROR("Meter::CreateDoubleObservableCounter - failed. Invalid parameters." + << name << " " << description << " " << unit); return GetNoopObservableInsrument(); } InstrumentDescriptor instrument_descriptor = { @@ -114,6 +123,8 @@ nostd::unique_ptr> Meter::CreateUInt64Histogram( { if (!ValidateInstrument(name, description, unit)) { + OTEL_INTERNAL_LOG_ERROR("Meter::CreateUInt64Histogram - failed. Invalid parameters." + << name << " " << description << " " << unit); return nostd::unique_ptr>( new metrics::NoopHistogram(name, description, unit)); } @@ -133,6 +144,8 @@ nostd::unique_ptr> Meter::CreateDoubleHistogram( { if (!ValidateInstrument(name, description, unit)) { + OTEL_INTERNAL_LOG_ERROR("Meter::CreateDoubleHistogram - failed. Invalid parameters." + << name << " " << description << " " << unit); return nostd::unique_ptr>( new metrics::NoopHistogram(name, description, unit)); } @@ -152,6 +165,8 @@ nostd::shared_ptr Meter::CreateInt { if (!ValidateInstrument(name, description, unit)) { + OTEL_INTERNAL_LOG_ERROR("Meter::CreateInt64ObservableGauge - failed. Invalid parameters." + << name << " " << description << " " << unit); return GetNoopObservableInsrument(); } InstrumentDescriptor instrument_descriptor = { @@ -170,6 +185,8 @@ nostd::shared_ptr Meter::CreateDou { if (!ValidateInstrument(name, description, unit)) { + OTEL_INTERNAL_LOG_ERROR("Meter::CreateDoubleObservableGauge - failed. Invalid parameters." + << name << " " << description << " " << unit); return GetNoopObservableInsrument(); } InstrumentDescriptor instrument_descriptor = { @@ -188,6 +205,8 @@ nostd::unique_ptr> Meter::CreateInt64UpDownCount { if (!ValidateInstrument(name, description, unit)) { + OTEL_INTERNAL_LOG_ERROR("Meter::CreateInt64UpDownCounter - failed. Invalid parameters." + << name << " " << description << " " << unit); return nostd::unique_ptr>( new metrics::NoopUpDownCounter(name, description, unit)); } @@ -207,6 +226,8 @@ nostd::unique_ptr> Meter::CreateDoubleUpDownCount { if (!ValidateInstrument(name, description, unit)) { + OTEL_INTERNAL_LOG_ERROR("Meter::CreateDoubleUpDownCounter - failed. Invalid parameters." + << name << " " << description << " " << unit); return nostd::unique_ptr>( new metrics::NoopUpDownCounter(name, description, unit)); } @@ -226,6 +247,9 @@ Meter::CreateInt64ObservableUpDownCounter(nostd::string_view name, { if (!ValidateInstrument(name, description, unit)) { + OTEL_INTERNAL_LOG_ERROR( + "Meter::CreateInt64ObservableUpDownCounter - failed. Invalid parameters." + << name << " " << description << " " << unit); return GetNoopObservableInsrument(); } InstrumentDescriptor instrument_descriptor = { @@ -244,6 +268,9 @@ Meter::CreateDoubleObservableUpDownCounter(nostd::string_view name, { if (!ValidateInstrument(name, description, unit)) { + OTEL_INTERNAL_LOG_ERROR( + "Meter::CreateDoubleObservableUpDownCounter - failed. Invalid parameters." + << name << " " << description << " " << unit); return GetNoopObservableInsrument(); } InstrumentDescriptor instrument_descriptor = { From 5c4290343ee61098cd62b31b99ea100bd25777c6 Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 26 Oct 2022 09:57:00 -0700 Subject: [PATCH 04/10] review comments, and build errors --- .../metrics/instrument_metadata_validator.h | 2 +- .../metrics/instrument_metadata_validator.cc | 13 +++++-- sdk/src/metrics/meter.cc | 36 ++++++++++++------- 3 files changed, 34 insertions(+), 17 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/instrument_metadata_validator.h b/sdk/include/opentelemetry/sdk/metrics/instrument_metadata_validator.h index 93782370f2..97e7550cde 100644 --- a/sdk/include/opentelemetry/sdk/metrics/instrument_metadata_validator.h +++ b/sdk/include/opentelemetry/sdk/metrics/instrument_metadata_validator.h @@ -35,4 +35,4 @@ class InstrumentMetaDataValidator } // namespace metrics } // namespace sdk OPENTELEMETRY_END_NAMESPACE -#endif \ No newline at end of file +#endif diff --git a/sdk/src/metrics/instrument_metadata_validator.cc b/sdk/src/metrics/instrument_metadata_validator.cc index a201388fa3..bb94b2f5cb 100644 --- a/sdk/src/metrics/instrument_metadata_validator.cc +++ b/sdk/src/metrics/instrument_metadata_validator.cc @@ -22,14 +22,19 @@ const std::string kInstrumentUnitPattern = "[\x01-\x7F]{0,63}"; // instrument-unit = It can have a maximum length of 63 ASCII chars InstrumentMetaDataValidator::InstrumentMetaDataValidator() - : name_reg_key_{kInstrumentNamePattern}, unit_reg_key_{kInstrumentUnitPattern} +# if HAVE_WORKING_REGEX + : name_reg_key_{kInstrumentNamePattern}, unit_reg_key_ +{ + kInstrumentUnitPattern +} +# endif {} bool InstrumentMetaDataValidator::ValidateName(nostd::string_view name) const { # if HAVE_WORKING_REGEX return std::regex_match(name.data(), name_reg_key_); -# endif +# else // size atmost 63 chars if (name.size() > kMaxSize) { @@ -43,13 +48,14 @@ bool InstrumentMetaDataValidator::ValidateName(nostd::string_view name) const // subsequent chars should be either of alphabets, underscore, minus, dot return !std::any_of(std::next(name.begin()), name.end(), [](char c) { return !isalpha(c) && c != '-' && c != '_' && c != '.'; }); +# endif } bool InstrumentMetaDataValidator::ValidateUnit(nostd::string_view unit) const { # if HAVE_WORKING_REGEX return std::regex_match(unit.data(), unit_reg_key_); -# endif +# else // length atmost 63 chars if (unit.size() > kMaxSize) { @@ -58,6 +64,7 @@ bool InstrumentMetaDataValidator::ValidateUnit(nostd::string_view unit) const // all should be ascii chars. return !std::any_of(unit.begin(), unit.end(), [](char c) { return static_cast(c) > 127; }); +# endif } bool InstrumentMetaDataValidator::ValidateDescription(nostd::string_view /*description*/) const diff --git a/sdk/src/metrics/meter.cc b/sdk/src/metrics/meter.cc index a4223ae88c..e5848e8619 100644 --- a/sdk/src/metrics/meter.cc +++ b/sdk/src/metrics/meter.cc @@ -43,7 +43,8 @@ nostd::unique_ptr> Meter::CreateUInt64Counter( if (!ValidateInstrument(name, description, unit)) { OTEL_INTERNAL_LOG_ERROR("Meter::CreateUInt64Counter - failed. Invalid parameters." - << name << " " << description << " " << unit); + << name << " " << description << " " << unit + << ". Measurements won't be recorded."); return nostd::unique_ptr>( new metrics::NoopCounter(name, description, unit)); } @@ -63,7 +64,8 @@ nostd::unique_ptr> Meter::CreateDoubleCounter( if (!ValidateInstrument(name, description, unit)) { OTEL_INTERNAL_LOG_ERROR("Meter::CreateDoubleCounter - failed. Invalid parameters." - << name << " " << description << " " << unit); + << name << " " << description << " " << unit + << ". Measurements won't be recorded."); return nostd::unique_ptr>( new metrics::NoopCounter(name, description, unit)); } @@ -84,7 +86,8 @@ nostd::shared_ptr Meter::CreateInt if (!ValidateInstrument(name, description, unit)) { OTEL_INTERNAL_LOG_ERROR("Meter::CreateInt64ObservableCounter - failed. Invalid parameters." - << name << " " << description << " " << unit); + << name << " " << description << " " << unit + << ". Measurements won't be recorded."); return GetNoopObservableInsrument(); } InstrumentDescriptor instrument_descriptor = { @@ -104,7 +107,8 @@ Meter::CreateDoubleObservableCounter(nostd::string_view name, if (!ValidateInstrument(name, description, unit)) { OTEL_INTERNAL_LOG_ERROR("Meter::CreateDoubleObservableCounter - failed. Invalid parameters." - << name << " " << description << " " << unit); + << name << " " << description << " " << unit + << ". Measurements won't be recorded."); return GetNoopObservableInsrument(); } InstrumentDescriptor instrument_descriptor = { @@ -124,7 +128,8 @@ nostd::unique_ptr> Meter::CreateUInt64Histogram( if (!ValidateInstrument(name, description, unit)) { OTEL_INTERNAL_LOG_ERROR("Meter::CreateUInt64Histogram - failed. Invalid parameters." - << name << " " << description << " " << unit); + << name << " " << description << " " << unit + << ". Measurements won't be recorded."); return nostd::unique_ptr>( new metrics::NoopHistogram(name, description, unit)); } @@ -145,7 +150,8 @@ nostd::unique_ptr> Meter::CreateDoubleHistogram( if (!ValidateInstrument(name, description, unit)) { OTEL_INTERNAL_LOG_ERROR("Meter::CreateDoubleHistogram - failed. Invalid parameters." - << name << " " << description << " " << unit); + << name << " " << description << " " << unit + << ". Measurements won't be recorded."); return nostd::unique_ptr>( new metrics::NoopHistogram(name, description, unit)); } @@ -166,7 +172,8 @@ nostd::shared_ptr Meter::CreateInt if (!ValidateInstrument(name, description, unit)) { OTEL_INTERNAL_LOG_ERROR("Meter::CreateInt64ObservableGauge - failed. Invalid parameters." - << name << " " << description << " " << unit); + << name << " " << description << " " << unit + << ". Measurements won't be recorded."); return GetNoopObservableInsrument(); } InstrumentDescriptor instrument_descriptor = { @@ -186,7 +193,8 @@ nostd::shared_ptr Meter::CreateDou if (!ValidateInstrument(name, description, unit)) { OTEL_INTERNAL_LOG_ERROR("Meter::CreateDoubleObservableGauge - failed. Invalid parameters." - << name << " " << description << " " << unit); + << name << " " << description << " " << unit + << ". Measurements won't be recorded."); return GetNoopObservableInsrument(); } InstrumentDescriptor instrument_descriptor = { @@ -206,7 +214,8 @@ nostd::unique_ptr> Meter::CreateInt64UpDownCount if (!ValidateInstrument(name, description, unit)) { OTEL_INTERNAL_LOG_ERROR("Meter::CreateInt64UpDownCounter - failed. Invalid parameters." - << name << " " << description << " " << unit); + << name << " " << description << " " << unit + << ". Measurements won't be recorded."); return nostd::unique_ptr>( new metrics::NoopUpDownCounter(name, description, unit)); } @@ -227,7 +236,8 @@ nostd::unique_ptr> Meter::CreateDoubleUpDownCount if (!ValidateInstrument(name, description, unit)) { OTEL_INTERNAL_LOG_ERROR("Meter::CreateDoubleUpDownCounter - failed. Invalid parameters." - << name << " " << description << " " << unit); + << name << " " << description << " " << unit + << ". Measurements won't be recorded."); return nostd::unique_ptr>( new metrics::NoopUpDownCounter(name, description, unit)); } @@ -248,8 +258,8 @@ Meter::CreateInt64ObservableUpDownCounter(nostd::string_view name, if (!ValidateInstrument(name, description, unit)) { OTEL_INTERNAL_LOG_ERROR( - "Meter::CreateInt64ObservableUpDownCounter - failed. Invalid parameters." - << name << " " << description << " " << unit); + "Meter::CreateInt64ObservableUpDownCounter - failed. Invalid parameters -" + << name << " " << description << " " << unit << ". Measurements won't be recorded."); return GetNoopObservableInsrument(); } InstrumentDescriptor instrument_descriptor = { @@ -270,7 +280,7 @@ Meter::CreateDoubleObservableUpDownCounter(nostd::string_view name, { OTEL_INTERNAL_LOG_ERROR( "Meter::CreateDoubleObservableUpDownCounter - failed. Invalid parameters." - << name << " " << description << " " << unit); + << name << " " << description << " " << unit << ". Measurements won't be recorded."); return GetNoopObservableInsrument(); } InstrumentDescriptor instrument_descriptor = { From f9938cd0fd25d9a44ccdd3c90fcc4ccb7c82eb5d Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 26 Oct 2022 11:35:08 -0700 Subject: [PATCH 05/10] disable clang --- sdk/src/metrics/instrument_metadata_validator.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sdk/src/metrics/instrument_metadata_validator.cc b/sdk/src/metrics/instrument_metadata_validator.cc index bb94b2f5cb..4dcdbfd033 100644 --- a/sdk/src/metrics/instrument_metadata_validator.cc +++ b/sdk/src/metrics/instrument_metadata_validator.cc @@ -23,10 +23,10 @@ const std::string kInstrumentUnitPattern = "[\x01-\x7F]{0,63}"; InstrumentMetaDataValidator::InstrumentMetaDataValidator() # if HAVE_WORKING_REGEX - : name_reg_key_{kInstrumentNamePattern}, unit_reg_key_ -{ - kInstrumentUnitPattern -} + // clang-format off + : name_reg_key_{kInstrumentNamePattern}, + unit_reg_key_{kInstrumentUnitPattern} +// clang-format on # endif {} From 061614718b17f6d96b6566f4ad6102ca6992273c Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 26 Oct 2022 11:59:15 -0700 Subject: [PATCH 06/10] fix gcc4.8 unit tests --- sdk/src/metrics/instrument_metadata_validator.cc | 6 ++++-- sdk/test/metrics/instrument_metadata_validator_test.cc | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/sdk/src/metrics/instrument_metadata_validator.cc b/sdk/src/metrics/instrument_metadata_validator.cc index 4dcdbfd033..ef36a65d5a 100644 --- a/sdk/src/metrics/instrument_metadata_validator.cc +++ b/sdk/src/metrics/instrument_metadata_validator.cc @@ -32,9 +32,11 @@ InstrumentMetaDataValidator::InstrumentMetaDataValidator() bool InstrumentMetaDataValidator::ValidateName(nostd::string_view name) const { + # if HAVE_WORKING_REGEX return std::regex_match(name.data(), name_reg_key_); # else + // size atmost 63 chars if (name.size() > kMaxSize) { @@ -45,9 +47,9 @@ bool InstrumentMetaDataValidator::ValidateName(nostd::string_view name) const { return false; } - // subsequent chars should be either of alphabets, underscore, minus, dot + // subsequent chars should be either of alphabets, digits, underscore, minus, dot return !std::any_of(std::next(name.begin()), name.end(), - [](char c) { return !isalpha(c) && c != '-' && c != '_' && c != '.'; }); + [](char c) { return !isalnum(c) && c != '-' && c != '_' && c != '.'; }); # endif } diff --git a/sdk/test/metrics/instrument_metadata_validator_test.cc b/sdk/test/metrics/instrument_metadata_validator_test.cc index df7e7be594..2b8b993eaf 100644 --- a/sdk/test/metrics/instrument_metadata_validator_test.cc +++ b/sdk/test/metrics/instrument_metadata_validator_test.cc @@ -30,7 +30,7 @@ TEST(InstrumentMetadataValidator, TestName) }; for (auto const &str : invalid_names) { - EXPECT_FALSE(validator.ValidateName(str)); + // EXPECT_FALSE(validator.ValidateName(str)); } std::vector valid_names = { From 766c2dd6093e539933ef8b95cec99d9cba55dd03 Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 26 Oct 2022 12:05:18 -0700 Subject: [PATCH 07/10] fix --- sdk/src/metrics/instrument_metadata_validator.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/src/metrics/instrument_metadata_validator.cc b/sdk/src/metrics/instrument_metadata_validator.cc index ef36a65d5a..e70c98e7ac 100644 --- a/sdk/src/metrics/instrument_metadata_validator.cc +++ b/sdk/src/metrics/instrument_metadata_validator.cc @@ -42,7 +42,7 @@ bool InstrumentMetaDataValidator::ValidateName(nostd::string_view name) const { return false; } - // first char should be alphanumeric + // first char should be alpha if (!isalpha(name[0])) { return false; From cbfceaeff98697e26df043905556f5de53f72fdd Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 26 Oct 2022 13:07:47 -0700 Subject: [PATCH 08/10] fix --- sdk/src/metrics/instrument_metadata_validator.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/src/metrics/instrument_metadata_validator.cc b/sdk/src/metrics/instrument_metadata_validator.cc index e70c98e7ac..3b61c8f791 100644 --- a/sdk/src/metrics/instrument_metadata_validator.cc +++ b/sdk/src/metrics/instrument_metadata_validator.cc @@ -77,4 +77,4 @@ bool InstrumentMetaDataValidator::ValidateDescription(nostd::string_view /*descr } // namespace metrics } // namespace sdk OPENTELEMETRY_END_NAMESPACE -#endif \ No newline at end of file +#endif From c191e6914d470e9444e7e0449391d142811d3238 Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 26 Oct 2022 14:05:34 -0700 Subject: [PATCH 09/10] fix --- sdk/test/metrics/instrument_metadata_validator_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/test/metrics/instrument_metadata_validator_test.cc b/sdk/test/metrics/instrument_metadata_validator_test.cc index 2b8b993eaf..df7e7be594 100644 --- a/sdk/test/metrics/instrument_metadata_validator_test.cc +++ b/sdk/test/metrics/instrument_metadata_validator_test.cc @@ -30,7 +30,7 @@ TEST(InstrumentMetadataValidator, TestName) }; for (auto const &str : invalid_names) { - // EXPECT_FALSE(validator.ValidateName(str)); + EXPECT_FALSE(validator.ValidateName(str)); } std::vector valid_names = { From c22da62eb7cc09c386203d190dc1523adab1e6b7 Mon Sep 17 00:00:00 2001 From: Lalit Date: Wed, 26 Oct 2022 17:07:52 -0700 Subject: [PATCH 10/10] fix more errors --- sdk/src/metrics/instrument_metadata_validator.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/src/metrics/instrument_metadata_validator.cc b/sdk/src/metrics/instrument_metadata_validator.cc index 3b61c8f791..8dd0930e77 100644 --- a/sdk/src/metrics/instrument_metadata_validator.cc +++ b/sdk/src/metrics/instrument_metadata_validator.cc @@ -14,7 +14,6 @@ namespace sdk { namespace metrics { -const size_t kMaxSize = 63; // instrument-name = ALPHA 0*62 ("_" / "." / "-" / ALPHA / DIGIT) const std::string kInstrumentNamePattern = "[a-zA-Z][-_.a-zA-Z0-9]{0,62}"; // @@ -36,7 +35,7 @@ bool InstrumentMetaDataValidator::ValidateName(nostd::string_view name) const # if HAVE_WORKING_REGEX return std::regex_match(name.data(), name_reg_key_); # else - + const size_t kMaxSize = 63; // size atmost 63 chars if (name.size() > kMaxSize) { @@ -58,6 +57,7 @@ bool InstrumentMetaDataValidator::ValidateUnit(nostd::string_view unit) const # if HAVE_WORKING_REGEX return std::regex_match(unit.data(), unit_reg_key_); # else + const size_t kMaxSize = 63; // length atmost 63 chars if (unit.size() > kMaxSize) {