[LOGS] Fix Logger::EmitLogRecord unsafe static_cast to Recordable

[om7057]

Fixes #4537

Changes

While a Logger is disabled, CreateLogRecord() returned a kNoopLogger-created NoopLogRecord, which is not the SDK's internal Recordable. EmitLogRecord() re-checks the enabled state at emit time rather than trusting what it was at creation time, so enabling the logger in between (e.g. via LoggerProvider::UpdateLoggerConfigurator()) let that NoopLogRecord reach EmitLogRecord()'s unconditional static_cast<Recordable *>, and then MultiLogRecordProcessor::OnEmit()'s own static_cast<MultiRecordable *>, both undefined behavior.

The issue's suggested fix uses dynamic_cast, but that is not an option here: this project supports building with RTTI disabled, and the Bazel nortti CI job builds and tests the whole tree (including sdk/logs) with -fno-rtti, where a dynamic_cast on a polymorphic type does not compile.

Per the design discussion on #4537, CreateLogRecord() (both the no-argument overload and the ABI v2 context_or_span overload) now returns an empty MultiRecordable while the logger is disabled, instead of a NoopLogRecord. This needs no detection at emit time: an empty MultiRecordable is a real Recordable, so both static_casts above are always safe, and its Set* calls and ReleaseRecordable() simply loop over zero wrapped recordables, so the record is dropped without ever reaching a real processor. No API or ABI change.

Added LoggerSDK.EmitLogRecordSafeWhenEnabledBetweenCreateAndEmit, which creates a record through both CreateLogRecord() overloads while the logger is disabled, enables the logger, and emits both, asserting no crash and that nothing reaches the processor. Ran the full sdk/logs and api/logs test suites under both ABI v1 and ABI v2 with no regressions.

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.53%. Comparing base (ad74ba2) to head (5e36b31).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4624      +/-   ##
==========================================
+ Coverage   86.53%   86.53%   +0.01%     
==========================================
  Files         525      525              
  Lines       20475    20475              
==========================================
+ Hits        17715    17717       +2     
+ Misses       2760     2758       -2     

Files with missing lines	Coverage Δ
sdk/src/logs/logger.cc	95.10% <100.00%> (+0.99%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lalitb]

Thanks for working on this. I agree with the empty MultiRecordable approach discussed in #4537. Let's rework this along those lines, with a regression test for enabling the logger between record creation and emission.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: om7057 / name: Om Kulkarni (07a0e47, 225e51a, 63a307b, b72a588, ef64107)

[om7057]

Reworked to the empty MultiRecordable approach from #4537: Logger::CreateLogRecord() (both overloads) now returns a bare MultiRecordable() with no wrapped recordables while the logger is disabled, instead of kNoopLogger.CreateLogRecord(). This needs no detection at emit time, since Set* and ReleaseRecordable() on an empty MultiRecordable already loop over zero children, so MultiLogRecordProcessor::OnEmit()'s static_cast is always safe regardless of whether the logger got enabled in between.

Reverted the LogRecord::IsRecordable() virtual and the EmitLogRecord() guard entirely, so there's no API/ABI change now.

Replaced the old test with EmitLogRecordSafeWhenEnabledBetweenCreateAndEmit, which creates a record while disabled, enables the logger via UpdateLoggerConfigurator(), then emits it, and asserts no crash and no data reaching the processor. Verified locally under both ABIv1 and ABIv2 (10 and 19 tests respectively in logger_sdk_test), plus the full sdk/logs and api/logs suites (78 tests total), all passing.

cc: @lalitb

[mateenali66]

Built main with only your test file added and it dies with SIGBUS, exit 138. Same test on your head passes, so it does regress in a plain debug build.

The ABI v2 CreateLogRecord overload takes the same change, but nothing reaches it while disabled, since the test uses the no argument one. Worth a second create under the v2 guard in the same test?

On credit, yes please, and thanks for asking.

[om7057]

Thanks for verifying the crash reproduces on main. Added v2 coverage in a1bcbf4c: the test now also creates a record through CreateLogRecord(context_or_span) while disabled and emits it after enabling, alongside the existing no-argument case. 19/19 pass under ABIv2, 10/10 under ABIv1 with the v2 branch compiled out.

Added you as co-author on that commit.

[lalitb]

LGTM, thanks for the update and coverage for both overloads. Could you also refresh the PR description to match the current implementation?

[om7057]

PR description updated to describe the current empty-MultiRecordable implementation instead of the original IsRecordable() approach.