Move attribute references to instrumentation api

Author: JonasKunz

exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporter.java

@@ -16,7 +16,6 @@
 import io.opentelemetry.sdk.common.CompletableResultCode;
 import io.opentelemetry.sdk.common.InternalTelemetrySchemaVersion;
 import io.opentelemetry.sdk.internal.ComponentId;
-import io.opentelemetry.sdk.internal.SemConvAttributes;
 import io.opentelemetry.sdk.internal.ThrottlingLogger;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.Supplier;
@@ -86,15 +85,15 @@ private void onResponse(
       GrpcResponse grpcResponse) {
     int statusCode = grpcResponse.grpcStatusValue();
 
-    Attributes requestAttributes = Attributes.of(SemConvAttributes.RPC_GRPC_STATUS_CODE, (long) statusCode);
+    metricRecording.setGrpcStatusCode(statusCode);
 
     if (statusCode == 0) {
-      metricRecording.finishSuccessful(requestAttributes);
+      metricRecording.finishSuccessful();
       result.succeed();
       return;
     }
 
-    metricRecording.finishFailed(String.valueOf(statusCode), requestAttributes);
+    metricRecording.finishFailed(String.valueOf(statusCode));
     switch (statusCode) {
       case GRPC_STATUS_UNIMPLEMENTED:
         if (loggedUnimplemented.compareAndSet(false, true)) {
@@ -130,7 +129,7 @@ private void onError(
       CompletableResultCode result,
       ExporterInstrumentation.Recording metricRecording,
       Throwable e) {
-    metricRecording.finishFailed(e, Attributes.empty());
+    metricRecording.finishFailed(e);
     logger.log(
         Level.SEVERE,
         "Failed to export "

exporters/common/src/main/java/io/opentelemetry/exporter/internal/http/HttpExporter.java

@@ -14,7 +14,6 @@
 import io.opentelemetry.sdk.common.CompletableResultCode;
 import io.opentelemetry.sdk.common.InternalTelemetrySchemaVersion;
 import io.opentelemetry.sdk.internal.ComponentId;
-import io.opentelemetry.sdk.internal.SemConvAttributes;
 import io.opentelemetry.sdk.internal.ThrottlingLogger;
 import java.io.IOException;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -84,15 +83,15 @@ private void onResponse(
       HttpSender.Response httpResponse) {
     int statusCode = httpResponse.statusCode();
 
-    Attributes requestAttributes = Attributes.of(SemConvAttributes.HTTP_RESPONSE_STATUS_CODE, (long) statusCode);
+    metricRecording.setHttpStatusCode(statusCode);
 
     if (statusCode >= 200 && statusCode < 300) {
-      metricRecording.finishSuccessful(requestAttributes);
+      metricRecording.finishSuccessful();
       result.succeed();
       return;
     }
 
-    metricRecording.finishFailed(String.valueOf(statusCode), requestAttributes);
+    metricRecording.finishFailed(String.valueOf(statusCode));
 
     byte[] body = null;
     try {
@@ -119,7 +118,7 @@ private void onError(
       CompletableResultCode result,
       ExporterInstrumentation.Recording metricRecording,
       Throwable e) {
-    metricRecording.finishFailed(e, Attributes.empty());
+    metricRecording.finishFailed(e);
     logger.log(
         Level.SEVERE,
         "Failed to export "

exporters/common/src/main/java/io/opentelemetry/exporter/internal/metrics/ExporterInstrumentation.java

@@ -9,14 +9,15 @@
 import io.opentelemetry.api.metrics.MeterProvider;
 import io.opentelemetry.sdk.common.InternalTelemetrySchemaVersion;
 import io.opentelemetry.sdk.internal.ComponentId;
+import io.opentelemetry.sdk.internal.SemConvAttributes;
 import java.util.function.Supplier;
 import javax.annotation.Nullable;
 
 /**
  * This class is internal and is hence not for public use. Its APIs are unstable and can change at
  * any time.
  */
-public class ExporterInstrumentation implements ExporterMetrics {
+public class ExporterInstrumentation {
 
   private final ExporterMetrics implementation;
 
@@ -51,7 +52,6 @@ public ExporterInstrumentation(
     }
   }
 
-  @Override
   public Recording startRecordingExport(int itemCount) {
     return new Recording(implementation.startRecordingExport(itemCount));
   }
@@ -60,28 +60,64 @@ public Recording startRecordingExport(int itemCount) {
    * This class is internal and is hence not for public use. Its APIs are unstable and can change at
    * any time.
    */
-  public static class Recording extends ExporterMetrics.Recording {
+  public static class Recording {
 
     private final ExporterMetrics.Recording delegate;
+    @Nullable private Long httpStatusCode;
+    @Nullable private Long grpcStatusCode;
 
     private Recording(ExporterMetrics.Recording delegate) {
       this.delegate = delegate;
     }
 
+    public void setHttpStatusCode(long httpStatusCode) {
+      if (grpcStatusCode != null) {
+        throw new IllegalStateException(
+            "gRPC status code already set, can only set either gRPC or HTTP");
+      }
+      this.httpStatusCode = httpStatusCode;
+    }
+
+    public void setGrpcStatusCode(long grpcStatusCode) {
+      if (httpStatusCode != null) {
+        throw new IllegalStateException(
+            "HTTP status code already set, can only set either gRPC or HTTP");
+      }
+      this.grpcStatusCode = grpcStatusCode;
+    }
+
+    /** Callback to notify that the export was successful. */
+    public void finishSuccessful() {
+      delegate.finishSuccessful(buildRequestAttributes());
+    }
+
     /**
      * Callback to notify that the export has failed with the given {@link Throwable} as failure
      * cause.
      *
      * @param failureCause the cause of the failure
-     * @param requestAttributes additional attributes to add to request metrics
      */
-    public final void finishFailed(Throwable failureCause, Attributes requestAttributes) {
-      finishFailed(failureCause.getClass().getName(), requestAttributes);
+    public void finishFailed(Throwable failureCause) {
+      finishFailed(failureCause.getClass().getName());
+    }
+
+    /**
+     * Callback to notify that the export has failed.
+     *
+     * @param errorType a failure reason suitable for the error.type attribute
+     */
+    public void finishFailed(String errorType) {
+      delegate.finishFailed(errorType, buildRequestAttributes());
     }
 
-    @Override
-    protected void doFinish(@Nullable String errorType, Attributes requestAttributes) {
-      delegate.doFinish(errorType, requestAttributes);
+    private Attributes buildRequestAttributes() {
+      if (httpStatusCode != null) {
+        return Attributes.of(SemConvAttributes.HTTP_RESPONSE_STATUS_CODE, httpStatusCode);
+      }
+      if (grpcStatusCode != null) {
+        return Attributes.of(SemConvAttributes.RPC_GRPC_STATUS_CODE, grpcStatusCode);
+      }
+      return Attributes.empty();
     }
   }
 }

exporters/common/src/main/java/io/opentelemetry/exporter/internal/metrics/ExporterMetrics.java

@@ -26,22 +26,11 @@ abstract class Recording {
 
     protected Recording() {}
 
-    /**
-     * Callback to notify that the export was successful.
-     *
-     * @param requestAttributes additional attributes to add to request metrics
-     */
     public final void finishSuccessful(Attributes requestAttributes) {
       ensureEndedOnce();
       doFinish(null, requestAttributes);
     }
 
-    /**
-     * Callback to notify that the export has failed.
-     *
-     * @param errorType a failure reason suitable for the error.type attribute
-     * @param requestAttributes additional attributes to add to request metrics
-     */
     public final void finishFailed(String errorType, Attributes requestAttributes) {
       ensureEndedOnce();
       if (errorType == null || errorType.isEmpty()) {

exporters/common/src/main/java/io/opentelemetry/exporter/internal/metrics/NoopExporterMetrics.java

@@ -14,13 +14,11 @@ class NoopExporterMetrics implements ExporterMetrics {
 
   @Override
   public Recording startRecordingExport(int itemCount) {
-    return NoopRecording.INSTANCE;
+    return new NoopRecording();
   }
 
   private static class NoopRecording extends Recording {
 
-    private static final NoopRecording INSTANCE = new NoopRecording();
-
     @Override
     protected void doFinish(@Nullable String errorType, Attributes requestAttributes) {}
   }

exporters/common/src/test/java/io/opentelemetry/exporter/internal/metrics/ExporterInstrumentationTest.java

@@ -12,7 +12,6 @@
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
 
-import io.opentelemetry.api.common.Attributes;
 import io.opentelemetry.api.metrics.MeterProvider;
 import io.opentelemetry.sdk.common.CompletableResultCode;
 import io.opentelemetry.sdk.common.InternalTelemetrySchemaVersion;
@@ -72,16 +71,16 @@ public AggregationTemporality getAggregationTemporality(
 
     // Verify the supplier is only called once per underlying meter.
 
-    instrumentation.startRecordingExport(42).finishFailed("foo", Attributes.empty());
-    instrumentation.startRecordingExport(42).finishSuccessful(Attributes.empty());
+    instrumentation.startRecordingExport(42).finishFailed("foo");
+    instrumentation.startRecordingExport(42).finishSuccessful();
     if (schemaVersion == InternalTelemetrySchemaVersion.DISABLED) {
       verifyNoInteractions(meterProviderSupplier);
     } else {
       verify(meterProviderSupplier, atLeastOnce()).get();
     }
 
-    instrumentation.startRecordingExport(42).finishFailed("foo", Attributes.empty());
-    instrumentation.startRecordingExport(42).finishSuccessful(Attributes.empty());
+    instrumentation.startRecordingExport(42).finishFailed("foo");
+    instrumentation.startRecordingExport(42).finishSuccessful();
     verifyNoMoreInteractions(meterProviderSupplier);
   }
 
@@ -103,13 +102,13 @@ void noopMeterProvider(InternalTelemetrySchemaVersion schemaVersion) {
     verifyNoInteractions(meterProviderSupplier); // Ensure lazy
 
     // Verify the supplier is invoked multiple times since it returns a noop meter.
-    instrumentation.startRecordingExport(42).finishFailed("foo", Attributes.empty());
-    instrumentation.startRecordingExport(42).finishSuccessful(Attributes.empty());
+    instrumentation.startRecordingExport(42).finishFailed("foo");
+    instrumentation.startRecordingExport(42).finishSuccessful();
     verify(meterProviderSupplier, atLeastOnce()).get();
 
     Mockito.clearInvocations((Object) meterProviderSupplier);
-    instrumentation.startRecordingExport(42).finishFailed("foo", Attributes.empty());
-    instrumentation.startRecordingExport(42).finishSuccessful(Attributes.empty());
+    instrumentation.startRecordingExport(42).finishFailed("foo");
+    instrumentation.startRecordingExport(42).finishSuccessful();
     verify(meterProviderSupplier, atLeastOnce()).get();
   }
 }

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

@@ -67,7 +67,6 @@ public final class ZipkinSpanExporter implements SpanExporter {
     } else {
       exporterType = ComponentId.StandardExporterType.ZIPKIN_HTTP_SPAN_EXPORTER;
     }
-    // TODO: add server address and port attributes
     this.exporterMetrics =
         new ExporterInstrumentation(
             internalTelemetrySchemaVersion,
@@ -98,10 +97,10 @@ public CompletableResultCode export(Collection<SpanData> spanDataList) {
         () -> {
           try {
             sender.send(encodedSpans);
-            metricRecording.finishSuccessful(Attributes.empty());
+            metricRecording.finishSuccessful();
             resultCode.succeed();
           } catch (IOException | RuntimeException e) {
-            metricRecording.finishFailed(e, Attributes.empty());
+            metricRecording.finishFailed(e);
             logger.log(Level.WARNING, "Failed to export spans", e);
             resultCode.fail();
           }