Change SpanExporter::export &self to &mut self

[frigus02]

This is a proposal. I think it would make sense to change &self in the SpanExporter::export function to a &mut self. The spec says

Export() will never be called concurrently for the same exporter instance. Export() can be called again only after the current call returns.

This change would enforce the restriction at compile time, which is nice.

While doing this change I noticed that the SimpleSpanProcessor doesn't adhere to this. It calls export in on_end without any checks and on_end can be called from multiple threads concurrently. I changed it to use a Mutex for now.

As a second step I removed the Sync requirement for SpanExporter. We can now do this without any problems, because both of it's functions take a &mut self. This means the stdout exporter no longer needs an internal Mutex. I'm not entirely sure about this change, though. Is it possible for the spec to have a non-breaking change, that would require us to add Sync back (which would be a breaking change)?

[codecov]

Codecov Report

Merging #350 (4916c2c) into master (fd8cf65) will increase coverage by 0.39%.
The diff coverage is 91.22%.

@@            Coverage Diff             @@
##           master     #350      +/-   ##
==========================================
+ Coverage   53.81%   54.21%   +0.39%     
==========================================
  Files          69       69              
  Lines        5779     5829      +50     
==========================================
+ Hits         3110     3160      +50     
  Misses       2669     2669              

Impacted Files	Coverage Δ
opentelemetry-otlp/src/span.rs	2.67% <ø> (ø)
opentelemetry/src/api/trace/noop.rs	68.93% <0.00%> (ø)
opentelemetry/src/exporter/trace/mod.rs	70.27% <ø> (ø)
opentelemetry/src/exporter/trace/stdout.rs	23.07% <ø> (-5.50%)	⬇️
opentelemetry/src/testing/trace.rs	72.91% <91.42%> (+49.83%)	⬆️
opentelemetry/src/sdk/trace/span_processor.rs	77.65% <95.23%> (+8.42%)	⬆️
...ntelemetry/src/sdk/metrics/aggregators/ddsketch.rs	78.59% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd8cf65...4916c2c. Read the comment docs.

[TommyCpp]

Looks great! Just a small nit.

[jtescher]

I think this is a good idea, should be able to remove the inner mutexes from the exporters now too

e.g.

opentelemetry-rust/opentelemetry-jaeger/src/agent.rs

Lines 40 to 43 in 03fe982

	conn: tokio::sync::Mutex<tokio::net::UdpSocket>,
	#[cfg(all(feature = "async-std", not(feature = "tokio")))]
	conn: async_std::sync::Mutex<async_std::net::UdpSocket>,
	buffer_client: Mutex<BufferClient>,

[frigus02]

should be able to remove the inner mutexes from the exporters now too

I think I looked at all exporters except Jaeger. Didn't think it would contain mutexes 🤦. Will remove those, too.

[jtescher]

Looks good! I think the jaeger exporter buffer clone can probably be removed now too, but can be done in a subsequent pr

[frigus02]

Yeah, I think we should be able to remove that. But I'm not entirely sure how. We want the thrift client to write into a buffer, which we can later read. What's the pattern we would typically use in Rust? A channel?

[jtescher]

Would be nice if the thrift crate had a cleaner way of writing and reading since it does not do either one in parallel, but the current API only lets you split. In that case it would ordinarily be fine to have a Rc<RefCell<T>>, but we need this processing to be spawned so it has to be Send. Even if we stay with Arc<Mutex<T>> we should still be able to lock, send data to the socket and then clear the vec, as opposed to clone then send data via reference to the cloned vec, but std mutexs don't play well across await points. This could use the async mutex types that were used before behind feature flags though.