Update LogRecord batching processor behavior description #4409
Conversation
|
…scription of maxExportBatchSize consistent with Trace for clarity
Hopefully soon yes, waiting on it. |
…ded for previous format then not cleaned up correctly)
…cation into issue-4402
pellared
changed the title
pellared
added
area:sdk
|
Please also update the PR description as it is outdated. |
specification/logs/sdk.md
Outdated
| - `scheduledDelayMillis` after the processor is constructed OR the first `LogRecord` | ||
| is received by the log record processor. |
There was a problem hiding this comment.
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
LogRecordis received by the log record processor.
There was a problem hiding this comment.
I was bringing this documentation in line with how the trace batching processor describes behaviour. I'm happy to make changes (with direction) in this PR or could there be another issue to address this part of the specification for both logs and trace batching?
My original issue was with environment variables not referring back to related parts of the SDKs, and us finding this "missing" content for logs is extras that have been suggested as improvements. Typical case of the more you look, the more you find :)
There was a problem hiding this comment.
I see that you were trying to get the spec from Trace and copy it to Log. But IMHO, the trace section itself is not very clear. (I am sure I have discussed this before, but not able to find that discussion anymore!!)
https://github.com/open-telemetry/opentelemetry-specification/pull/4409/files#r1968141138 - This comment has my suggested wording for both trace/log. See if it is simpler and yet conveys the full intent.
specification/logs/sdk.md
Outdated
| 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` |
There was a problem hiding this comment.
the first
LogRecord
is received by the log record processor.
Why export when first record is received 🤔 ?
There was a problem hiding this comment.
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.
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
specification/logs/sdk.md
Outdated
| @@ -425,6 +425,21 @@ representations to the configured `LogRecordExporter`. | |||
| The processor MUST synchronize calls to `LogRecordExporter`'s `Export` | |||
| to make sure that they are not invoked concurrently. | |||
|
|
|||
| The processor SHOULD export a batch when any of the following happens AND the | |||
There was a problem hiding this comment.
Batching processor
This is an implementation of the LogRecordProcessor which create batches
of LogRecords and passes the export-friendly ReadableLogRecord
representations to the configured LogRecordExporter.
The processor MUST synchronize calls to LogRecordExporter's Export
to make sure that they are not invoked concurrently.
A batch SHOULD be exported when any of the following conditions are met:
- The scheduled delay (scheduledDelayMillis) elapses.
- The queue contains at least maxExportBatchSize LogRecords.
- ForceFlush is called.
- Shutdown is called.
If the queue is empty when an export is triggered, the processor MAY either export an empty batch or skip the export and consider it complete immediately.
^Wording suggestion.
Additional notes from implementing this:
I don't think the spec has explicitly clarified the following, so I went with my own interpretations!
- Does time taken for export be considered? For example, should export gets triggered at consistent times T,T+d,T+2d,T+2d
Or
T, T+e1+d, T+e2+2d... where e1,e2 represents the time take for the export itself - What happens if export() takes more time than the interval itself? Should the next export() be skipped? or attempted?
|
I've unpicked the changes that are in discussion for wider general improvements and raised a follow up issue referencing the discussions here #4434. This PR now just adds the links as needed and hopefully is OK. Sorry, still waiting on CLA... |
|
@cijothomas I raised #4434 |
|
@JDUNNIN can you clear the CLA?
|
From #4409 (comment):
|
Fixes #4402
Changes