Introduce an ReadableAttributes interface, with 2 implementations (#1336

)

* Introduce an Attributes interface and have ImmutableAttributes and the AttributesMap implement it.

* make the attribute limiting a little clearer.

* tiny javadoc fix

* Rework to restore the Attributes class, and introduce a ReadableAttributes interface

* polish the javadoc a bit

* fix some broken javadoc

* revert example change; revert test name change

* javadoc de-escalation

* fix bad merge

api/src/main/java/io/opentelemetry/common/Attributes.java

@@ -33,7 +33,8 @@
  * <p>The keys are {@link String}s and the values are {@link AttributeValue} instances.
  */
 @Immutable
-public abstract class Attributes extends ImmutableKeyValuePairs<AttributeValue> {
+public abstract class Attributes extends ImmutableKeyValuePairs<AttributeValue>
+    implements ReadableAttributes {
   private static final Attributes EMPTY = Attributes.newBuilder().build();
 
   @AutoValue

api/src/main/java/io/opentelemetry/common/ReadableAttributes.java

@@ -0,0 +1,44 @@
+/*
+ * Copyright 2020, OpenTelemetry Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.opentelemetry.common;
+
+import javax.annotation.Nullable;
+
+/**
+ * A read-only container for String-keyed attributes.
+ *
+ * <p>See {@link Attributes} for the public API implementation.
+ */
+public interface ReadableAttributes {
+  /** The number of attributes contained in this. */
+  int size();
+
+  /** Whether there are any attributes contained in this. */
+  boolean isEmpty();
+
+  /** Iterates over all the key-value pairs of attributes contained by this instance. */
+  void forEach(KeyValueConsumer<AttributeValue> consumer);
+
+  /**
+   * Returns the value of the given key, or null if the key does not exist.
+   *
+   * <p>Currently may be implemented via a linear search, depending on implementation, so O(n)
+   * performance in the worst case.
+   */
+  @Nullable
+  AttributeValue get(String key);
+}

examples/metrics/src/main/java/io/opentelemetry/example/metrics/LongValueObserverExample.java

@@ -23,7 +23,7 @@ public static void main(String[] args) {
             .build();
 
     observer.setCallback(
-        new LongValueObserver.Callback<LongResult>() {
+        new LongValueObserver.Callback<LongValueObserver.LongResult>() {
           @Override
           public void update(LongResult result) {
             result.observe(Runtime.getRuntime().totalMemory());

exporters/jaeger/src/main/java/io/opentelemetry/exporters/jaeger/Adapter.java

@@ -21,8 +21,8 @@
 import com.google.protobuf.Timestamp;
 import com.google.protobuf.util.Timestamps;
 import io.opentelemetry.common.AttributeValue;
-import io.opentelemetry.common.Attributes;
 import io.opentelemetry.common.KeyValueConsumer;
+import io.opentelemetry.common.ReadableAttributes;
 import io.opentelemetry.exporters.jaeger.proto.api_v2.Model;
 import io.opentelemetry.sdk.extensions.otproto.TraceProtoUtils;
 import io.opentelemetry.sdk.trace.data.SpanData;
@@ -180,7 +180,7 @@ static Collection<Model.KeyValue> toKeyValues(Map<String, AttributeValue> attrib
    * @see #toKeyValue(String, AttributeValue)
    */
   @VisibleForTesting
-  static Collection<Model.KeyValue> toKeyValues(Attributes attributes) {
+  static Collection<Model.KeyValue> toKeyValues(ReadableAttributes attributes) {
     final ArrayList<Model.KeyValue> tags = new ArrayList<>(attributes.size());
     attributes.forEach(
         new KeyValueConsumer<AttributeValue>() {

exporters/zipkin/src/main/java/io/opentelemetry/exporters/zipkin/ZipkinSpanExporter.java

@@ -20,8 +20,8 @@
 
 import io.opentelemetry.common.AttributeValue;
 import io.opentelemetry.common.AttributeValue.Type;
-import io.opentelemetry.common.Attributes;
 import io.opentelemetry.common.KeyValueConsumer;
+import io.opentelemetry.common.ReadableAttributes;
 import io.opentelemetry.sdk.common.export.ConfigBuilder;
 import io.opentelemetry.sdk.resources.ResourceConstants;
 import io.opentelemetry.sdk.trace.data.SpanData;
@@ -141,7 +141,7 @@ static Span generateSpan(SpanData spanData, Endpoint localEndpoint) {
       spanBuilder.parentId(spanData.getParentSpanId().toLowerBase16());
     }
 
-    Attributes spanAttributes = spanData.getAttributes();
+    ReadableAttributes spanAttributes = spanData.getAttributes();
     spanAttributes.forEach(
         new KeyValueConsumer<AttributeValue>() {
           @Override

opentracing_shim/src/test/java/io/opentelemetry/opentracingshim/testbed/nestedcallbacks/NestedCallbacksTest.java

@@ -22,7 +22,7 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 
-import io.opentelemetry.common.Attributes;
+import io.opentelemetry.common.ReadableAttributes;
 import io.opentelemetry.exporters.inmemory.InMemoryTracing;
 import io.opentelemetry.opentracingshim.TraceShim;
 import io.opentelemetry.sdk.correlationcontext.CorrelationContextManagerSdk;
@@ -60,7 +60,7 @@ public void test() {
     assertEquals(1, spans.size());
     assertEquals("one", spans.get(0).getName());
 
-    Attributes attrs = spans.get(0).getAttributes();
+    ReadableAttributes attrs = spans.get(0).getAttributes();
     assertEquals(3, attrs.size());
     for (int i = 1; i <= 3; i++) {
       assertEquals(

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

@@ -17,6 +17,8 @@
 package io.opentelemetry.sdk.trace;
 
 import io.opentelemetry.common.AttributeValue;
+import io.opentelemetry.common.KeyValueConsumer;
+import io.opentelemetry.common.ReadableAttributes;
 import java.util.HashMap;
 import java.util.Map;
 import javax.annotation.Nullable;
@@ -27,7 +29,7 @@
  * <p>Some APIs may have slightly different behaviors, like `put` which returns null if out of
  * capacity.
  */
-final class AttributesMap extends HashMap<String, AttributeValue> {
+final class AttributesMap extends HashMap<String, AttributeValue> implements ReadableAttributes {
 
   private final long capacity;
   private int totalAddedValues = 0;
@@ -83,4 +85,17 @@ public boolean replace(String key, AttributeValue oldValue, AttributeValue newVa
   int getTotalAddedValues() {
     return totalAddedValues;
   }
+
+  @Override
+  public void forEach(KeyValueConsumer<AttributeValue> consumer) {
+    for (Entry<String, AttributeValue> entry : entrySet()) {
+      consumer.consume(entry.getKey(), entry.getValue());
+    }
+  }
+
+  @Nullable
+  @Override
+  public AttributeValue get(String key) {
+    return super.get(key);
+  }
 }

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

@@ -23,6 +23,7 @@
 import io.opentelemetry.common.AttributeValue;
 import io.opentelemetry.common.Attributes;
 import io.opentelemetry.common.KeyValueConsumer;
+import io.opentelemetry.common.ReadableAttributes;
 import io.opentelemetry.sdk.common.Clock;
 import io.opentelemetry.sdk.common.InstrumentationLibraryInfo;
 import io.opentelemetry.sdk.resources.Resource;
@@ -366,9 +367,9 @@ static Attributes copyAndLimitAttributes(final Attributes attributes, final int
       return attributes;
     }
 
-    Attributes.Builder temp = Attributes.newBuilder();
-    attributes.forEach(new LimitingAttributeConsumer(limit, temp));
-    return temp.build();
+    Attributes.Builder result = Attributes.newBuilder();
+    attributes.forEach(new LimitingAttributeConsumer(limit, result));
+    return result.build();
   }
 
   private void addTimedEvent(TimedEvent timedEvent) {
@@ -515,10 +516,16 @@ private List<Event> getImmutableTimedEvents() {
   }
 
   @GuardedBy("lock")
-  private Attributes getImmutableAttributes() {
+  private ReadableAttributes getImmutableAttributes() {
     if (attributes == null || attributes.isEmpty()) {
       return Attributes.empty();
     }
+    // if the span has ended, then the attributes are unmodifiable,
+    // so we can return them directly and save copying all the data.
+    if (hasEnded) {
+      return attributes;
+    }
+    // otherwise, make a copy of the data into an immutable container.
     Attributes.Builder builder = Attributes.newBuilder();
     for (Entry<String, AttributeValue> entry : attributes.entrySet()) {
       builder.setAttribute(entry.getKey(), entry.getValue());
@@ -538,8 +545,9 @@ public LimitingAttributeConsumer(int limit, Attributes.Builder builder) {
 
     @Override
     public void consume(String key, AttributeValue value) {
-      if (added++ < limit) {
+      if (added < limit) {
         builder.setAttribute(key, value);
+        added++;
       }
     }
   }

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

@@ -17,7 +17,7 @@
 package io.opentelemetry.sdk.trace;
 
 import com.google.auto.value.AutoValue;
-import io.opentelemetry.common.Attributes;
+import io.opentelemetry.common.ReadableAttributes;
 import io.opentelemetry.sdk.common.InstrumentationLibraryInfo;
 import io.opentelemetry.sdk.resources.Resource;
 import io.opentelemetry.sdk.trace.data.SpanData;
@@ -39,7 +39,7 @@ abstract class SpanWrapper implements SpanData {
 
   abstract List<Event> resolvedEvents();
 
-  abstract Attributes attributes();
+  abstract ReadableAttributes attributes();
 
   abstract int totalAttributeCount();
 
@@ -55,7 +55,7 @@ static SpanWrapper create(
       RecordEventsReadableSpan delegate,
       List<Link> links,
       List<Event> events,
-      Attributes attributes,
+      ReadableAttributes attributes,
       int totalAttributeCount,
       int totalRecordedEvents,
       Status status) {
@@ -114,7 +114,7 @@ public long getStartEpochNanos() {
   }
 
   @Override
-  public Attributes getAttributes() {
+  public ReadableAttributes getAttributes() {
     return attributes();
   }
 

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

@@ -18,6 +18,7 @@
 
 import com.google.auto.value.AutoValue;
 import io.opentelemetry.common.Attributes;
+import io.opentelemetry.common.ReadableAttributes;
 import io.opentelemetry.sdk.common.InstrumentationLibraryInfo;
 import io.opentelemetry.sdk.resources.Resource;
 import io.opentelemetry.sdk.trace.config.TraceConfig;
@@ -122,7 +123,7 @@ public interface SpanData {
    * @return the attributes recorded for this {@code Span}.
    * @since 0.1.0
    */
-  Attributes getAttributes();
+  ReadableAttributes getAttributes();
 
   /**
    * Returns the timed events recorded for this {@code Span}.

sdk/src/main/java/io/opentelemetry/sdk/trace/data/test/TestSpanData.java

@@ -19,6 +19,7 @@
 import com.google.auto.value.AutoValue;
 import io.opentelemetry.common.AttributeValue;
 import io.opentelemetry.common.Attributes;
+import io.opentelemetry.common.ReadableAttributes;
 import io.opentelemetry.sdk.common.InstrumentationLibraryInfo;
 import io.opentelemetry.sdk.resources.Resource;
 import io.opentelemetry.sdk.trace.data.SpanData;
@@ -188,7 +189,7 @@ public abstract Builder setInstrumentationLibraryInfo(
      * @see AttributeValue
      * @since 0.1.0
      */
-    public abstract Builder setAttributes(Attributes attributes);
+    public abstract Builder setAttributes(ReadableAttributes attributes);
 
     /**
      * Set timed events that are associated with this span. Must not be null, may be empty.

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

@@ -21,6 +21,7 @@
 import io.opentelemetry.common.AttributeValue;
 import io.opentelemetry.common.Attributes;
 import io.opentelemetry.common.KeyValueConsumer;
+import io.opentelemetry.common.ReadableAttributes;
 import io.opentelemetry.sdk.common.InstrumentationLibraryInfo;
 import io.opentelemetry.sdk.internal.TestClock;
 import io.opentelemetry.sdk.resources.Resource;
@@ -633,7 +634,7 @@ private void spanDoWork(RecordEventsReadableSpan span, @Nullable Status status)
 
   private void verifySpanData(
       SpanData spanData,
-      Attributes attributes,
+      final ReadableAttributes attributes,
       List<Event> eventData,
       List<io.opentelemetry.trace.Link> links,
       String spanName,
@@ -649,13 +650,23 @@ private void verifySpanData(
     assertThat(spanData.getResource()).isEqualTo(resource);
     assertThat(spanData.getInstrumentationLibraryInfo()).isEqualTo(instrumentationLibraryInfo);
     assertThat(spanData.getName()).isEqualTo(spanName);
-    assertThat(spanData.getAttributes()).isEqualTo(attributes);
     assertThat(spanData.getEvents()).isEqualTo(eventData);
     assertThat(spanData.getLinks()).isEqualTo(links);
     assertThat(spanData.getStartEpochNanos()).isEqualTo(startEpochNanos);
     assertThat(spanData.getEndEpochNanos()).isEqualTo(endEpochNanos);
     assertThat(spanData.getStatus().getCanonicalCode()).isEqualTo(status.getCanonicalCode());
     assertThat(spanData.getHasEnded()).isEqualTo(hasEnded);
+
+    // verify equality manually, since the implementations don't all equals with each other.
+    ReadableAttributes spanDataAttributes = spanData.getAttributes();
+    assertThat(spanDataAttributes.size()).isEqualTo(attributes.size());
+    spanDataAttributes.forEach(
+        new KeyValueConsumer<AttributeValue>() {
+          @Override
+          public void consume(String key, AttributeValue value) {
+            assertThat(attributes.get(key)).isEqualTo(value);
+          }
+        });
   }
 
   @Test
@@ -722,7 +733,7 @@ public void consume(String key, AttributeValue value) {
     SpanData result = readableSpan.toSpanData();
     verifySpanData(
         result,
-        attributes,
+        attributesWithCapacity,
         events,
         Collections.<io.opentelemetry.trace.Link>singletonList(link1),
         name,

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

@@ -21,6 +21,7 @@
 
 import io.opentelemetry.common.AttributeValue;
 import io.opentelemetry.common.Attributes;
+import io.opentelemetry.common.ReadableAttributes;
 import io.opentelemetry.context.Scope;
 import io.opentelemetry.sdk.trace.config.TraceConfig;
 import io.opentelemetry.sdk.trace.data.SpanData;
@@ -215,7 +216,7 @@ public void setAttribute() {
     RecordEventsReadableSpan span = (RecordEventsReadableSpan) spanBuilder.startSpan();
     try {
       SpanData spanData = span.toSpanData();
-      Attributes attrs = spanData.getAttributes();
+      ReadableAttributes attrs = spanData.getAttributes();
       assertThat(attrs.size()).isEqualTo(5);
       assertThat(attrs.get("string")).isEqualTo(AttributeValue.stringAttributeValue("value"));
       assertThat(attrs.get("long")).isEqualTo(AttributeValue.longAttributeValue(12345L));
@@ -240,7 +241,7 @@ public void setAttribute_afterEnd() {
 
     RecordEventsReadableSpan span = (RecordEventsReadableSpan) spanBuilder.startSpan();
     try {
-      Attributes attrs = span.toSpanData().getAttributes();
+      ReadableAttributes attrs = span.toSpanData().getAttributes();
       assertThat(attrs.size()).isEqualTo(5);
       assertThat(attrs.get("string")).isEqualTo(AttributeValue.stringAttributeValue("value"));
       assertThat(attrs.get("long")).isEqualTo(AttributeValue.longAttributeValue(12345L));
@@ -258,7 +259,7 @@ public void setAttribute_afterEnd() {
     span.setAttribute("boolean2", true);
     span.setAttribute("stringAttribute2", AttributeValue.stringAttributeValue("attrvalue"));
 
-    Attributes attrs = span.toSpanData().getAttributes();
+    ReadableAttributes attrs = span.toSpanData().getAttributes();
     assertThat(attrs.size()).isEqualTo(5);
     assertThat(attrs.get("string2")).isNull();
     assertThat(attrs.get("long2")).isNull();
@@ -310,7 +311,7 @@ public void setAttribute_NoEffectAfterStartSpan() {
     spanBuilder.setAttribute("key2", "value2");
     RecordEventsReadableSpan span = (RecordEventsReadableSpan) spanBuilder.startSpan();
 
-    Attributes beforeAttributes = span.toSpanData().getAttributes();
+    ReadableAttributes beforeAttributes = span.toSpanData().getAttributes();
     assertThat(beforeAttributes.size()).isEqualTo(2);
     assertThat(beforeAttributes.get("key1"))
         .isEqualTo(AttributeValue.stringAttributeValue("value1"));
@@ -319,7 +320,7 @@ public void setAttribute_NoEffectAfterStartSpan() {
 
     spanBuilder.setAttribute("key3", "value3");
 
-    Attributes afterAttributes = span.toSpanData().getAttributes();
+    ReadableAttributes afterAttributes = span.toSpanData().getAttributes();
     assertThat(afterAttributes.size()).isEqualTo(2);
     assertThat(afterAttributes.get("key1"))
         .isEqualTo(AttributeValue.stringAttributeValue("value1"));
@@ -402,7 +403,7 @@ public void droppingAttributes() {
     }
     RecordEventsReadableSpan span = (RecordEventsReadableSpan) spanBuilder.startSpan();
     try {
-      Attributes attrs = span.toSpanData().getAttributes();
+      ReadableAttributes attrs = span.toSpanData().getAttributes();
       assertThat(attrs.size()).isEqualTo(maxNumberOfAttrs);
       for (int i = 0; i < maxNumberOfAttrs; i++) {
         assertThat(attrs.get("key" + i)).isEqualTo(AttributeValue.longAttributeValue(i));