Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix default Metric view name #1515

Merged
merged 13 commits into from
Aug 10, 2022
Merged

Fix default Metric view name #1515

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f908433
Fix default Metric view name
esigo Jul 24, 2022
f6413a7
Merge branch 'main' into Default-Metric-view-name
esigo Jul 25, 2022
d811f1e
fix CI
esigo Jul 25, 2022
d85f08c
Merge branch 'main' into Default-Metric-view-name
esigo Aug 2, 2022
9d4a847
use InstrumentationScope
esigo Aug 2, 2022
1edb4de
fix gcc 4.8 CI
esigo Aug 2, 2022
919e12f
Merge branch 'main' into Default-Metric-view-name
lalitb Aug 2, 2022
f5a07c2
Merge branch 'main' into Default-Metric-view-name
lalitb Aug 2, 2022
79551d0
Merge branch 'main' into Default-Metric-view-name
esigo Aug 4, 2022
eb6c698
Merge branch 'main' into Default-Metric-view-name
esigo Aug 9, 2022
193d9f1
review comment
esigo Aug 9, 2022
757c0fe
use const view
esigo Aug 9, 2022
9e74e43
Merge branch 'main' into Default-Metric-view-name
esigo Aug 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 42 additions & 20 deletions sdk/include/opentelemetry/sdk/metrics/view/view_registry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,29 +46,14 @@ class ViewRegistry
bool FindViews(
const opentelemetry::sdk::metrics::InstrumentDescriptor &instrument_descriptor,
const opentelemetry::sdk::instrumentationscope::InstrumentationScope &instrumentation_scope,
nostd::function_ref<bool(const View &)> callback) const
nostd::function_ref<bool(const View &)> callback)
{
bool found = false;
for (auto const &registered_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;
bool found = findViewsInternal(instrument_descriptor, instrumentation_scope, callback);
// create default view if none found;
if (!found)
{
static View view("otel-default-view");
if (!callback(view))
{
return false;
}
createView(instrument_descriptor, instrumentation_scope);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking if we just create a static view with the empty name:

if (!found)
{
  static View view("");
  . . . 
}

And then this part of code -

opentelemetry-cpp/sdk/src/metrics/meter.cc

Line 212 in 90fd291

view_instr_desc.name_ = view.GetName();
- will not overwrite the name in InstrumentDescriptor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esigo - pinging just in case these comments got missed :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lalitb thanks, I use static view now.

return findViewsInternal(instrument_descriptor, instrumentation_scope, callback);
}
return true;
}
Expand All @@ -95,6 +80,43 @@ 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<bool(const View &)> callback) const
{
bool found = false;
for (auto const &registered_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<opentelemetry::sdk::metrics::InstrumentSelector>{
new opentelemetry::sdk::metrics::InstrumentSelector{instrument_descriptor.type_,
instrument_descriptor.name_}},
std::unique_ptr<opentelemetry::sdk::metrics::MeterSelector>{
new opentelemetry::sdk::metrics::MeterSelector{instrumentation_scope.GetName(),
instrumentation_scope.GetVersion(),
instrumentation_scope.GetSchemaURL()}},
std::unique_ptr<opentelemetry::sdk::metrics::View>{
new opentelemetry::sdk::metrics::View{instrumentation_scope.GetName()}});
}
};
}; // namespace metrics
} // namespace sdk
Expand Down
8 changes: 7 additions & 1 deletion sdk/test/metrics/view_registry_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,17 @@ TEST(ViewRegistry, FindViewsEmptyRegistry)
registry.FindViews(default_instrument_descriptor, *default_instrumentation_scope.get(),
[&count](const View &view) {
count++;
EXPECT_EQ(view.GetName(), "otel-default-view");
# if HAVE_WORKING_REGEX
EXPECT_EQ(view.GetName(), "default");
EXPECT_EQ(view.GetDescription(), "");
# endif
EXPECT_EQ(view.GetAggregationType(), AggregationType::kDefault);
return true;
});
# if HAVE_WORKING_REGEX
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I forgot to create an issue to support this for gcc4.8. Will create one.

EXPECT_EQ(count, 1);
EXPECT_EQ(status, true);
# endif
}

TEST(ViewRegistry, FindNonExistingView)
Expand Down Expand Up @@ -76,7 +80,9 @@ TEST(ViewRegistry, FindNonExistingView)
# endif
return true;
});
# if HAVE_WORKING_REGEX
EXPECT_EQ(count, 1);
EXPECT_EQ(status, true);
# endif
}
#endif