Ensure is_recording is false after span ends

[jtescher]

See open-telemetry/opentelemetry-specification#1011 for details.

[morigs]

It seems currently span can be ended in two ways:

• when goes out of scope (using drop)
• when end_with_timestamp is called

Right now is_recording handles only the first case. So I would like to suggest just to replace this line with something like this:

match self.with_data(|data| data.end_time.is_some()) {
  Some(ended) => !ended,
  None => false,
}

[jtescher]

@morigs yeah additionally it may be a good idea to move the logic from drop to end_with_timestamp so other methods like setting attributes are no-ops as defined by the spec

[morigs]

@jtescher I think it's better not to move logic from drop because this will not allow to get SpanContext after end.
As an alternative I could suggest adding additional check to every method (check end_time for example)

[morigs]

Oh-oh, I forgot that end_time is always set. May be we should define some new bool (is_ended) and set it in end?

[jtescher]

Actually it's probably preferable to move span context outside the mutex, would also allow span context to return a reference instead of a clone which would be an improvement. I have a branch that does this that I could clean up and push.

[morigs]

I have added some tests in #309
Some of them related to this issue

[morigs]

@jtescher Hi. I'm working on moving logic from drop to end_with_timestamp. Two points here:

• We still must end span on drop. So I've replaced impl Drop for SpanInner with impl Drop for Span because SpanInner doesn't have end method
• end_with_timestamp will require mutable reference to take ownership of inner.data. This conflicts with api::trace::span::Span trait. Is it ok to change Span trait in the api?

Click to expand

error[E0053]: method `end_with_timestamp` has an incompatible type for trait
   --> opentelemetry\src\sdk\trace\span.rs:142:27
    |
142 |     fn end_with_timestamp(&mut self, timestamp: SystemTime) {
    |                           ^^^^^^^^^ types differ in mutability        
    | 
   ::: opentelemetry\src\api\trace\span.rs:165:27
    |
165 |     fn end_with_timestamp(&self, timestamp: SystemTime);
    |                           ----- type in trait
    |
    = note: expected fn pointer `fn(&sdk::trace::span::Span, std::time::SystemTime)`
               found fn pointer `fn(&mut sdk::trace::span::Span, std::time::SystemTime)`

[jtescher]

@morigs I believe drop has to be on SpanInner because you can have many clones of a span and the first drop does not necessarily imply span end. It should be possible currently if you move the logic in drop to a function that can be called from span or on inner drop. Maybe something along the lines of:

impl SpanInner {
    fn end_with_timestamp(&self, timestamp: Option<SystemTime>) {
        // Calling end on unsampled span or already ended span is ignored
        if let Some(data) = &self.data {
            if let Ok(mut span_data) = data.lock().map(|mut data| data.take()) {

                // Ensure end time is set via explicit end or implicitly on drop
                if let Some(span_data) = span_data.as_mut() {
                    if let Some(timestamp) = timestamp {
                        span_data.end_time = timestamp;
                    } else if span_data.end_time == span_data.start_time {
                        span_data.end_time = SystemTime::now();
                    }
                }

                // Notify each span processor that the span has ended
                if let Some(provider) = self.tracer.provider() {
                    let mut processors = provider.span_processors().iter().peekable();
                    while let Some(processor) = processors.next() {
                        let span_data = if processors.peek().is_none() {
                            // last loop or single processor/exporter, move data
                            span_data.take()
                        } else {
                            // clone so each exporter gets owned data
                            span_data.clone()
                        };

                        if let Some(span_data) = span_data {
                            processor.on_end(build_export_data(
                                span_data,
                                self.span_context.clone(),
                                &self.tracer,
                            ));
                        }
                    }
                }
            }
        }
    }
}