Remove mut self reference from LogExporter::export() method.

[lalitb]

Changes

As discussed here - #2374 (comment)
The changes are minimal, and the stress test for the logs now includes an exporter call over the future, causing a slight regression.

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)

[codecov]

Codecov Report

Attention: Patch coverage is 15.78947% with 16 lines in your changes missing coverage. Please review.

Project coverage is 79.4%. Comparing base (957659f) to head (87f984f).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
opentelemetry-stdout/src/logs/exporter.rs	0.0%	7 Missing ⚠️
opentelemetry-otlp/src/exporter/tonic/logs.rs	0.0%	5 Missing ⚠️
opentelemetry-appender-tracing/src/layer.rs	0.0%	1 Missing ⚠️
opentelemetry-otlp/src/exporter/http/logs.rs	0.0%	1 Missing ⚠️
opentelemetry-otlp/src/logs.rs	0.0%	1 Missing ⚠️
opentelemetry-sdk/src/logs/log_processor.rs	66.6%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #2380   +/-   ##
=====================================
  Coverage   79.4%   79.4%           
=====================================
  Files        123     123           
  Lines      21479   21484    +5     
=====================================
+ Hits       17064   17068    +4     
- Misses      4415    4416    +1     

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

[cijothomas]

I think this is the right direction. Our processors are only requiring self and not mut self, so there seems to be no strong reason for exporter methods to require mut self. Also looked at tracing-subscriber - its on_event method is also just requiring self , and it lets layers manage its state via interior mutability.

Requiring mut self would prevent exporters's export() method from being called in parallel. While this is a spec requirement now, we expect to allow exporters to be called parallel in certain situations. Instead of adding another trait for that, it seems okay to use self itself and let exporters handle state via interior mutability, if they need to.

Would love to get more feedback on this, as we are close to declaring stability, so can't make breaking changes after that.

[lalitb]

agree, would prefer to have closer on this sooner, the slight regression seen in stress test can then be evaluated for improvement with #2374.

[scottgerring]

Cool! LGTM. As a ordinary user of the library I'd prefer for any "incidental mutability" of a particular exporter to be handled internally, because it makes my life easier. I reckon as users of the library implementing custom exporters will be a much smaller set of folks, it makes sense for the ergonomics to focus on improving the lives of the former group.

A couple of minor observations that don't think really affect the PR.

It's a bit unfortunate that tonic only provides a (&mut self...) for the interceptor trait call. As far as I can tell the only place we're using the interceptor is in add_metadata, and it's definitely not mutating anything on itself. The lock is necessary to keep the borrow-checker happy, but only because of this.

I see PushMetricExporter doesn't have a mutable export fn ✅ - but SpanExporter still does - is the story there different, or should this drop the &mut self too?

opentelemetry-rust/opentelemetry-sdk/src/export/trace.rs

Lines 20 to 33 in 957659f

	pub trait SpanExporter: Send + Sync + Debug {
	/// Exports a batch of readable spans. Protocol exporters that will
	/// implement this function are typically expected to serialize and transmit
	/// the data to the destination.
	///
	/// This function will never be called concurrently for the same exporter
	/// instance. It can be called again only after the current call returns.
	///
	/// This function must not block indefinitely, there must be a reasonable
	/// upper limit after which the call must time out with an error result.
	///
	/// Any retry logic that is required by the exporter is the responsibility
	/// of the exporter.
	fn export(&mut self, batch: Vec<SpanData>) -> BoxFuture<'static, ExportResult>;

[cijothomas]

nit: This is no longer NoOp. Maybe rename?

[lalitb]

yes, changed to Mock.

[lalitb]

It's a bit unfortunate that tonic only provides a (&mut self...) for the interceptor trait call. As far as I can tell the only place we're using the interceptor is in add_metadata, and it's definitely not mutating anything on itself. The lock is necessary to keep the borrow-checker happy, but only because of this.

Thanks @scottgerring for the review. That's the only unfortunate aspect of this change, which we discussed in the last community meeting - exporters need to acquire an uncontested lock for their internal mutable state during each export to satisfy the borrow checker. That said, we agreed the overhead should be minimal, likely no more than an atomic operation.

[lalitb]

I see PushMetricExporter doesn't have a mutable export fn ✅ - but SpanExporter still does - is the story there different, or should this drop the &mut self too?

Yes, I believe we should do the similar change for SpanExporter too.

[cijothomas]

It's a bit unfortunate that tonic only provides a (&mut self...) for the interceptor trait call. As far as I can tell the only place we're using the interceptor is in add_metadata, and it's definitely not mutating anything on itself. The lock is necessary to keep the borrow-checker happy, but only because of this.

Thanks @scottgerring for the review. That's the only unfortunate aspect of this change, which we discussed in the last community meeting - exporters need to acquire an uncontested lock for their internal mutable state during each export to satisfy the borrow checker. That said, we agreed the overhead should be minimal, likely no more than an atomic operation.

Can exporters leverage other interior mutability techniques (RefCell) other than mutex locks?

[TommyCpp]

Yes, I believe we should do the similar change for SpanExporter too.

+1 on keeping then consistent

[TommyCpp]

LGTM. We have to handle mutability somewhere along the call stack. Having it in the exporter seems fine.

[lalitb]

Can exporters leverage other interior mutability techniques (RefCell) other than mutex locks?

Yes, I believe exporters could use either mutexes or RefCell here for interior mutability, with similar overhead. Uncontested mutexes will involve atomic operations, while RefCell relies on lightweight runtime borrowing checks.

[scottgerring]

The only reason it needs the mutex at the moment is that trait we're using to add metadata has the mut on it. I wonder if there's a different way to put the metadata on or meet the interface we expose, without using that tonic extension point?

To be clear - I think it's important to get this in to get the interface locked down, so i'm not suggesting this as part of this change - just thinking out loud!

[cijothomas]

LGTM.