-
Notifications
You must be signed in to change notification settings - Fork 440
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.
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
Fix default Metric view name #1515
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1515 +/- ##
==========================================
+ Coverage 83.85% 83.91% +0.07%
==========================================
Files 156 156
Lines 4908 4908
==========================================
+ Hits 4115 4118 +3
+ Misses 793 790 -3
|
EXPECT_EQ(view.GetAggregationType(), AggregationType::kDefault); | ||
return true; | ||
}); | ||
# if HAVE_WORKING_REGEX |
There was a problem hiding this comment.
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.
{ | ||
return false; | ||
} | ||
createView(instrument_descriptor, instrumentation_scope); |
There was a problem hiding this comment.
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(); |
InstrumentDescriptor
.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
@@ -64,7 +64,7 @@ class ViewRegistry | |||
// return default view if none found; | |||
if (!found) | |||
{ | |||
static View view("otel-default-view"); | |||
static View view(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be declared as static const?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, yes I changed to const
.
Fixes #1510 (issue)
Changes
when the view is not registered, creates and registers a new view with the provided information.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes