[On Hold] Improve behavior of un-ended ReadableSpan #693

Oberon00 · 2019-12-12T11:59:21Z

Original title: Simplify span end time.

The only reason to have now() returned in getEndEpochNanos seems to be
that it kinda makes sense for getLatencyMs, but that method is
package-private and never used.

See also
open-telemetry/opentelemetry-specification#373 (comment).

codecov-io · 2019-12-12T12:05:04Z

Codecov Report

Merging #693 into master will decrease coverage by 0.04%.
The diff coverage is 85%.

@@             Coverage Diff              @@
##             master     #693      +/-   ##
============================================
- Coverage     78.42%   78.38%   -0.05%     
  Complexity      746      746              
============================================
  Files            93       93              
  Lines          2609     2618       +9     
  Branches        234      239       +5     
============================================
+ Hits           2046     2052       +6     
- Misses          466      467       +1     
- Partials         97       99       +2

Impacted Files	Coverage Δ	Complexity Δ
...ntelemetry/sdk/trace/RecordEventsReadableSpan.java	`89.65% <100%> (+0.18%)`	`49 <1> (ø)`	⬇️
...main/java/io/opentelemetry/sdk/trace/SpanData.java	`89.28% <40%> (-10.72%)`	`2 <0> (ø)`
...telemetry/contrib/metrics/runtime/MemoryPools.java	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...etry/contrib/metrics/runtime/GarbageCollector.java	`0% <0%> (ø)`	`0% <0%> (ø)`	⬇️
...elemetry/sdk/trace/export/BatchSpansProcessor.java	`89.36% <0%> (+1.06%)`	`7% <0%> (ø)`	⬇️

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 7505c12...a030e1b. Read the comment docs.

sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java

Oberon00 · 2019-12-16T11:28:37Z

I'm not sure we need the latency on the span data, but I'll add it in a separate commit for now.

Add a getter to Span and ReadableSpan to allow checking if the span is ended without using a private field or rely on internals like span.duration[0] < 0. Refs: open-telemetry/opentelemetry-java#693 Refs: open-telemetry/opentelemetry-java#697

The only reason to have now() returned in getEndEpochNanos seems to be that it kinda makes sense for getLatencyMs, but that method is package-private and never used. See also open-telemetry/opentelemetry-specification#373 (comment).

sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java

Oberon00 · 2019-12-16T12:34:25Z

@arminru You probably want to re-review that, since a lot has changed since your approval.

In-reply-to: open-telemetry#693 (comment)

Oberon00 · 2019-12-16T14:28:40Z

Actually, when adding the latency to SpanData it breaks all the tests in #697 because now the latency is among the values that are compared when comparing SpanData objects, and calling toSpanData on an un-ended span will have a different value there every time. That's fixable but cumbersome. I wonder if we really want to have the latency at the SpanData or if the ReadableSpan is enough.

sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java

This reverts commit b3e419a.

Oberon00 · 2019-12-19T13:46:08Z

@open-telemetry/java-approvers Anyone care to look at this? Do you think I should really add the latency to SpanData?

jkwatson · 2019-12-19T16:36:52Z

sdk/src/main/java/io/opentelemetry/sdk/trace/ReadableSpan.java

+  boolean hasEnded();
+
+  /**
+   * Returns the latency of the {@code Span} in nanos. If still active then returns now() - start


I think it would be nice in this javadoc to define "latency" in this context, because it is a word that some may interpret as meaning "network latency", which seems like an odd thing to have on a generic Span.

Actually I think it might be better to just rename this to currentDuration. Latency, while technically correct (see open-telemetry/opentelemetry-specification#330), is probably confusing to most users.

Or maybe "elapsedTime" ?

jkwatson · 2019-12-19T16:39:05Z

sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java

@@ -190,6 +192,13 @@ public SpanData toSpanData() {
    return result;
  }

+  @Override
+  public boolean hasEnded() {
+    synchronized (lock) {


this proliferation of synchronization really makes me want to implement this with AtomicBoolean, since it's lock-free.

You could remove the lock here then, but all the checks for hasEnded within span still need to hold the lock if they want to ensure that they don't set properties after the span has ended.

yeah. IMO, the whole thread-safety of this class is pretty janky. We should think about separating the mutable parts from the immutable and having a simpler synchronization model.

jkwatson · 2019-12-19T16:39:44Z

sdk/src/main/java/io/opentelemetry/sdk/trace/SpanData.java

+   * @return {@code true} if the span has already been ended, {@code false} if not.
+   * @since 0.4.0
+   */
+  public abstract boolean getHasEnded();


