-
Notifications
You must be signed in to change notification settings - Fork 438
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
Logger-Support-for-Instrumentation-library #1149
Logger-Support-for-Instrumentation-library #1149
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1149 +/- ##
==========================================
+ Coverage 93.29% 93.29% +0.01%
==========================================
Files 174 174
Lines 6402 6404 +2
==========================================
+ Hits 5972 5974 +2
Misses 430 430
|
private: | ||
// The name of this logger | ||
std::string logger_name_; | ||
|
||
// The logger provider of this Logger. Uses a weak_ptr to avoid cyclic dependency issues the with | ||
// logger provider | ||
std::weak_ptr<LoggerProvider> logger_provider_; | ||
std::shared_ptr<instrumentationlibrary::InstrumentationLibrary> instrumentation_library_; |
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.
I understand this is coming from tracer implementation, but now thinking why do store it as shared_ptr
instead of unique_ptr, or as value. We are anyway passing it to exporters as reference.
*/ | ||
virtual void SetInstrumentationLibrary( | ||
const opentelemetry::sdk::instrumentationlibrary::InstrumentationLibrary | ||
&instrumentation_library) noexcept = 0; |
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.
We need to call this method from within Logger::Log() for exporters to be able to read this. Do we plan to have it as separate PR?
Co-authored-by: Lalit Kumar Bhasin <[email protected]>
Co-authored-by: Lalit Kumar Bhasin <[email protected]>
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.
LGTM. Thanks for the changes.
@ThomsonTan - Can you pls have a look to this PR. Thanks. |
api/test/logs/provider_test.cc
Outdated
auto logger2 = tf->GetLogger("logger2", args); | ||
std::array<nostd::string_view, 1> sv{"string"}; | ||
nostd::span<nostd::string_view> args{sv}; | ||
auto logger2 = tf->GetLogger("logger2", args, "lib_name", "", schema_url); |
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.
nit: use another fake library name like opentelelemtry_library
and make it consistent in all the tests?
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.
done
Would it be possible to rerun the CI jobs? |
Fixes #1087 (issue)
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes