Merge start and end time extractors (#4692)

docs/contributing/using-instrumenter-api.md

@@ -294,32 +294,26 @@ default `jdk()` implementation that removes the known JDK wrapper exception type
 You can set the `ErrorCauseExtractor` in the `InstrumenterBuilder` using
 the `setErrorCauseExtractor()` method.
 
-### Provide custom operation start and end times using the `StartTimeExtractor` and `EndTimeExtractor`
+### Provide custom operation start and end times using the `TimeExtractor`
 
 In some cases, the instrumented library provides a way to retrieve accurate timestamps of when the
-operation starts and ends. The `StartTimeExtractor` and `EndTimeExtractor` interfaces can be used to
-feed this information into OpenTelemetry trace and metrics data. Provide either both time extractors
-or none to the `InstrumenterBuilder`: it is crucial to avoid a situation where one time measurement
-uses the library timestamp and the other the internal OpenTelemetry SDK clock, as it would result in
-inaccurate telemetry.
+operation starts and ends. The `TimeExtractor` interface can be used to
+feed this information into OpenTelemetry trace and metrics data.
 
-The `StartTimeExtractor` can only extract the timestamp from the request. The `EndTimeExtractor`
+`extractStartTime()` can only extract the timestamp from the request. `extractEndTime()`
 accepts the request, an optional response, and an optional `Throwable` error. Consider the following
 example:
 
 ```java
-class MyStartTimeExtractor implements StartTimeExtractor<Request> {
+class MyTimeExtractor implements TimeExtractor<Request, Response> {
 
   @Override
-  public Instant extract(Request request) {
+  public Instant extractStartTime(Request request) {
     return request.startTimestamp();
   }
-}
-
-class MyEndTimeExtractor implements EndTimeExtractor<Request, Response> {
 
   @Override
-  public Instant extract(Request request, @Nullable Response response, @Nullable Throwable error) {
+  public Instant extractEndTime(Request request, @Nullable Response response, @Nullable Throwable error) {
     if (response != null) {
       return response.endTimestamp();
     }
@@ -332,7 +326,7 @@ The sample implementations above use the request to retrieve the start timestamp
 used to compute the end time if it is available; in case it is missing (for example, when an error
 occurs) the same time source is used to compute the current timestamp.
 
-You can set both time extractors in the `InstrumenterBuilder` using the `setTimeExtractors()`
+You can set the time extractor in the `InstrumenterBuilder` using the `setTimeExtractor()`
 method.
 
 ### Register metrics by implementing the `RequestMetrics` and `RequestListener`

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

@@ -102,8 +102,7 @@ public static <REQUEST, RESPONSE> InstrumenterBuilder<REQUEST, RESPONSE> builder
   private final List<? extends RequestListener> requestListeners;
   private final List<? extends RequestListener> requestMetricListeners;
   private final ErrorCauseExtractor errorCauseExtractor;
-  @Nullable private final StartTimeExtractor<REQUEST> startTimeExtractor;
-  @Nullable private final EndTimeExtractor<REQUEST, RESPONSE> endTimeExtractor;
+  @Nullable private final TimeExtractor<REQUEST, RESPONSE> timeExtractor;
   private final boolean disabled;
   private final SpanSuppressionStrategy spanSuppressionStrategy;
 
@@ -120,8 +119,7 @@ public static <REQUEST, RESPONSE> InstrumenterBuilder<REQUEST, RESPONSE> builder
     this.requestListeners = new ArrayList<>(builder.requestListeners);
     this.requestMetricListeners = new ArrayList<>(builder.requestMetricListeners);
     this.errorCauseExtractor = builder.errorCauseExtractor;
-    this.startTimeExtractor = builder.startTimeExtractor;
-    this.endTimeExtractor = builder.endTimeExtractor;
+    this.timeExtractor = builder.timeExtractor;
     this.disabled = builder.disabled;
     this.spanSuppressionStrategy = builder.getSpanSuppressionStrategy();
   }
@@ -161,8 +159,8 @@ public Context start(Context parentContext, REQUEST request) {
             .setParent(parentContext);
 
     Instant startTime = null;
-    if (startTimeExtractor != null) {
-      startTime = startTimeExtractor.extract(request);
+    if (timeExtractor != null) {
+      startTime = timeExtractor.extractStartTime(request);
       spanBuilder.setStartTimestamp(startTime);
     }
 
@@ -228,8 +226,8 @@ public void end(
     span.setAllAttributes(attributes);
 
     Instant endTime = null;
-    if (endTimeExtractor != null) {
-      endTime = endTimeExtractor.extract(request, response, error);
+    if (timeExtractor != null) {
+      endTime = timeExtractor.extractEndTime(request, response, error);
     }
 
     if (!requestListeners.isEmpty() || !requestMetricListeners.isEmpty()) {

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java

@@ -56,8 +56,7 @@ public final class InstrumenterBuilder<REQUEST, RESPONSE> {
   SpanStatusExtractor<? super REQUEST, ? super RESPONSE> spanStatusExtractor =
       SpanStatusExtractor.getDefault();
   ErrorCauseExtractor errorCauseExtractor = ErrorCauseExtractor.jdk();
-  @Nullable StartTimeExtractor<REQUEST> startTimeExtractor = null;
-  @Nullable EndTimeExtractor<REQUEST, RESPONSE> endTimeExtractor = null;
+  @Nullable TimeExtractor<REQUEST, RESPONSE> timeExtractor = null;
   boolean disabled = false;
 
   private boolean enableSpanSuppressionByType = ENABLE_SPAN_SUPPRESSION_BY_TYPE;
@@ -140,15 +139,13 @@ public InstrumenterBuilder<REQUEST, RESPONSE> setErrorCauseExtractor(
   }
 
   /**
-   * Sets the {@link StartTimeExtractor} and the {@link EndTimeExtractor} to extract the timestamp
-   * marking the start and end of processing. If unset, the constructed instrumenter will defer
-   * determining start and end timestamps to the OpenTelemetry SDK.
+   * Sets the {@link TimeExtractor} to extract the timestamp marking the start and end of
+   * processing. If unset, the constructed instrumenter will defer determining start and end
+   * timestamps to the OpenTelemetry SDK.
    */
-  public InstrumenterBuilder<REQUEST, RESPONSE> setTimeExtractors(
-      StartTimeExtractor<REQUEST> startTimeExtractor,
-      EndTimeExtractor<REQUEST, RESPONSE> endTimeExtractor) {
-    this.startTimeExtractor = requireNonNull(startTimeExtractor);
-    this.endTimeExtractor = requireNonNull(endTimeExtractor);
+  public InstrumenterBuilder<REQUEST, RESPONSE> setTimeExtractor(
+      TimeExtractor<REQUEST, RESPONSE> timeExtractor) {
+    this.timeExtractor = requireNonNull(timeExtractor);
     return this;
   }
 

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/TimeExtractor.java

@@ -0,0 +1,19 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.instrumenter;
+
+import java.time.Instant;
+import javax.annotation.Nullable;
+
+/** Extractor of the start and end times of request processing. */
+public interface TimeExtractor<REQUEST, RESPONSE> {
+
+  /** Returns the timestamp marking the start of the request processing. */
+  Instant extractStartTime(REQUEST request);
+
+  /** Returns the timestamp marking the end of the response processing. */
+  Instant extractEndTime(REQUEST request, @Nullable RESPONSE response, @Nullable Throwable error);
+}

instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java

@@ -129,6 +129,20 @@ public void extract(
     }
   }
 
+  static class TestTimeExtractor implements TimeExtractor<Instant, Instant> {
+
+    @Override
+    public Instant extractStartTime(Instant request) {
+      return request;
+    }
+
+    @Override
+    public Instant extractEndTime(
+        Instant request, @Nullable Instant response, @Nullable Throwable error) {
+      return response;
+    }
+  }
+
   static class MapGetter implements TextMapGetter<Map<String, String>> {
 
     @Override
@@ -450,7 +464,7 @@ void shouldStartSpanWithGivenStartTime() {
     Instrumenter<Instant, Instant> instrumenter =
         Instrumenter.<Instant, Instant>builder(
                 otelTesting.getOpenTelemetry(), "test", request -> "test span")
-            .setTimeExtractors(request -> request, (request, response, error) -> response)
+            .setTimeExtractor(new TestTimeExtractor())
             .newInstrumenter();
 
     Instant startTime = Instant.ofEpochSecond(100);

instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageTimeExtractor.java

@@ -0,0 +1,24 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.jms;
+
+import io.opentelemetry.instrumentation.api.instrumenter.TimeExtractor;
+import java.time.Instant;
+import javax.annotation.Nullable;
+
+class JmsMessageTimeExtractor implements TimeExtractor<MessageWithDestination, Void> {
+
+  @Override
+  public Instant extractStartTime(MessageWithDestination request) {
+    return request.startTime();
+  }
+
+  @Override
+  public Instant extractEndTime(
+      MessageWithDestination request, @Nullable Void unused, @Nullable Throwable error) {
+    return request.endTime();
+  }
+}

instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsSingletons.java

@@ -45,8 +45,7 @@ private static Instrumenter<MessageWithDestination, Void> buildConsumerInstrumen
     return Instrumenter.<MessageWithDestination, Void>builder(
             GlobalOpenTelemetry.get(), INSTRUMENTATION_NAME, spanNameExtractor)
         .addAttributesExtractor(attributesExtractor)
-        .setTimeExtractors(
-            MessageWithDestination::startTime, (request, response, error) -> request.endTime())
+        .setTimeExtractor(new JmsMessageTimeExtractor())
         .setDisabled(ExperimentalConfig.get().suppressMessagingReceiveSpans())
         .newInstrumenter(SpanKindExtractor.alwaysConsumer());
   }

instrumentation/kafka-clients/kafka-clients-common/library/src/main/java/io/opentelemetry/instrumentation/kafka/internal/KafkaConsumerTimeExtractor.java

@@ -0,0 +1,24 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.kafka.internal;
+
+import io.opentelemetry.instrumentation.api.instrumenter.TimeExtractor;
+import java.time.Instant;
+import javax.annotation.Nullable;
+
+class KafkaConsumerTimeExtractor implements TimeExtractor<ReceivedRecords, Void> {
+
+  @Override
+  public Instant extractStartTime(ReceivedRecords request) {
+    return request.startTime();
+  }
+
+  @Override
+  public Instant extractEndTime(
+      ReceivedRecords request, @Nullable Void unused, @Nullable Throwable error) {
+    return request.now();
+  }
+}

instrumentation/kafka-clients/kafka-clients-common/library/src/main/java/io/opentelemetry/instrumentation/kafka/internal/KafkaInstrumenterFactory.java

@@ -62,7 +62,7 @@ public static Instrumenter<ReceivedRecords, Void> createConsumerReceiveInstrumen
             openTelemetry, instrumentationName, spanNameExtractor)
         .addAttributesExtractor(attributesExtractor)
         .addAttributesExtractors(extractors)
-        .setTimeExtractors(ReceivedRecords::startTime, (request, response, error) -> request.now())
+        .setTimeExtractor(new KafkaConsumerTimeExtractor())
         .setDisabled(ExperimentalConfig.get().suppressMessagingReceiveSpans())
         .newInstrumenter(SpanKindExtractor.alwaysConsumer());
   }

instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/client/NettyClientSingletons.java

@@ -54,9 +54,7 @@ public final class NettyClientSingletons {
             .addAttributesExtractor(nettyConnectAttributesExtractor)
             .addAttributesExtractor(
                 PeerServiceAttributesExtractor.create(nettyConnectAttributesExtractor))
-            .setTimeExtractors(
-                request -> request.timer().startTime(),
-                (request, channel, error) -> request.timer().now())
+            .setTimeExtractor(new NettyConnectionTimeExtractor())
             .newInstrumenter(SpanKindExtractor.alwaysClient());
   }
 

instrumentation/netty/netty-3.8/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/v3_8/client/NettyConnectionTimeExtractor.java

@@ -0,0 +1,26 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.netty.v3_8.client;
+
+import io.opentelemetry.instrumentation.api.instrumenter.TimeExtractor;
+import io.opentelemetry.javaagent.instrumentation.netty.common.NettyConnectionRequest;
+import java.time.Instant;
+import javax.annotation.Nullable;
+import org.jboss.netty.channel.Channel;
+
+class NettyConnectionTimeExtractor implements TimeExtractor<NettyConnectionRequest, Channel> {
+
+  @Override
+  public Instant extractStartTime(NettyConnectionRequest request) {
+    return request.timer().startTime();
+  }
+
+  @Override
+  public Instant extractEndTime(
+      NettyConnectionRequest request, @Nullable Channel channel, @Nullable Throwable error) {
+    return request.timer().now();
+  }
+}

instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/client/NettyClientInstrumenterFactory.java

@@ -56,9 +56,7 @@ public NettyConnectionInstrumenter createConnectionInstrumenter() {
                 GlobalOpenTelemetry.get(), instrumentationName, NettyConnectionRequest::spanName)
             .addAttributesExtractor(netAttributesExtractor)
             .addAttributesExtractor(PeerServiceAttributesExtractor.create(netAttributesExtractor))
-            .setTimeExtractors(
-                request -> request.timer().startTime(),
-                (request, channel, error) -> request.timer().now())
+            .setTimeExtractor(new NettyConnectionTimeExtractor())
             .newInstrumenter(
                 alwaysCreateConnectSpan
                     ? SpanKindExtractor.alwaysInternal()
@@ -76,9 +74,7 @@ public NettySslInstrumenter createSslInstrumenter() {
                 GlobalOpenTelemetry.get(), instrumentationName, NettySslRequest::spanName)
             .addAttributesExtractor(netAttributesExtractor)
             .addAttributesExtractor(PeerServiceAttributesExtractor.create(netAttributesExtractor))
-            .setTimeExtractors(
-                request -> request.timer().startTime(),
-                (request, channel, error) -> request.timer().now())
+            .setTimeExtractor(new NettySslTimeExtractor())
             .newInstrumenter(
                 sslTelemetryEnabled
                     ? SpanKindExtractor.alwaysInternal()

instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/client/NettyConnectionTimeExtractor.java

@@ -0,0 +1,26 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.netty.common.client;
+
+import io.netty.channel.Channel;
+import io.opentelemetry.instrumentation.api.instrumenter.TimeExtractor;
+import io.opentelemetry.javaagent.instrumentation.netty.common.NettyConnectionRequest;
+import java.time.Instant;
+import javax.annotation.Nullable;
+
+class NettyConnectionTimeExtractor implements TimeExtractor<NettyConnectionRequest, Channel> {
+
+  @Override
+  public Instant extractStartTime(NettyConnectionRequest request) {
+    return request.timer().startTime();
+  }
+
+  @Override
+  public Instant extractEndTime(
+      NettyConnectionRequest request, @Nullable Channel channel, @Nullable Throwable error) {
+    return request.timer().now();
+  }
+}

instrumentation/netty/netty-4-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/netty/common/client/NettySslTimeExtractor.java

@@ -0,0 +1,24 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.netty.common.client;
+
+import io.opentelemetry.instrumentation.api.instrumenter.TimeExtractor;
+import java.time.Instant;
+import javax.annotation.Nullable;
+
+class NettySslTimeExtractor implements TimeExtractor<NettySslRequest, Void> {
+
+  @Override
+  public Instant extractStartTime(NettySslRequest request) {
+    return request.timer().startTime();
+  }
+
+  @Override
+  public Instant extractEndTime(
+      NettySslRequest request, @Nullable Void unused, @Nullable Throwable error) {
+    return request.timer().now();
+  }
+}