Repository navigation
Conversation
…_emit A failing LogRecordProcessor.on_emit no longer skips the remaining processors registered on the synchronous or concurrent multi processor. Fixes open-telemetry#5841
Pull request dashboard statusWaiting on the author · refreshed 2026-11-05 01:28 UTC Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):
Status above doesn't look right?
|
| mock.on_emit.assert_called_with(record) | ||
| multi_log_record_processor.shutdown() | ||
|
|
||
| def test_on_emit_exception_does_not_stop_other_processors(self): |
There was a problem hiding this comment.
Can you add a test with in-memory processors that fully integrates with a failing log processor and validates that telemetry is produced correctly end-to-end for non-erroring processors?
There was a problem hiding this comment.
yeah makes sense, I added one that wires up a real LoggerProvider with a processor that throws in the middle and in memory exporters before and after it so you can see the records actually come out the other side, it runs for both sync and concurrent, just pushed it
There was a problem hiding this comment.
🟡 Changes recommended
Failure diagnostics can recursively re-enter the failing processor or deadlock behind a root LoggingHandler, requiring a non-propagating internal logger and regression coverage.
1 open issue
What changed in this PR
This PR addresses #5841 by adding exception handling around child log processors’ on_emit calls, aiming to keep processor failures from interrupting application logging or delivery to other processors.
Changes:
- Catch and log child processor exceptions in synchronous and concurrent processing.
- Add a shared regression test verifying that other processors still receive the record.
- Add a changelog fragment documenting the fix.
| File | Description |
|---|---|
opentelemetry-sdk/tests/logs/test_multi_log_processor.py |
Tests exception reporting and continued delivery for both processor types. |
opentelemetry-sdk/src/opentelemetry/sdk/_logs/_internal/__init__.py |
Adds exception guards around child on_emit calls. |
.changelog/5846.fixed |
Documents the log processor exception-handling change. |
Review effort: Balanced
| try: | ||
| lp.on_emit(log_record) | ||
| except Exception: # pylint: disable=broad-exception-caught | ||
| _logger.exception("Exception while calling on_emit on %s.", lp) |
There was a problem hiding this comment.
I think this one was real, with a LoggingHandler on root the exception log from on_emit went right back through the same handler so it could loop and the concurrent one could block on the handler lock. I switched both to a logger that doesn't propagate, same as what SimpleLogRecordProcessor already does, and added a test with a root handler and a failing processor for sync and concurrent. pushed with the other test
|
@hippi345 Please sign the CLA. |
…ot handlers Route processor on_emit exception diagnostics through a non-propagating internal logger, matching SimpleLogRecordProcessor export error handling. Add LoggerProvider integration and root LoggingHandler regression tests for synchronous and concurrent multi processors. Co-authored-by: Joel Town Road <hippi345@users.noreply.github.com>
…on shutdown Release ThreadPoolExecutor worker threads when ConcurrentMultiLogRecordProcessor shuts down so PyPy test runs can exit cleanly. Ensure force_flush test shuts down its processor.
3e40cfe to
7fe8e99
Compare
|
Hi @hippi345 — just a friendly reminder that this pull request is waiting on you. The dashboard status comment has the open items and is kept current.
|
Changes
SynchronousMultiLogRecordProcessorandConcurrentMultiLogRecordProcessornow catch exceptions from each childLogRecordProcessor.on_emit, log them, and continue with the other processors. One failing processor no longer crashes the caller or drops the record for processors registered after it.Fixes #5841
Checklist