[On Hold] Improve behavior of un-ended ReadableSpan

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

@@ -34,6 +34,7 @@
 import io.opentelemetry.trace.Tracer;
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.logging.Level;
@@ -159,24 +160,29 @@ static RecordEventsReadableSpan startSpan(
   @Override
   public SpanData toSpanData() {
     SpanContext spanContext = getSpanContext();
-    return SpanData.newBuilder()
-        .setName(getName())
-        .setInstrumentationLibraryInfo(instrumentationLibraryInfo)
-        .setTraceId(spanContext.getTraceId())
-        .setSpanId(spanContext.getSpanId())
-        .setTraceFlags(spanContext.getTraceFlags())
-        .setTracestate(spanContext.getTracestate())
-        .setAttributes(getAttributes())
-        .setStartEpochNanos(startEpochNanos)
-        .setEndEpochNanos(getEndEpochNanos())
-        .setKind(kind)
-        .setLinks(getLinks())
-        .setParentSpanId(parentSpanId)
-        .setHasRemoteParent(hasRemoteParent)
-        .setResource(resource)
-        .setStatus(getStatus())
-        .setTimedEvents(adaptTimedEvents())
-        .build();
+    // Set immutable info outside the lock.
+    SpanData.Builder builder =
+        SpanData.newBuilder()
+            .setInstrumentationLibraryInfo(instrumentationLibraryInfo)
+            .setTraceId(spanContext.getTraceId())
+            .setSpanId(spanContext.getSpanId())
+            .setTraceFlags(spanContext.getTraceFlags())
+            .setTracestate(spanContext.getTracestate())
+            .setStartEpochNanos(startEpochNanos)
+            .setKind(kind)
+            .setLinks(getLinks())
+            .setParentSpanId(parentSpanId)
+            .setHasRemoteParent(hasRemoteParent)
+            .setResource(resource);
+    synchronized (lock) {
+      builder
+          .setName(getName())
+          .setAttributes(hasBeenEnded ? attributes : new HashMap<>(attributes))
+          .setEndEpochNanos(getEndEpochNanos())
+          .setStatus(getStatus())
+          .setTimedEvents(adaptTimedEvents());
+    }
+    return builder.build();
   }
 
   private List<SpanData.TimedEvent> adaptTimedEvents() {
@@ -220,14 +226,14 @@ public InstrumentationLibraryInfo getInstrumentationLibraryInfo() {
   }
 
   /**
-   * Returns the end nano time (see {@link System#nanoTime()}). If the current {@code Span} is not
-   * ended then returns {@link Clock#nanoTime()}.
+   * Returns the end nano time (see {@link System#nanoTime()}) or zero if the current {@code Span}
+   * is not ended.
    *
    * @return the end nano time.
    */
   private long getEndEpochNanos() {
     synchronized (lock) {
-      return getEndNanoTimeInternal();
+      return endEpochNanos;
     }
   }
 
@@ -261,22 +267,20 @@ private List<TimedEvent> getTimedEvents() {
    */
   @VisibleForTesting
   List<Link> getLinks() {
-    synchronized (lock) {
-      if (links == null) {
-        return Collections.emptyList();
-      }
-      List<Link> result = new ArrayList<>(links.size());
-      for (Link link : links) {
-        Link newLink = link;
-        if (!(link instanceof SpanData.Link)) {
-          // Make a copy because the given Link may not be immutable and we may reference a lot of
-          // memory.
-          newLink = SpanData.Link.create(link.getContext(), link.getAttributes());
-        }
-        result.add(newLink);
+    if (links == null) {
+      return Collections.emptyList();
+    }
+    List<Link> result = new ArrayList<>(links.size());
+    for (Link link : links) {
+      Link newLink = link;
+      if (!(link instanceof SpanData.Link)) {
+        // Make a copy because the given Link may not be immutable and we may reference a lot of
+        // memory.
+        newLink = SpanData.Link.create(link.getContext(), link.getAttributes());
       }
-      return Collections.unmodifiableList(result);
+      result.add(newLink);
     }
+    return Collections.unmodifiableList(result);
   }
 
   /**
@@ -291,24 +295,6 @@ Map<String, AttributeValue> getAttributes() {
     }
   }
 
-  /**
-   * Returns the latency of the {@code Span} in nanos. If still active then returns now() - start
-   * time.
-   *
-   * @return the latency of the {@code Span} in nanos.
-   */
-  long getLatencyNs() {
-    synchronized (lock) {
-      return getEndNanoTimeInternal() - startEpochNanos;
-    }
-  }
-
-  // Use getEndNanoTimeInternal to avoid over-locking.
-  @GuardedBy("lock")
-  private long getEndNanoTimeInternal() {
-    return hasBeenEnded ? endEpochNanos : clock.now();
-  }
-
   /**
    * Returns the kind of this {@code Span}.
    *

sdk/src/test/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpanTest.java

@@ -134,7 +134,7 @@ public void toSpanData_ActiveSpan() {
           Collections.singletonList(link),
           SPAN_NEW_NAME,
           startEpochNanos,
-          testClock.now(),
+          0,
           Status.OK);
     } finally {
       span.end();
@@ -243,32 +243,6 @@ public void getAndUpdateSpanName() {
     }
   }
 
-  @Test
-  public void getLatencyNs_ActiveSpan() {
-    RecordEventsReadableSpan span = createTestSpan(Kind.INTERNAL);
-    try {
-      testClock.advanceMillis(MILLIS_PER_SECOND);
-      long elapsedTimeNanos1 = testClock.now() - startEpochNanos;
-      assertThat(span.getLatencyNs()).isEqualTo(elapsedTimeNanos1);
-      testClock.advanceMillis(MILLIS_PER_SECOND);
-      long elapsedTimeNanos2 = testClock.now() - startEpochNanos;
-      assertThat(span.getLatencyNs()).isEqualTo(elapsedTimeNanos2);
-    } finally {
-      span.end();
-    }
-  }
-
-  @Test
-  public void getLatencyNs_EndedSpan() {
-    RecordEventsReadableSpan span = createTestSpan(Kind.INTERNAL);
-    testClock.advanceMillis(MILLIS_PER_SECOND);
-    span.end();
-    long elapsedTimeNanos = testClock.now() - startEpochNanos;
-    assertThat(span.getLatencyNs()).isEqualTo(elapsedTimeNanos);
-    testClock.advanceMillis(MILLIS_PER_SECOND);
-    assertThat(span.getLatencyNs()).isEqualTo(elapsedTimeNanos);
-  }
-
   @Test
   public void setAttribute() {
     RecordEventsReadableSpan span = createTestRootSpan();