Expose log batchconfig

[CosminLazar]

Fixes #1468
Design discussion issue (if applicable) #

Changes

Allows configuring the BatchLogProcessor<T> with standard environment variables or in code via OtlpLogPipeline::with_batch_config.

The change is inspired from OtlpTracePipeline::with_batch_config

Merge requirement checklist

• CONTRIBUTING guidelines followed
• Unit tests added/updated (if applicable)
• Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
• Changes in public API reviewed (if applicable)

Extra discussion

• Now that these two methods of configuration has been introduced, do we still need the with_max_queue_size, with_scheduled_delay, with_max_timeout and with_max_export_batch_size functions on BatchLogProcessorBuilder<E, R>? Same question applies to BatchSpanProcessorBuilder<E, R>.
• The invariant that max_export_batch_size (OTEL_BLRP_MAX_EXPORT_BATCH_SIZE) needs to be less than or equal to max_queue_size (OTEL_BLRP_MAX_QUEUE_SIZE) is enforced in multiple places:
  • BatchConfig implementation of the Default trait
  • with_max_export_batch_size function of BatchLogProcessorBuilder<E, R>

and not at all in BatchConfig (I did it like this to mirror the setup from the BatchConfig of the SpanProcessor). Would it be a better idea to introduce a BatchConfigBuilder?

• The OtlpTracePipeline::with_batch_config also allows configuration from non-standard (to my knowledge) environmental variables, namely OTEL_BSP_EXPORT_TIMEOUT_MILLIS and OTEL_BSP_SCHEDULE_DELAY_MILLIS. I replicated the same setup in this PR for the sake of consistency. Do we want to allow configuration from non-standard environment variables when standard ones exist?

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: CosminLazar / name: Cosmin Lazar (8751ad0, 3c71d08, a170fc8, d8d34e8, d12295f, 055466d, 0dba432, 4afc4ab, fa7d4b5, a28bf82, d37b369, 1f0e6a2, 380d9e4)

[codecov]

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (27d338d) 60.8% compared to head (380d9e4) 61.3%.

Files	Patch %	Lines
opentelemetry-otlp/src/logs.rs	0.0%	13 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1471     +/-   ##
=======================================
+ Coverage   60.8%   61.3%   +0.4%     
=======================================
  Files        146     146             
  Lines      19230   19459    +229     
=======================================
+ Hits       11711   11947    +236     
+ Misses      7519    7512      -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lalitb]

Now that these two methods of configuration has been introduced, do we still need the with_max_queue_size, with_scheduled_delay, with_max_timeout and with_max_export_batch_size functions on BatchLogProcessorBuilder<E, R>? Same question applies to BatchSpanProcessorBuilder<E, R>.

I believe BatchLogProcessorBuilder is still required to configure LoggerProvider/TracerProvider with an instance of BatchSpanProcessor. Something like this:

    let batch = BatchSpanProcessor::builder(
        stdout::new_simple_exporter(), // Using a simple stdout exporter
        trace::runtime::Tokio,         // Using Tokio runtime
    )
    .with_max_queue_size(2048)        // Optional: Configure max queue size
    .with_scheduled_delay(std::time::Duration::from_secs(5))  // Optional: Configure scheduled delay
    .build();

    // Build the TracerProvider with the configured BatchSpanProcessor
    let provider = TracerProvider::builder()
        .with_span_processor(batch)
        .build();

Do we want to allow configuration from non-standard environment variables when standard ones exist?

I thought OTEL_BSP_EXPORT_TIMEOUT_MILLIS and OTEL_BSP_SCHEDULE_DELAY_MILLIS are standard env-variables as specified in the specs - https://github.com/open-telemetry/opentelemetry-specification/blob/main/spec-compliance-matrix.md#environment-variables (refer OTEL_BSP_* and OTEL_BLRP_*). Let me know if I am missing something ?

[CosminLazar]

I might be wrong here, but if I look at the specs https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#batch-logrecord-processor for both Batch Span Processor and Batch LogRecord Processor I cannot find any references to OTEL_BSP_EXPORT_TIMEOUT_MILLIS, OTEL_BSP_SCHEDULE_DELAY_MILLIS, OTEL_BLRP_EXPORT_TIMEOUT_MILLIS or OTEL_BLRP_SCHEDULE_DELAY_MILLIS only the variants without the _MILLIS postfix are shown there.

[lalitb]

I might be wrong here, but if I look at the specs https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#batch-logrecord-processor for both Batch Span Processor and Batch LogRecord Processor I cannot find any references to OTEL_BSP_EXPORT_TIMEOUT_MILLIS, OTEL_BSP_SCHEDULE_DELAY_MILLIS, OTEL_BLRP_EXPORT_TIMEOUT_MILLIS or OTEL_BLRP_SCHEDULE_DELAY_MILLIS only the variants without the _MILLIS postfix are shown there.

Ok yeah. I don't have the background for using non-standard OTELP_BSP_*_MILLIS env-variables, perhaps for backward compatibility. I think this can be discussed and removed as separate PR if all agree since we are still not 1.0. Don't see that a blocker for this PR.

[lalitb]

Now that these two methods of configuration has been introduced, do we still need the with_max_queue_size, with_scheduled_delay, with_max_timeout and with_max_export_batch_size functions on BatchLogProcessorBuilder<E, R>? Same question applies to BatchSpanProcessorBuilder<E, R>.

Thinking about this again, I agree it would make sense to remove these functions from BatchLogProcessorBuilder, and just use BatchConfig. Something like:

let batch_config = BatchConfig::builder()
                              .with_max_queue_size(200)
                             .with_scheduled_delay(std::time::Duration::from_secs(5))
                             .build();
let batch_processor = BatchLogProcessor::builder(exporter, runtime)
                             .with_batch_config(batch_config)
                             .build();
provider = LoggerProvider::builder().with_log_processor(batch_processor);

Good to have a minimal API/SDK surface, and remove redundant configuration options. Do you want to create separate PR for both spans and logs for this, as it would be a breaking change for the customer already using this interface?

[CosminLazar]

Yes, a new PR sounds like the way forward here. I will look into it.

[lalitb]

LGTM, the changes are consistent with the BatchSpanProcessor. There are some cleanups discussed in the comments, which can be done as separate PR. Thanks for the PR

[TommyCpp]

LGTM. Can we also add CHANGELOGS?

[CosminLazar]

Will do!

[CosminLazar]

@TommyCpp Entry in the CHANGELOG was added.
Thanks for the review!

[cijothomas]

@TommyCpp Entry in the CHANGELOG was added. Thanks for the review!

Thanks for your contribution! Would love to see more contribs, if you are interested!