[API] Return NoopLogRecord from NoopLogger #2668
Conversation
Nit: I personally don't care but when you have new contributors (e.g. someone just starting in oss) I always try to preserve their own commits, e.g. by branching off of their branch, so that they get proper credit as contributors. E.g. GitHub shows all authors: |
Good point, I need to improve my github skills to do that. Now that 2662 is closed, the branch is deleted so I can't reopen it to use it. @yurishkuro Feel like submitting a 1 line change using the web interface, to get the git credits fixed ? |
|
Do not merge label, waiting for additional commit. Ready for review. |
no it's fine, like I said, I don't care, it was more of a general comment on a nicer way of accepting contributions. |
Thanks. Point taken. |
|
To add to change set upon merge: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2668 +/- ##
==========================================
+ Coverage 87.12% 87.59% +0.47%
==========================================
Files 200 188 -12
Lines 6109 5847 -262
==========================================
- Hits 5322 5121 -201
+ Misses 787 726 -61
|
There was a problem hiding this comment.
For - #2668 (comment)
I see a perf concern here if the SDK is not configured.
I'll take perf concern over segfault. Right now the noop implementation is simply not usable. |
Yes segfault was indeed a bug to be fixed. The fix is unfortunately adding a perf overhead for instrumented libraries when the logging is not enabled, which is not good. Not an issue with this PR, more of the constraint added by the design. I will create a tracking issue for this to be handled separately. |
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
Fixes #2656
Based on PR contribution #2662 from @yurishkuro
Changes
Please provide a brief description of the changes here.
NoopLogger was incorrectly returning nullptr, causing segfaults in client code. After this change it returns a dummy no-op log record.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes