search

open-telemetry / opentelemetry-cpp

The OpenTelemetry C++ Client
https://opentelemetry.io/
Apache License 2.0
815 stars 391 forks source link

[API] Return NoopLogRecord from NoopLogger #2668

Closed marcalff closed 1 month ago

marcalff commented 1 month ago

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:

yurishkuro commented 1 month ago

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:

image
marcalff commented 1 month ago

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 commented 1 month ago

Do not merge label, waiting for additional commit.

Ready for review.

yurishkuro commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

To add to change set upon merge:

Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
codecov[bot] commented 1 month ago

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 [![Impacted file tree graph](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2668/graphs/tree.svg?width=650&height=150&src=pr&token=FJESTYQ2AD&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2668?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) ```diff @@ 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](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2668?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry) | Coverage Δ | | |---|---|---| | [api/include/opentelemetry/logs/event\_logger.h](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2668?src=pr&el=tree&filepath=api%2Finclude%2Fopentelemetry%2Flogs%2Fevent_logger.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-YXBpL2luY2x1ZGUvb3BlbnRlbGVtZXRyeS9sb2dzL2V2ZW50X2xvZ2dlci5o) | `90.91% <ø> (-1.39%)` | :arrow_down: | | [api/include/opentelemetry/logs/logger.h](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2668?src=pr&el=tree&filepath=api%2Finclude%2Fopentelemetry%2Flogs%2Flogger.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-YXBpL2luY2x1ZGUvb3BlbnRlbGVtZXRyeS9sb2dzL2xvZ2dlci5o) | `63.89% <ø> (-0.69%)` | :arrow_down: | | [sdk/src/logs/logger.cc](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2668?src=pr&el=tree&filepath=sdk%2Fsrc%2Flogs%2Flogger.cc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-c2RrL3NyYy9sb2dzL2xvZ2dlci5jYw==) | `80.56% <ø> (+1.61%)` | :arrow_up: | | [api/include/opentelemetry/logs/noop.h](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2668?src=pr&el=tree&filepath=api%2Finclude%2Fopentelemetry%2Flogs%2Fnoop.h&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry#diff-YXBpL2luY2x1ZGUvb3BlbnRlbGVtZXRyeS9sb2dzL25vb3AuaA==) | `77.78% <64.29%> (-0.79%)` | :arrow_down: | ... and [55 files with indirect coverage changes](https://app.codecov.io/gh/open-telemetry/opentelemetry-cpp/pull/2668/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=open-telemetry)
yurishkuro commented 1 month ago

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 commented 1 month ago

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.