I recommend just "hasEnded" without the extraneous get

Unfortunately, AutoValue seems to require a get-prefix here (see also getHasRemoteParent).

yet another reason for me to hate autovalue

I'm trying to decide if isEnded is better, or just as ugly

isFinished ?

I think isFinished sounds like it would be true if the span is exported (finished passing through the span pipeline). Maybe a name for the inverse could be used, like isRunning.

I like that idea!

isActive? just to throw out another option

Being "active" (on a scope) is already another concept in OpenTelemetry, unfortunately.

I like isRunning

jkwatson · 2019-12-19T16:41:29Z

sdk/src/main/java/io/opentelemetry/sdk/trace/SpanData.java

+      if (!getLatencyNanos().isPresent()
+          && getStartEpochNanos().isPresent()
+          && getEndEpochNanos().isPresent()
+          && getEndEpochNanos().get() >= getStartEpochNanos().get()) {


this is really ugly block of code. Can we at least move it into it's own method?

Actually, this is only required for tests. The best option from my perspective would be to remove the latency from SpanData.

I am fine with removing latency from the SpanData. For the EndEpoch what is the behavior you want?

bogdandrutu · 2019-12-25T16:27:56Z

@Oberon00 I think this PR got very hard to review, and I cannot follow all the changes, can you please consider to: split PR if possible in changes to ReadableSpan and changes to SpanData, update the description of the PR and documented what changed.

In-reply-to: open-telemetry#693 (comment)

Oberon00 · 2020-01-07T09:49:13Z

I split #733 from this one, will do further splitting once that one is handled.

* Add hasBeenEnded property to ReadableSpan. * Add hasBeenEnded to SpanData. * Rename hasBeenEnded to hasEnded. In-reply-to: #693 (comment) * Check SpanData.hasEnded in unit test. * Fix mismerge.

bogdandrutu · 2020-01-07T22:52:15Z

Please rebase to include the latest merged PR.

Oberon00 · 2020-01-08T13:58:42Z

Closing this one in favor of #737.

Add a getter to Span and ReadableSpan to allow checking if the span is ended without using a private field or rely on internals like span.duration[0] < 0. Refs: open-telemetry/opentelemetry-java#693 Refs: open-telemetry/opentelemetry-java#697

* feat: introduce ended property on Span Add a getter to Span and ReadableSpan to allow checking if the span is ended without using a private field or rely on internals like span.duration[0] < 0. Refs: open-telemetry/opentelemetry-java#693 Refs: open-telemetry/opentelemetry-java#697 * chore: adapt after merge from master * chore: revert changes in API * chore: remove unneeded isEnded

Oberon00 force-pushed the simplify-span-endtime branch from 79b9fe0 to 4582da8 Compare December 12, 2019 12:01

Oberon00 marked this pull request as ready for review December 12, 2019 12:06

Oberon00 requested review from arminru, bogdandrutu, carlosalberto, jkwatson, pavolloffay, songy23 and tylerbenson as code owners December 12, 2019 12:06

arminru approved these changes Dec 12, 2019

View reviewed changes

Oberon00 changed the title ~~Simplify span end time.~~ Simplify span end time (changes semantics of end time of active span) Dec 12, 2019

bogdandrutu reviewed Dec 13, 2019

View reviewed changes

sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java Show resolved Hide resolved

Oberon00 mentioned this pull request Dec 13, 2019

DISCUSSION: Add SpanWatcherProcessor (spec#373) #697

Closed

Oberon00 changed the title ~~Simplify span end time (changes semantics of end time of active span)~~ Improve behavior of un-ended ReadableSpan Dec 16, 2019

Flarna mentioned this pull request Dec 16, 2019

feat: introduce ended property on Span open-telemetry/opentelemetry-js#625

Merged

Oberon00 added 7 commits December 16, 2019 13:29

Simplify span end time.

6de98a6

The only reason to have now() returned in getEndEpochNanos seems to be that it kinda makes sense for getLatencyMs, but that method is package-private and never used. See also open-telemetry/opentelemetry-specification#373 (comment).

Fix toSpanData synchronization (fixes open-telemetry#699).

b3e419a

Add back getLatencyNs, add it to ReadableSpan.

346a47b

Rename getLatencyNs to getLatencyNanos.

3688ccb

Add hasBeenEnded property to ReadableSpan.

95ec8b0

Add hasBeenEnded to SpanData.

07e7a3b

Add latencyNanos to SpanData.

9875df5

Oberon00 force-pushed the simplify-span-endtime branch from 65f0592 to 9875df5 Compare December 16, 2019 12:29

Oberon00 commented Dec 16, 2019

View reviewed changes

sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java Outdated Show resolved Hide resolved

Oberon00 requested a review from arminru December 16, 2019 12:34

Rename hasBeenEnded to hasEnded.

d3153a3

In-reply-to: open-telemetry#693 (comment)

bogdandrutu reviewed Dec 16, 2019

View reviewed changes

sdk/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java Outdated Show resolved Hide resolved

Revert "Fix toSpanData synchronization (fixes open-telemetry#699)."

997aaf8

This reverts commit b3e419a.

Oberon00 force-pushed the simplify-span-endtime branch from a1fc2b6 to b7114f0 Compare December 17, 2019 10:34

Oberon00 added 2 commits December 17, 2019 11:39

Check SpanData.hasEnded in unit test.

425edcc

Check SpanData.latencyNanos in unit test.

a030e1b

Oberon00 force-pushed the simplify-span-endtime branch from b7114f0 to a030e1b Compare December 17, 2019 10:40

jkwatson reviewed Dec 19, 2019

View reviewed changes

Oberon00 added a commit to dynatrace-oss-contrib/opentelemetry-java that referenced this pull request Jan 7, 2020

Rename hasBeenEnded to hasEnded.

29176c1

In-reply-to: open-telemetry#693 (comment)

Oberon00 added a commit to dynatrace-oss-contrib/opentelemetry-java that referenced this pull request Jan 7, 2020

Rename hasBeenEnded to hasEnded.

8484a63

In-reply-to: open-telemetry#693 (comment)

Oberon00 mentioned this pull request Jan 7, 2020

Add hasEnded to ReadableSpan and SpanData (split from #693) #733

Merged

Oberon00 changed the title ~~Improve behavior of un-ended ReadableSpan~~ [On Hold] Improve behavior of un-ended ReadableSpan Jan 7, 2020

Oberon00 mentioned this pull request Jan 8, 2020

Rework latency/duration/endTime of Span #737

Merged

Oberon00 closed this Jan 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[On Hold] Improve behavior of un-ended ReadableSpan #693

[On Hold] Improve behavior of un-ended ReadableSpan #693

Oberon00 commented Dec 12, 2019 •

edited

Loading

codecov-io commented Dec 12, 2019 •

edited

Loading

Oberon00 commented Dec 16, 2019

Oberon00 commented Dec 16, 2019

Oberon00 commented Dec 16, 2019 •

edited

Loading

Oberon00 commented Dec 19, 2019

jkwatson Dec 19, 2019

Oberon00 Dec 20, 2019

jkwatson Dec 20, 2019

jkwatson Dec 19, 2019

Oberon00 Dec 20, 2019

jkwatson Dec 20, 2019

jkwatson Dec 19, 2019

Oberon00 Dec 20, 2019

jkwatson Dec 20, 2019

jkwatson Dec 20, 2019

jkwatson Dec 20, 2019

Oberon00 Dec 20, 2019

jkwatson Dec 20, 2019

trask Dec 20, 2019

Oberon00 Dec 20, 2019

bogdandrutu Dec 25, 2019

jkwatson Dec 19, 2019

Oberon00 Dec 20, 2019

bogdandrutu Dec 25, 2019

bogdandrutu commented Dec 25, 2019

Oberon00 commented Jan 7, 2020

bogdandrutu commented Jan 7, 2020

Oberon00 commented Jan 8, 2020

[On Hold] Improve behavior of un-ended ReadableSpan #693

[On Hold] Improve behavior of un-ended ReadableSpan #693

Conversation

Oberon00 commented Dec 12, 2019 • edited Loading

codecov-io commented Dec 12, 2019 • edited Loading

Codecov Report

Oberon00 commented Dec 16, 2019

Oberon00 commented Dec 16, 2019

Oberon00 commented Dec 16, 2019 • edited Loading

Oberon00 commented Dec 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu commented Dec 25, 2019

Oberon00 commented Jan 7, 2020

bogdandrutu commented Jan 7, 2020

Oberon00 commented Jan 8, 2020

Oberon00 commented Dec 12, 2019 •

edited

Loading

codecov-io commented Dec 12, 2019 •

edited

Loading

Oberon00 commented Dec 16, 2019 •

edited

Loading