[API] Return NoopLogRecord from NoopLogger

[marcalff]

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.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[yurishkuro]

Based on PR contribution #2662 from @yurishkuro

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:

[marcalff]

Based on PR contribution #2662 from @yurishkuro

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.

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 ?

[marcalff]

Do not merge label, waiting for additional commit.

Ready for review.

[yurishkuro]

Feel like submitting a 1 line change using the web interface, to get the git credits fixed ?

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.

[marcalff]

Feel like submitting a 1 line change using the web interface, to get the git credits fixed ?

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.

[marcalff]

To add to change set upon merge:

Co-authored-by: Yuri Shkuro <[email protected]>

[codecov]

Codecov Report

Attention: Patch coverage is 64.28571% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 87.59%. Comparing base (497eaf4) to head (75ec527).
Report is 63 commits behind head on main.

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     

Files	Coverage Δ
api/include/opentelemetry/logs/event_logger.h	90.91% <ø> (-1.39%)	⬇️
api/include/opentelemetry/logs/logger.h	63.89% <ø> (-0.69%)	⬇️
sdk/src/logs/logger.cc	80.56% <ø> (+1.61%)	⬆️
api/include/opentelemetry/logs/noop.h	77.78% <64.29%> (-0.79%)	⬇️

... and 55 files with indirect coverage changes

[lalitb]

For - #2668 (comment)

I see a perf concern here if the SDK is not configured.

[yurishkuro]

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.

[lalitb]

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.