Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update LogRecord batching processor behavior description #4409

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

JDUNNIN
Copy link

@JDUNNIN JDUNNIN commented Feb 10, 2025

Fixes #4402

Changes

  • Update both Batch Span Processor and Batch LogRecord Processor to refer to the respective Trace and Logs SDK topics on batch processors.
  • Update the logs SDK Batching processor description to include more information on SHOULD based guidance and also update maxExportBatchSize to provide more information.
  • Add in missing Shutdown is called guidance in Trace and Logs.

@JDUNNIN JDUNNIN requested review from a team as code owners February 10, 2025 13:22
Copy link

linux-foundation-easycla bot commented Feb 10, 2025

CLA Not Signed

@JDUNNIN JDUNNIN requested a review from cijothomas February 11, 2025 12:36
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you sign the CLA?

specification/logs/sdk.md Show resolved Hide resolved
specification/configuration/sdk-environment-variables.md Outdated Show resolved Hide resolved
@JDUNNIN
Copy link
Author

JDUNNIN commented Feb 14, 2025

Can you sign the CLA?

Hopefully soon yes, waiting on it.

@JDUNNIN JDUNNIN requested a review from pellared February 14, 2025 11:48
specification/logs/sdk.md Show resolved Hide resolved
@pellared pellared changed the title Add more description to OTEL_BSP_SCHEDULE_DELAY and OTEL_BLRP_SCHEDUL… Update LogRecord batching processor behavior description Feb 14, 2025
@pellared pellared added area:sdk Related to the SDK spec:logs Related to the specification/logs directory spec:trace Related to the specification/trace directory clarification clarify ambiguity in specification labels Feb 14, 2025
@pellared
Copy link
Member

Please also update the PR description as it is outdated.

Comment on lines +427 to +428
- `scheduledDelayMillis` after the processor is constructed OR the first `LogRecord`
is received by the log record processor.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we want to have the 2nd part? Asking this question more coming from "why do we want to have complex rules instead of simple rules".

  • the first LogRecord is received by the log record processor.

The processor SHOULD export a batch when any of the following happens AND the
previous export call has returned:

- `scheduledDelayMillis` after the processor is constructed OR the first `LogRecord`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the first LogRecord
is received by the log record processor.

Why export when first record is received 🤔 ?

Copy link
Member

@pellared pellared Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the original author meant "delay after first record'. Probably this could be clarified, but I do not feel it has to be done in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK clarification clarify ambiguity in specification spec:logs Related to the specification/logs directory spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more description for OTEL_BSP_SCHEDULE_DELAY
4 participants