Refactor BatchLogRecordProcessor and associated tests#4535
Merged
lzchen merged 7 commits intoopen-telemetry:mainfrom Apr 24, 2025
Merged
Refactor BatchLogRecordProcessor and associated tests#4535lzchen merged 7 commits intoopen-telemetry:mainfrom
lzchen merged 7 commits intoopen-telemetry:mainfrom
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Refactor BatchLogRecordProcessor, keeping the existing behavior mostly the same. This PR cleans up the code, including the tests, and also adds some new tests.
One exception is
forceFlushwhich now callsexportsynchronously from the main thread and waits for it to finish.Previously
forceFlushwould waittimeout_millisfor the worker thread to make and finish anexportcall, and if an export call was in progress it would wait for the subsequentexportcall to finish. It would returntrueif this export call completed in time andfalseotherwise. It didn't cancel the request after timeout, it just stopped waiting for it to finish.I think ideally
forceFlush.timeout_millis(and alsoshutdown.timeout_millis) should be used as the time after which theexportcall(s) gets cancelled. But for that to work we need to be able to pass a timeout toexportlike what was proposed in #4183. Until then I think we should ignore it and document that it doesn't work.I'm not sure what
forceFlushshould return, currently I have it return nothing (same as javascript. It could always return True, to signify thatexportwas called until the queue was empty. It could return True if allexportcalls succeeded, and False otherwise, and it could stop flushing after the first failed export, like how go lang does it.I think my proposed behavior is more inline with the spec too.
Note that the default for
forceFlush.timeout_milliscame from theOTEL_BLRP_EXPORT_TIMEOUTenvironment variable which is supposed to configure "the maximum allowed time to export data from the BatchLogRecordProcessor". I propose we leave this env var unused for now, and document that it doesn't do anything. This flag seems redundant with the OTLP Exporter timeout env vars anyway. Maybe in other languages the BatchLogRecordProcessor isn't the default one used for auto instrumentation, so it makes more sense for it to be configurable ?Type of change
Please delete options that are not relevant.Please delete options that are not relevant.
How Has This Been Tested?
Added lots of unit tests.
Does This PR Require a Contrib Repo Change?
Checklist: