Skip to content

Refactor BatchLogRecordProcessor and associated tests#4535

Merged
lzchen merged 7 commits intoopen-telemetry:mainfrom
DylanRussell:refactor_blrp
Apr 24, 2025
Merged

Refactor BatchLogRecordProcessor and associated tests#4535
lzchen merged 7 commits intoopen-telemetry:mainfrom
DylanRussell:refactor_blrp

Conversation

@DylanRussell
Copy link
Contributor

@DylanRussell DylanRussell commented Apr 9, 2025

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 forceFlush which now calls export synchronously from the main thread and waits for it to finish.

Previously forceFlush would wait timeout_millis for the worker thread to make and finish an export call, and if an export call was in progress it would wait for the subsequent export call to finish. It would return true if this export call completed in time and false otherwise. It didn't cancel the request after timeout, it just stopped waiting for it to finish.

I think ideally forceFlush.timeout_millis (and also shutdown.timeout_millis) should be used as the time after which the export call(s) gets cancelled. But for that to work we need to be able to pass a timeout to export like 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 forceFlush should return, currently I have it return nothing (same as javascript. It could always return True, to signify that export was called until the queue was empty. It could return True if all export calls 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_millis came from the OTEL_BLRP_EXPORT_TIMEOUT environment 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.

  • [ X] Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Added lots of unit tests.

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • [ x] No.

Checklist:

  • [x ] Followed the style guidelines of this project
  • Changelogs have been updated
  • [x ] Unit tests have been added
  • Documentation has been updated

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Skip Changelog PRs that do not require a CHANGELOG.md entry

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants