From f90843313286b724097b4a314d55dc28a6316581 Mon Sep 17 00:00:00 2001 From: Oblivion Date: Sun, 24 Jul 2022 14:02:57 +0000 Subject: [PATCH 1/6] Fix default Metric view name --- .../sdk/metrics/view/view_registry.h | 64 +++++++++++++------ sdk/test/metrics/view_registry_test.cc | 2 +- 2 files changed, 45 insertions(+), 21 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h index 795049dd9a..ce9615fc63 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h @@ -46,29 +46,15 @@ class ViewRegistry bool FindViews(const opentelemetry::sdk::metrics::InstrumentDescriptor &instrument_descriptor, const opentelemetry::sdk::instrumentationlibrary::InstrumentationLibrary &instrumentation_library, - nostd::function_ref callback) const + nostd::function_ref callback) { - bool found = false; - for (auto const ®istered_view : registered_views_) - { - if (MatchMeter(registered_view->meter_selector_.get(), instrumentation_library) && - MatchInstrument(registered_view->instrument_selector_.get(), instrument_descriptor)) - { - found = true; - if (!callback(*(registered_view->view_.get()))) - { - return false; - } - } - } - // return default view if none found; + bool found = findViewsInternal(instrument_descriptor, instrumentation_library, callback); + // create default view if none found; if (!found) { - static View view("otel-default-view"); - if (!callback(view)) - { - return false; - } + createView(instrument_descriptor, instrumentation_library); + return findViewsInternal(instrument_descriptor, instrumentation_library, callback); + ; } return true; } @@ -95,6 +81,44 @@ class ViewRegistry return selector->GetNameFilter()->Match(instrument_descriptor.name_) && (selector->GetInstrumentType() == instrument_descriptor.type_); } + + bool findViewsInternal( + const opentelemetry::sdk::metrics::InstrumentDescriptor &instrument_descriptor, + const opentelemetry::sdk::instrumentationlibrary::InstrumentationLibrary + &instrumentation_library, + nostd::function_ref callback) const + { + bool found = false; + for (auto const ®istered_view : registered_views_) + { + if (MatchMeter(registered_view->meter_selector_.get(), instrumentation_library) && + MatchInstrument(registered_view->instrument_selector_.get(), instrument_descriptor)) + { + found = true; + if (!callback(*(registered_view->view_.get()))) + { + return false; + } + } + } + return found; + } + + void createView(const opentelemetry::sdk::metrics::InstrumentDescriptor &instrument_descriptor, + const opentelemetry::sdk::instrumentationlibrary::InstrumentationLibrary + &instrumentation_library) + { + AddView( + std::unique_ptr{ + new opentelemetry::sdk::metrics::InstrumentSelector{instrument_descriptor.type_, + instrument_descriptor.name_}}, + std::unique_ptr{ + new opentelemetry::sdk::metrics::MeterSelector{instrumentation_library.GetName(), + instrumentation_library.GetVersion(), + instrumentation_library.GetSchemaURL()}}, + std::unique_ptr{ + new opentelemetry::sdk::metrics::View{instrumentation_library.GetName()}}); + } }; }; // namespace metrics } // namespace sdk diff --git a/sdk/test/metrics/view_registry_test.cc b/sdk/test/metrics/view_registry_test.cc index 8151d37545..38d0c5e2a5 100644 --- a/sdk/test/metrics/view_registry_test.cc +++ b/sdk/test/metrics/view_registry_test.cc @@ -29,7 +29,7 @@ TEST(ViewRegistry, FindViewsEmptyRegistry) registry.FindViews(default_instrument_descriptor, *default_instrumentation_lib.get(), [&count](const View &view) { count++; - EXPECT_EQ(view.GetName(), "otel-default-view"); + EXPECT_EQ(view.GetName(), "default"); EXPECT_EQ(view.GetDescription(), ""); EXPECT_EQ(view.GetAggregationType(), AggregationType::kDefault); return true; From d811f1e5508de5ac772853403dcde60be1275441 Mon Sep 17 00:00:00 2001 From: Oblivion Date: Mon, 25 Jul 2022 20:02:18 +0000 Subject: [PATCH 2/6] fix CI --- sdk/test/metrics/view_registry_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/sdk/test/metrics/view_registry_test.cc b/sdk/test/metrics/view_registry_test.cc index 38d0c5e2a5..17cba953e7 100644 --- a/sdk/test/metrics/view_registry_test.cc +++ b/sdk/test/metrics/view_registry_test.cc @@ -29,8 +29,10 @@ TEST(ViewRegistry, FindViewsEmptyRegistry) registry.FindViews(default_instrument_descriptor, *default_instrumentation_lib.get(), [&count](const View &view) { count++; +# if HAVE_WORKING_REGEX EXPECT_EQ(view.GetName(), "default"); EXPECT_EQ(view.GetDescription(), ""); +# endif EXPECT_EQ(view.GetAggregationType(), AggregationType::kDefault); return true; }); From 9d4a847fc7511b987a1e92e2ea0255cc096b7090 Mon Sep 17 00:00:00 2001 From: Oblivion Date: Tue, 2 Aug 2022 17:18:41 +0000 Subject: [PATCH 3/6] use InstrumentationScope --- .../sdk/metrics/view/view_registry.h | 40 ++++++------------- 1 file changed, 13 insertions(+), 27 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h index 00b481dbb5..b9f7320897 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h @@ -48,25 +48,12 @@ class ViewRegistry const opentelemetry::sdk::instrumentationscope::InstrumentationScope &instrumentation_scope, nostd::function_ref callback) { - bool found = false; - for (auto const ®istered_view : registered_views_) - { - if (MatchMeter(registered_view->meter_selector_.get(), instrumentation_library) && - MatchInstrument(registered_view->instrument_selector_.get(), instrument_descriptor)) - { - found = true; - if (!callback(*(registered_view->view_.get()))) - { - return false; - } - } - } - // return default view if none found; + bool found = findViewsInternal(instrument_descriptor, instrumentation_scope, callback); + // create default view if none found; if (!found) { - createView(instrument_descriptor, instrumentation_library); - return findViewsInternal(instrument_descriptor, instrumentation_library, callback); - ; + createView(instrument_descriptor, instrumentation_scope); + return findViewsInternal(instrument_descriptor, instrumentation_scope, callback); } return true; } @@ -96,14 +83,13 @@ class ViewRegistry bool findViewsInternal( const opentelemetry::sdk::metrics::InstrumentDescriptor &instrument_descriptor, - const opentelemetry::sdk::instrumentationlibrary::InstrumentationLibrary - &instrumentation_library, + const opentelemetry::sdk::instrumentationscope::InstrumentationScope &instrumentation_scope, nostd::function_ref callback) const { bool found = false; for (auto const ®istered_view : registered_views_) { - if (MatchMeter(registered_view->meter_selector_.get(), instrumentation_library) && + if (MatchMeter(registered_view->meter_selector_.get(), instrumentation_scope) && MatchInstrument(registered_view->instrument_selector_.get(), instrument_descriptor)) { found = true; @@ -116,20 +102,20 @@ class ViewRegistry return found; } - void createView(const opentelemetry::sdk::metrics::InstrumentDescriptor &instrument_descriptor, - const opentelemetry::sdk::instrumentationlibrary::InstrumentationLibrary - &instrumentation_library) + void createView( + const opentelemetry::sdk::metrics::InstrumentDescriptor &instrument_descriptor, + const opentelemetry::sdk::instrumentationscope::InstrumentationScope &instrumentation_scope) { AddView( std::unique_ptr{ new opentelemetry::sdk::metrics::InstrumentSelector{instrument_descriptor.type_, instrument_descriptor.name_}}, std::unique_ptr{ - new opentelemetry::sdk::metrics::MeterSelector{instrumentation_library.GetName(), - instrumentation_library.GetVersion(), - instrumentation_library.GetSchemaURL()}}, + new opentelemetry::sdk::metrics::MeterSelector{instrumentation_scope.GetName(), + instrumentation_scope.GetVersion(), + instrumentation_scope.GetSchemaURL()}}, std::unique_ptr{ - new opentelemetry::sdk::metrics::View{instrumentation_library.GetName()}}); + new opentelemetry::sdk::metrics::View{instrumentation_scope.GetName()}}); } }; }; // namespace metrics From 1edb4de777e0887a2c69ee4936c17514f0abf22a Mon Sep 17 00:00:00 2001 From: Oblivion Date: Tue, 2 Aug 2022 17:32:34 +0000 Subject: [PATCH 4/6] fix gcc 4.8 CI --- sdk/test/metrics/view_registry_test.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/sdk/test/metrics/view_registry_test.cc b/sdk/test/metrics/view_registry_test.cc index 9ea096ace9..c4905a28cf 100644 --- a/sdk/test/metrics/view_registry_test.cc +++ b/sdk/test/metrics/view_registry_test.cc @@ -36,8 +36,10 @@ TEST(ViewRegistry, FindViewsEmptyRegistry) EXPECT_EQ(view.GetAggregationType(), AggregationType::kDefault); return true; }); +# if HAVE_WORKING_REGEX EXPECT_EQ(count, 1); EXPECT_EQ(status, true); +# endif } TEST(ViewRegistry, FindNonExistingView) @@ -78,7 +80,9 @@ TEST(ViewRegistry, FindNonExistingView) # endif return true; }); +# if HAVE_WORKING_REGEX EXPECT_EQ(count, 1); EXPECT_EQ(status, true); +# endif } #endif From 193d9f174b001efeca1ef060a416eb41fa2d384f Mon Sep 17 00:00:00 2001 From: Ehsan Saei <71217171+esigo@users.noreply.github.com> Date: Tue, 9 Aug 2022 19:01:05 +0200 Subject: [PATCH 5/6] review comment --- .../sdk/metrics/view/view_registry.h | 62 ++++++------------- sdk/test/metrics/view_registry_test.cc | 2 +- 2 files changed, 21 insertions(+), 43 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h index b9f7320897..df97403dfb 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h @@ -46,14 +46,29 @@ class ViewRegistry bool FindViews( const opentelemetry::sdk::metrics::InstrumentDescriptor &instrument_descriptor, const opentelemetry::sdk::instrumentationscope::InstrumentationScope &instrumentation_scope, - nostd::function_ref callback) + nostd::function_ref callback) const { - bool found = findViewsInternal(instrument_descriptor, instrumentation_scope, callback); - // create default view if none found; + bool found = false; + for (auto const ®istered_view : registered_views_) + { + if (MatchMeter(registered_view->meter_selector_.get(), instrumentation_scope) && + MatchInstrument(registered_view->instrument_selector_.get(), instrument_descriptor)) + { + found = true; + if (!callback(*(registered_view->view_.get()))) + { + return false; + } + } + } + // return default view if none found; if (!found) { - createView(instrument_descriptor, instrumentation_scope); - return findViewsInternal(instrument_descriptor, instrumentation_scope, callback); + static View view(""); + if (!callback(view)) + { + return false; + } } return true; } @@ -80,43 +95,6 @@ class ViewRegistry return selector->GetNameFilter()->Match(instrument_descriptor.name_) && (selector->GetInstrumentType() == instrument_descriptor.type_); } - - bool findViewsInternal( - const opentelemetry::sdk::metrics::InstrumentDescriptor &instrument_descriptor, - const opentelemetry::sdk::instrumentationscope::InstrumentationScope &instrumentation_scope, - nostd::function_ref callback) const - { - bool found = false; - for (auto const ®istered_view : registered_views_) - { - if (MatchMeter(registered_view->meter_selector_.get(), instrumentation_scope) && - MatchInstrument(registered_view->instrument_selector_.get(), instrument_descriptor)) - { - found = true; - if (!callback(*(registered_view->view_.get()))) - { - return false; - } - } - } - return found; - } - - void createView( - const opentelemetry::sdk::metrics::InstrumentDescriptor &instrument_descriptor, - const opentelemetry::sdk::instrumentationscope::InstrumentationScope &instrumentation_scope) - { - AddView( - std::unique_ptr{ - new opentelemetry::sdk::metrics::InstrumentSelector{instrument_descriptor.type_, - instrument_descriptor.name_}}, - std::unique_ptr{ - new opentelemetry::sdk::metrics::MeterSelector{instrumentation_scope.GetName(), - instrumentation_scope.GetVersion(), - instrumentation_scope.GetSchemaURL()}}, - std::unique_ptr{ - new opentelemetry::sdk::metrics::View{instrumentation_scope.GetName()}}); - } }; }; // namespace metrics } // namespace sdk diff --git a/sdk/test/metrics/view_registry_test.cc b/sdk/test/metrics/view_registry_test.cc index c4905a28cf..a6b0115f42 100644 --- a/sdk/test/metrics/view_registry_test.cc +++ b/sdk/test/metrics/view_registry_test.cc @@ -30,7 +30,7 @@ TEST(ViewRegistry, FindViewsEmptyRegistry) [&count](const View &view) { count++; # if HAVE_WORKING_REGEX - EXPECT_EQ(view.GetName(), "default"); + EXPECT_EQ(view.GetName(), ""); EXPECT_EQ(view.GetDescription(), ""); # endif EXPECT_EQ(view.GetAggregationType(), AggregationType::kDefault); From 757c0fee77c7b63e58ed3adadf39977f511149d5 Mon Sep 17 00:00:00 2001 From: Oblivion Date: Tue, 9 Aug 2022 17:15:32 +0000 Subject: [PATCH 6/6] use const view --- sdk/include/opentelemetry/sdk/metrics/view/view_registry.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h index df97403dfb..41755d23bf 100644 --- a/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h +++ b/sdk/include/opentelemetry/sdk/metrics/view/view_registry.h @@ -64,7 +64,7 @@ class ViewRegistry // return default view if none found; if (!found) { - static View view(""); + static const View view(""); if (!callback(view)) { return false;