DISCUSSION: Add SpanWatcherProcessor (spec#373)

[Oberon00]

This is an example implementation of open-telemetry/opentelemetry-specification#373. It depends on #693 to detect ended spans.

The SpanWatcherProcessor is 80% a copy of the BatchSpansProcessor. It uses a list of weak references to detect Spans that were dropped by the user without ending them, as to not cause memory leaks.

It is probably rather slow since it has to use toSpanData on every span. To detect whether it was ended already, an alternative would be to use the onEnd callback, but then the spans would have to be stored in a map structure and some locking would have to take place there. Having an isEnded property on ReadableSpan would be more efficient. The age of the span (getStartEpochNanos) can also only be calculated with the SpanData. Assuming that most spans in applications have a duration of less than the reportInterval of the SpanWatcherProcessor, having just one of hasEnded or getStartEpochNanos exposed on the ReadableSpan should already bring a good improvement.

[codecov-io]

Codecov Report

Merging #697 into master will increase coverage by 0.52%.
The diff coverage is 89.42%.

@@             Coverage Diff              @@
##             master     #697      +/-   ##
============================================
+ Coverage     78.21%   78.74%   +0.52%     
- Complexity      761      769       +8     
============================================
  Files            98       99       +1     
  Lines          2676     2780     +104     
  Branches        247      257      +10     
============================================
+ Hits           2093     2189      +96     
- Misses          484      490       +6     
- Partials         99      101       +2

Impacted Files	Coverage Δ	Complexity Δ
...lemetry/sdk/trace/export/SpanWatcherProcessor.java	89.42% <89.42%> (ø)	7 <7> (?)
...elemetry/sdk/trace/export/BatchSpansProcessor.java	89.36% <0%> (+1.06%)	7% <0%> (ø)	⬇️
...elemetry/opentracingshim/SpanContextShimTable.java	96.42% <0%> (+7.14%)	8% <0%> (+1%)	⬆️

---

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 1e3a5ca...11e35ab. Read the comment docs.

[bogdandrutu]

Some high-level comments.

[bogdandrutu]

Maybe we want to keep the behavior to expose the duration for active (for example in OpenCensus z-pages we want to show for how long the Span was active), we have two options:

• Add a hasEnded on the SpanData and keep the end-time to be Clock::now
• Expose the Clock instance in SpanData.

I tend to like more the first option.

[Oberon00]

That comment would be better at #74 EDIT: #693 😃

Actually I'd like to have the hasEnded on the ReadableSpan already.

[bogdandrutu]

I think this is very similar to https://github.com/open-telemetry/opentelemetry-java/blob/master/sdk/src/main/java/io/opentelemetry/sdk/trace/export/BatchSpansProcessor.java. Is it possible to share at least the Worker/Queuing logic?

[Oberon00]

Yes, as stated in the description, it is probably 80% the same. But the span watcher has no queue, it has a watchlist. Spans are not removed on export. I'm not sure how easily one could share code. Also the types are slightly different since the watcher has to convert to SpanData earlier.

[Oberon00]

As of recently, the conversion to SpanData is now at the same point as in the BatchSpansProcessor, so the types have become even more similar now.

[bogdandrutu]

So if I understand correctly you will try to avoid duplicated code, is that correct?

[Oberon00]

Hey sorry for not replying, I will leave this PR lying around some more while we are still discussing requirements for this span processor (e.g. if it should also optionally export ended spans).

[bogdandrutu]

@Oberon00 is this PR still needed? If you still need time to think we can put a holding_on label, or close and reopen when ready.

[Oberon00]

Hi, I think the PR has become obsolete in the meantime.