From 0d3bc703e441faf1c51a538c7a70da5e48c5e658 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Wed, 14 Apr 2021 12:01:46 +0200 Subject: [PATCH 1/7] Use Instrumenter in JMS instrumentation And introduce messaging semantic conventions --- .../api/instrumenter/ClientInstrumenter.java | 6 +- .../api/instrumenter/Instrumenter.java | 19 +- .../api/instrumenter/InstrumenterBuilder.java | 28 ++- .../api/instrumenter/ServerInstrumenter.java | 6 +- .../api/instrumenter/SpanKindExtractor.java | 10 + .../messaging/MessageOperation.java | 21 +++ .../MessagingAttributesExtractor.java | 94 +++++++++ .../messaging/MessagingSpanNameExtractor.java | 45 +++++ .../api/instrumenter/InstrumenterTest.java | 23 +++ .../messaging/MessageOperationTest.java | 19 ++ .../MessagingAttributesExtractorTest.java | 178 ++++++++++++++++++ .../MessagingSpanNameExtractorTest.java | 59 ++++++ .../src/jms2Test/groovy/Jms2Test.groovy | 1 + .../instrumentation/jms/JmsInstrumenters.java | 59 ++++++ .../jms/JmsMessageAttributesExtractor.java | 98 ++++++++++ .../JmsMessageConsumerInstrumentation.java | 24 +-- .../JmsMessageListenerInstrumentation.java | 27 ++- .../JmsMessageProducerInstrumentation.java | 39 ++-- .../instrumentation/jms/JmsTracer.java | 156 --------------- .../jms/MessageDestination.java | 21 --- ...dapter.java => MessagePropertyGetter.java} | 15 +- ...dapter.java => MessagePropertySetter.java} | 12 +- .../jms/MessageWithDestination.java | 108 +++++++++++ .../javaagent/src/test/groovy/Jms1Test.groovy | 1 + 24 files changed, 829 insertions(+), 240 deletions(-) create mode 100644 instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java create mode 100644 instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java create mode 100644 instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractor.java create mode 100644 instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperationTest.java create mode 100644 instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractorTest.java create mode 100644 instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractorTest.java create mode 100644 instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsInstrumenters.java create mode 100644 instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageAttributesExtractor.java delete mode 100644 instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsTracer.java delete mode 100644 instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageDestination.java rename instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/{MessageExtractAdapter.java => MessagePropertyGetter.java} (56%) rename instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/{MessageInjectAdapter.java => MessagePropertySetter.java} (66%) create mode 100644 instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestination.java diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ClientInstrumenter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ClientInstrumenter.java index 22520eda3875..681c7a1fb2a5 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ClientInstrumenter.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ClientInstrumenter.java @@ -9,7 +9,9 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.propagation.ContextPropagators; import io.opentelemetry.context.propagation.TextMapSetter; +import java.time.Instant; import java.util.List; +import org.checkerframework.checker.nullness.qual.Nullable; final class ClientInstrumenter extends Instrumenter { @@ -39,8 +41,8 @@ final class ClientInstrumenter extends Instrumenter extractor : extractors) { extractor.onStart(attributes, request); diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java index c2980e4bde05..2a09221eda08 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java @@ -83,8 +83,8 @@ public InstrumenterBuilder setErrorCauseExtractor( } /** - * Returns a new client {@link Instrumenter} which will create client spans and inject context - * into requests. + * Returns a new {@link Instrumenter} which will create client spans and inject context into + * requests. */ public Instrumenter newClientInstrumenter(TextMapSetter setter) { return newInstrumenter( @@ -93,8 +93,8 @@ public Instrumenter newClientInstrumenter(TextMapSetter newServerInstrumenter(TextMapGetter getter) { return newInstrumenter( @@ -102,6 +102,26 @@ public Instrumenter newServerInstrumenter(TextMapGetter newProducerInstrumenter(TextMapSetter setter) { + return newInstrumenter( + InstrumenterConstructor.propagatingToDownstream(openTelemetry.getPropagators(), setter), + SpanKindExtractor.alwaysProducer()); + } + + /** + * Returns a new {@link Instrumenter} which will create consumer spans and extract context from + * requests. + */ + public Instrumenter newConsumerInstrumenter(TextMapGetter getter) { + return newInstrumenter( + InstrumenterConstructor.propagatingFromUpstream(openTelemetry.getPropagators(), getter), + SpanKindExtractor.alwaysConsumer()); + } + /** * Returns a new {@link Instrumenter} which will create internal spans and do no context * propagation. diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java index ad4cee09bced..4957e76e45f0 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java @@ -10,7 +10,9 @@ import io.opentelemetry.context.propagation.ContextPropagators; import io.opentelemetry.context.propagation.TextMapGetter; import io.opentelemetry.instrumentation.api.internal.ContextPropagationDebug; +import java.time.Instant; import java.util.List; +import org.checkerframework.checker.nullness.qual.Nullable; final class ServerInstrumenter extends Instrumenter { @@ -40,10 +42,10 @@ final class ServerInstrumenter extends Instrumenter SpanKindExtractor alwaysServer() { return request -> SpanKind.SERVER; } + /** Returns a {@link SpanNameExtractor} which always returns {@link SpanKind#PRODUCER}. */ + static SpanKindExtractor alwaysProducer() { + return request -> SpanKind.PRODUCER; + } + + /** Returns a {@link SpanNameExtractor} which always returns {@link SpanKind#CONSUMER}. */ + static SpanKindExtractor alwaysConsumer() { + return request -> SpanKind.CONSUMER; + } + /** Returns the {@link SpanKind} corresponding to the {@link REQUEST}. */ SpanKind extract(REQUEST request); } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java new file mode 100644 index 000000000000..f00de665152e --- /dev/null +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java @@ -0,0 +1,21 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.instrumenter.messaging; + +/** + * Represents type of operations + * that may be used in a messaging system. + */ +public enum MessageOperation { + SEND, + RECEIVE, + PROCESS; + + public String operationName() { + return name().toLowerCase(); + } +} diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java new file mode 100644 index 000000000000..e027be5453b7 --- /dev/null +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java @@ -0,0 +1,94 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.instrumenter.messaging; + +import io.opentelemetry.api.common.AttributesBuilder; +import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor; +import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; +import org.checkerframework.checker.nullness.qual.Nullable; + +/** + * Extractor of messaging + * attributes. Instrumentation of messaging frameworks/libraries should extend this class, + * defining {@link REQUEST} and {@link RESPONSE} with the actual request / response types of the + * instrumented library. If an attribute is not available in this library, it is appropriate to + * return {@code null} from the protected attribute methods, but implement as many as possible for + * best compliance with the OpenTelemetry specification. + */ +public abstract class MessagingAttributesExtractor + extends AttributesExtractor { + public static final String TEMP_DESTINATION_NAME = "(temporary)"; + + @Override + protected final void onStart(AttributesBuilder attributes, REQUEST request) { + set(attributes, SemanticAttributes.MESSAGING_SYSTEM, system(request)); + set(attributes, SemanticAttributes.MESSAGING_DESTINATION_KIND, destinationKind(request)); + boolean isTemporaryDestination = temporaryDestination(request); + if (isTemporaryDestination) { + set(attributes, SemanticAttributes.MESSAGING_TEMP_DESTINATION, true); + set(attributes, SemanticAttributes.MESSAGING_DESTINATION, TEMP_DESTINATION_NAME); + } else { + set(attributes, SemanticAttributes.MESSAGING_DESTINATION, destination(request)); + } + set(attributes, SemanticAttributes.MESSAGING_PROTOCOL, protocol(request)); + set(attributes, SemanticAttributes.MESSAGING_PROTOCOL_VERSION, protocolVersion(request)); + set(attributes, SemanticAttributes.MESSAGING_URL, url(request)); + set(attributes, SemanticAttributes.MESSAGING_CONVERSATION_ID, conversationId(request)); + set( + attributes, + SemanticAttributes.MESSAGING_MESSAGE_PAYLOAD_SIZE_BYTES, + messagePayloadSize(request)); + set( + attributes, + SemanticAttributes.MESSAGING_MESSAGE_PAYLOAD_COMPRESSED_SIZE_BYTES, + messagePayloadCompressedSize(request)); + MessageOperation operation = operation(request); + if (operation == MessageOperation.RECEIVE || operation == MessageOperation.PROCESS) { + set(attributes, SemanticAttributes.MESSAGING_OPERATION, operation.operationName()); + } + } + + @Override + protected final void onEnd(AttributesBuilder attributes, REQUEST request, RESPONSE response) { + set(attributes, SemanticAttributes.MESSAGING_MESSAGE_ID, messageId(request, response)); + } + + @Nullable + protected abstract String system(REQUEST request); + + @Nullable + protected abstract String destinationKind(REQUEST request); + + @Nullable + protected abstract String destination(REQUEST request); + + protected abstract boolean temporaryDestination(REQUEST request); + + @Nullable + protected abstract String protocol(REQUEST request); + + @Nullable + protected abstract String protocolVersion(REQUEST request); + + @Nullable + protected abstract String url(REQUEST request); + + @Nullable + protected abstract String conversationId(REQUEST request); + + @Nullable + protected abstract Long messagePayloadSize(REQUEST request); + + @Nullable + protected abstract Long messagePayloadCompressedSize(REQUEST request); + + @Nullable + protected abstract MessageOperation operation(REQUEST request); + + @Nullable + protected abstract String messageId(REQUEST request, RESPONSE response); +} diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractor.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractor.java new file mode 100644 index 000000000000..c8f71ffb9ded --- /dev/null +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractor.java @@ -0,0 +1,45 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.instrumenter.messaging; + +import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor; + +public final class MessagingSpanNameExtractor implements SpanNameExtractor { + + /** + * Returns a {@link SpanNameExtractor} that constructs the span name according to instrumenter = + Instrumenter.newBuilder( + otelTesting.getOpenTelemetry(), "test", request -> request) + .newInstrumenter(); + + Instant startTime = Instant.ofEpochSecond(100); + + // when + Context context = instrumenter.start(Context.root(), "test span", startTime); + instrumenter.end(context, "test span", null, null); + + // then + otelTesting + .assertTraces() + .hasTracesSatisfyingExactly( + trace -> + trace.hasSpansSatisfyingExactly( + span -> span.hasName("test span").startsAt(startTime))); + } } diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperationTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperationTest.java new file mode 100644 index 000000000000..dcc901437fb8 --- /dev/null +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperationTest.java @@ -0,0 +1,19 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.instrumenter.messaging; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; + +class MessageOperationTest { + @ParameterizedTest + @EnumSource(MessageOperation.class) + void shouldGetCorrectOperationName(MessageOperation operation) { + assertEquals(operation.name().toLowerCase(), operation.operationName()); + } +} diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractorTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractorTest.java new file mode 100644 index 000000000000..938550224205 --- /dev/null +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractorTest.java @@ -0,0 +1,178 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.instrumenter.messaging; + +import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat; +import static java.util.Collections.singletonMap; +import static org.assertj.core.api.Assertions.entry; + +import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.common.AttributesBuilder; +import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Stream; +import org.assertj.core.data.MapEntry; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +class MessagingAttributesExtractorTest { + MessagingAttributesExtractor, String> underTest = + new MessagingAttributesExtractor, String>() { + @Override + protected String system(Map request) { + return request.get("system"); + } + + @Override + protected String destinationKind(Map request) { + return request.get("destinationKind"); + } + + @Override + protected String destination(Map request) { + return request.get("destination"); + } + + @Override + protected boolean temporaryDestination(Map request) { + return request.containsKey("temporaryDestination"); + } + + @Override + protected String protocol(Map request) { + return request.get("protocol"); + } + + @Override + protected String protocolVersion(Map request) { + return request.get("protocolVersion"); + } + + @Override + protected String url(Map request) { + return request.get("url"); + } + + @Override + protected String conversationId(Map request) { + return request.get("conversationId"); + } + + @Override + protected Long messagePayloadSize(Map request) { + String payloadSize = request.get("payloadSize"); + return payloadSize == null ? null : Long.valueOf(payloadSize); + } + + @Override + protected Long messagePayloadCompressedSize(Map request) { + String payloadSize = request.get("payloadCompressedSize"); + return payloadSize == null ? null : Long.valueOf(payloadSize); + } + + @Override + protected MessageOperation operation(Map request) { + String operation = request.get("operation"); + return operation == null ? null : MessageOperation.valueOf(operation); + } + + @Override + protected String messageId(Map request, String response) { + return response; + } + }; + + @ParameterizedTest + @MethodSource("destinations") + void shouldExtractAllAvailableAttributes( + boolean temporary, + String destination, + MessageOperation operation, + String expectedDestination) { + // given + Map request = new HashMap<>(); + request.put("system", "myQueue"); + request.put("destinationKind", "topic"); + request.put("destination", destination); + if (temporary) { + request.put("temporaryDestination", "y"); + } + request.put("protocol", "AMQP"); + request.put("protocolVersion", "1.0.0"); + request.put("url", "http://broker/topic"); + request.put("conversationId", "42"); + request.put("payloadSize", "100"); + request.put("payloadCompressedSize", "10"); + request.put("operation", operation.name()); + + // when + AttributesBuilder startAttributes = Attributes.builder(); + underTest.onStart(startAttributes, request); + + AttributesBuilder endAttributes = Attributes.builder(); + underTest.onEnd(endAttributes, request, "42"); + + // then + List, Object>> expectedEntries = new ArrayList<>(); + expectedEntries.add(entry(SemanticAttributes.MESSAGING_SYSTEM, "myQueue")); + expectedEntries.add(entry(SemanticAttributes.MESSAGING_DESTINATION_KIND, "topic")); + expectedEntries.add(entry(SemanticAttributes.MESSAGING_DESTINATION, expectedDestination)); + if (temporary) { + expectedEntries.add(entry(SemanticAttributes.MESSAGING_TEMP_DESTINATION, true)); + } + expectedEntries.add(entry(SemanticAttributes.MESSAGING_PROTOCOL, "AMQP")); + expectedEntries.add(entry(SemanticAttributes.MESSAGING_PROTOCOL_VERSION, "1.0.0")); + expectedEntries.add(entry(SemanticAttributes.MESSAGING_URL, "http://broker/topic")); + expectedEntries.add(entry(SemanticAttributes.MESSAGING_CONVERSATION_ID, "42")); + expectedEntries.add(entry(SemanticAttributes.MESSAGING_MESSAGE_PAYLOAD_SIZE_BYTES, 100L)); + expectedEntries.add( + entry(SemanticAttributes.MESSAGING_MESSAGE_PAYLOAD_COMPRESSED_SIZE_BYTES, 10L)); + expectedEntries.add(entry(SemanticAttributes.MESSAGING_OPERATION, operation.operationName())); + + assertThat(startAttributes.build()).containsOnly(expectedEntries.toArray(new MapEntry[0])); + + assertThat(endAttributes.build()) + .containsOnly(entry(SemanticAttributes.MESSAGING_MESSAGE_ID, "42")); + } + + static Stream destinations() { + return Stream.of( + Arguments.of(false, "destination", MessageOperation.RECEIVE, "destination"), + Arguments.of(true, null, MessageOperation.PROCESS, "(temporary)")); + } + + @Test + void shouldNotSetSendOperation() { + // when + AttributesBuilder attributes = Attributes.builder(); + underTest.onStart(attributes, singletonMap("operation", MessageOperation.SEND.name())); + + // then + assertThat(attributes.build().isEmpty()).isTrue(); + } + + @Test + void shouldExtractNoAttributesIfNoneAreAvailable() { + // when + AttributesBuilder startAttributes = Attributes.builder(); + underTest.onStart(startAttributes, Collections.emptyMap()); + + AttributesBuilder endAttributes = Attributes.builder(); + underTest.onEnd(endAttributes, Collections.emptyMap(), null); + + // then + assertThat(startAttributes.build().isEmpty()).isTrue(); + + assertThat(endAttributes.build().isEmpty()).isTrue(); + } +} diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractorTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractorTest.java new file mode 100644 index 000000000000..d28f8a8f2380 --- /dev/null +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractorTest.java @@ -0,0 +1,59 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.instrumenter.messaging; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.BDDMockito.given; + +import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor; +import java.util.stream.Stream; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +class MessagingSpanNameExtractorTest { + @Mock MessagingAttributesExtractor attributesExtractor; + + @ParameterizedTest + @MethodSource("spanNameParams") + void shouldExtractSpanName( + boolean isTemporaryQueue, + String destinationName, + MessageOperation operation, + String expectedSpanName) { + // given + Message message = new Message(); + + if (isTemporaryQueue) { + given(attributesExtractor.temporaryDestination(message)).willReturn(true); + } else { + given(attributesExtractor.destination(message)).willReturn(destinationName); + } + given(attributesExtractor.operation(message)).willReturn(operation); + + SpanNameExtractor underTest = MessagingSpanNameExtractor.create(attributesExtractor); + + // when + String spanName = underTest.extract(message); + + // then + assertEquals(expectedSpanName, spanName); + } + + static Stream spanNameParams() { + return Stream.of( + Arguments.of(false, "destination", MessageOperation.SEND, "destination send"), + Arguments.of(true, null, MessageOperation.PROCESS, "(temporary) process"), + Arguments.of(false, null, MessageOperation.RECEIVE, "unknown receive"), + Arguments.of(false, "destination", null, "destination")); + } + + static class Message {} +} diff --git a/instrumentation/jms-1.1/javaagent/src/jms2Test/groovy/Jms2Test.groovy b/instrumentation/jms-1.1/javaagent/src/jms2Test/groovy/Jms2Test.groovy index ae3059f55d18..b2b7b0189a30 100644 --- a/instrumentation/jms-1.1/javaagent/src/jms2Test/groovy/Jms2Test.groovy +++ b/instrumentation/jms-1.1/javaagent/src/jms2Test/groovy/Jms2Test.groovy @@ -247,6 +247,7 @@ class Jms2Test extends AgentInstrumentationSpecification { if (destinationName == "(temporary)") { "${SemanticAttributes.MESSAGING_TEMP_DESTINATION.key}" true } + "${SemanticAttributes.MESSAGING_MESSAGE_ID.key}" String } } } diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsInstrumenters.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsInstrumenters.java new file mode 100644 index 000000000000..865218fcb3db --- /dev/null +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsInstrumenters.java @@ -0,0 +1,59 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.jms; + +import io.opentelemetry.api.GlobalOpenTelemetry; +import io.opentelemetry.api.OpenTelemetry; +import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter; +import io.opentelemetry.instrumentation.api.instrumenter.SpanKindExtractor; +import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor; +import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessagingSpanNameExtractor; + +public final class JmsInstrumenters { + private static final String INSTRUMENTATION_NAME = "io.opentelemetry.javaagent.jms-1.1"; + + private static final Instrumenter PRODUCER_INSTRUMENTER; + private static final Instrumenter CONSUMER_INSTRUMENTER; + private static final Instrumenter LISTENER_INSTRUMENTER; + + static { + JmsMessageAttributesExtractor attributesExtractor = new JmsMessageAttributesExtractor(); + SpanNameExtractor spanNameExtractor = + MessagingSpanNameExtractor.create(attributesExtractor); + + OpenTelemetry otel = GlobalOpenTelemetry.get(); + PRODUCER_INSTRUMENTER = + Instrumenter.newBuilder( + otel, INSTRUMENTATION_NAME, spanNameExtractor) + .addAttributesExtractor(attributesExtractor) + .newProducerInstrumenter(new MessagePropertySetter()); + // MessageConsumer does not do context propagation + CONSUMER_INSTRUMENTER = + Instrumenter.newBuilder( + otel, INSTRUMENTATION_NAME, spanNameExtractor) + .addAttributesExtractor(attributesExtractor) + .newInstrumenter(SpanKindExtractor.alwaysConsumer()); + LISTENER_INSTRUMENTER = + Instrumenter.newBuilder( + otel, INSTRUMENTATION_NAME, spanNameExtractor) + .addAttributesExtractor(attributesExtractor) + .newConsumerInstrumenter(new MessagePropertyGetter()); + } + + public static Instrumenter producerInstrumenter() { + return PRODUCER_INSTRUMENTER; + } + + public static Instrumenter consumerInstrumenter() { + return CONSUMER_INSTRUMENTER; + } + + public static Instrumenter listenerInstrumenter() { + return LISTENER_INSTRUMENTER; + } + + private JmsInstrumenters() {} +} diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageAttributesExtractor.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageAttributesExtractor.java new file mode 100644 index 000000000000..d708edf955d7 --- /dev/null +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageAttributesExtractor.java @@ -0,0 +1,98 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.jms; + +import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessageOperation; +import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessagingAttributesExtractor; +import javax.jms.JMSException; +import org.checkerframework.checker.nullness.qual.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class JmsMessageAttributesExtractor + extends MessagingAttributesExtractor { + private static final Logger log = LoggerFactory.getLogger(JmsMessageAttributesExtractor.class); + + @Nullable + @Override + protected String system(MessageWithDestination messageWithDestination) { + return "jms"; + } + + @Nullable + @Override + protected String destinationKind(MessageWithDestination messageWithDestination) { + return messageWithDestination.getDestinationKind(); + } + + @Nullable + @Override + protected String destination(MessageWithDestination messageWithDestination) { + return messageWithDestination.getDestinationName(); + } + + @Override + protected boolean temporaryDestination(MessageWithDestination messageWithDestination) { + return messageWithDestination.isTemporaryDestination(); + } + + @Nullable + @Override + protected String protocol(MessageWithDestination messageWithDestination) { + return null; + } + + @Nullable + @Override + protected String protocolVersion(MessageWithDestination messageWithDestination) { + return null; + } + + @Nullable + @Override + protected String url(MessageWithDestination messageWithDestination) { + return null; + } + + @Nullable + @Override + protected String conversationId(MessageWithDestination messageWithDestination) { + try { + return messageWithDestination.getMessage().getJMSCorrelationID(); + } catch (JMSException e) { + log.debug("Failure getting JMS correlation id", e); + return null; + } + } + + @Nullable + @Override + protected Long messagePayloadSize(MessageWithDestination messageWithDestination) { + return null; + } + + @Nullable + @Override + protected Long messagePayloadCompressedSize(MessageWithDestination messageWithDestination) { + return null; + } + + @Override + protected MessageOperation operation(MessageWithDestination messageWithDestination) { + return messageWithDestination.getMessageOperation(); + } + + @Nullable + @Override + protected String messageId(MessageWithDestination messageWithDestination, Void unused) { + try { + return messageWithDestination.getMessage().getJMSMessageID(); + } catch (JMSException e) { + log.debug("Failure getting JMS message id", e); + return null; + } + } +} diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java index 5984155a606e..48d016dee666 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java @@ -5,7 +5,7 @@ package io.opentelemetry.javaagent.instrumentation.jms; -import static io.opentelemetry.javaagent.instrumentation.jms.JmsTracer.tracer; +import static io.opentelemetry.javaagent.instrumentation.jms.JmsInstrumenters.consumerInstrumenter; import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.AgentElementMatchers.implementsInterface; import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.ClassLoaderMatcher.hasClassesNamed; import static net.bytebuddy.matcher.ElementMatchers.isPublic; @@ -13,7 +13,10 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArguments; import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessageOperation; +import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; +import java.time.Instant; import java.util.HashMap; import java.util.Map; import javax.jms.Message; @@ -49,28 +52,27 @@ public Map, String> transfor public static class ConsumerAdvice { @Advice.OnMethodEnter - public static long onEnter() { - return System.currentTimeMillis(); + public static Instant onEnter() { + return Instant.now(); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( - @Advice.Enter long startTime, + @Advice.Enter Instant startTime, @Advice.Return Message message, @Advice.Thrown Throwable throwable) { - MessageDestination destination; if (message == null) { // Do not create span when no message is received return; } - destination = tracer().extractDestination(message, null); - Context context = tracer().startConsumerSpan(destination, "receive", message, startTime); + Context parentContext = Java8BytecodeBridge.currentContext(); + MessageWithDestination request = + MessageWithDestination.create(message, MessageOperation.RECEIVE, null); - if (throwable != null) { - tracer().endExceptionally(context, throwable); - } else { - tracer().end(context); + if (consumerInstrumenter().shouldStart(parentContext, request)) { + Context context = consumerInstrumenter().start(parentContext, request, startTime); + consumerInstrumenter().end(context, request, null, throwable); } } } diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageListenerInstrumentation.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageListenerInstrumentation.java index b5e972de5be0..ffd8773b652a 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageListenerInstrumentation.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageListenerInstrumentation.java @@ -5,7 +5,7 @@ package io.opentelemetry.javaagent.instrumentation.jms; -import static io.opentelemetry.javaagent.instrumentation.jms.JmsTracer.tracer; +import static io.opentelemetry.javaagent.instrumentation.jms.JmsInstrumenters.listenerInstrumenter; import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.AgentElementMatchers.implementsInterface; import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.ClassLoaderMatcher.hasClassesNamed; import static java.util.Collections.singletonMap; @@ -15,6 +15,8 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; +import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessageOperation; +import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; import java.util.Map; import javax.jms.Message; @@ -47,27 +49,32 @@ public static class MessageListenerAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) public static void onEnter( @Advice.Argument(0) Message message, + @Advice.Local("otelRequest") MessageWithDestination request, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { - MessageDestination destination = tracer().extractDestination(message, null); - context = - tracer().startConsumerSpan(destination, "process", message, System.currentTimeMillis()); + Context parentContext = Java8BytecodeBridge.currentContext(); + request = MessageWithDestination.create(message, MessageOperation.PROCESS, null); + + if (!listenerInstrumenter().shouldStart(parentContext, request)) { + return; + } + + context = listenerInstrumenter().start(parentContext, request); scope = context.makeCurrent(); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( + @Advice.Local("otelRequest") MessageWithDestination request, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope, @Advice.Thrown Throwable throwable) { - scope.close(); - - if (throwable != null) { - tracer().endExceptionally(context, throwable); - } else { - tracer().end(context); + if (scope == null) { + return; } + scope.close(); + listenerInstrumenter().end(context, request, null, throwable); } } } diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageProducerInstrumentation.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageProducerInstrumentation.java index 016bec033c89..6d1e7cc53696 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageProducerInstrumentation.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageProducerInstrumentation.java @@ -5,7 +5,7 @@ package io.opentelemetry.javaagent.instrumentation.jms; -import static io.opentelemetry.javaagent.instrumentation.jms.JmsTracer.tracer; +import static io.opentelemetry.javaagent.instrumentation.jms.JmsInstrumenters.producerInstrumenter; import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.AgentElementMatchers.implementsInterface; import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.ClassLoaderMatcher.hasClassesNamed; import static net.bytebuddy.matcher.ElementMatchers.isPublic; @@ -14,7 +14,9 @@ import io.opentelemetry.context.Context; import io.opentelemetry.context.Scope; +import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessageOperation; import io.opentelemetry.javaagent.instrumentation.api.CallDepthThreadLocalMap; +import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; import java.util.HashMap; import java.util.Map; @@ -60,6 +62,7 @@ public static class ProducerAdvice { public static void onEnter( @Advice.Argument(0) Message message, @Advice.This MessageProducer producer, + @Advice.Local("otelRequest") MessageWithDestination request, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { int callDepth = CallDepthThreadLocalMap.incrementCallDepth(MessageProducer.class); @@ -74,14 +77,19 @@ public static void onEnter( defaultDestination = null; } - MessageDestination messageDestination = - tracer().extractDestination(message, defaultDestination); - context = tracer().startProducerSpan(messageDestination, message); + Context parentContext = Java8BytecodeBridge.currentContext(); + request = MessageWithDestination.create(message, MessageOperation.SEND, defaultDestination); + if (!producerInstrumenter().shouldStart(parentContext, request)) { + return; + } + + context = producerInstrumenter().start(parentContext, request); scope = context.makeCurrent(); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( + @Advice.Local("otelRequest") MessageWithDestination request, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope, @Advice.Thrown Throwable throwable) { @@ -91,11 +99,7 @@ public static void stopSpan( CallDepthThreadLocalMap.reset(MessageProducer.class); scope.close(); - if (throwable != null) { - tracer().endExceptionally(context, throwable); - } else { - tracer().end(context); - } + producerInstrumenter().end(context, request, null, throwable); } } @@ -105,6 +109,7 @@ public static class ProducerWithDestinationAdvice { public static void onEnter( @Advice.Argument(0) Destination destination, @Advice.Argument(1) Message message, + @Advice.Local("otelRequest") MessageWithDestination request, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope) { int callDepth = CallDepthThreadLocalMap.incrementCallDepth(MessageProducer.class); @@ -112,13 +117,19 @@ public static void onEnter( return; } - MessageDestination messageDestination = tracer().extractDestination(message, destination); - context = tracer().startProducerSpan(messageDestination, message); + Context parentContext = Java8BytecodeBridge.currentContext(); + request = MessageWithDestination.create(message, MessageOperation.SEND, destination); + if (!producerInstrumenter().shouldStart(parentContext, request)) { + return; + } + + context = producerInstrumenter().start(parentContext, request); scope = context.makeCurrent(); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( + @Advice.Local("otelRequest") MessageWithDestination request, @Advice.Local("otelContext") Context context, @Advice.Local("otelScope") Scope scope, @Advice.Thrown Throwable throwable) { @@ -128,11 +139,7 @@ public static void stopSpan( CallDepthThreadLocalMap.reset(MessageProducer.class); scope.close(); - if (throwable != null) { - tracer().endExceptionally(context, throwable); - } else { - tracer().end(context); - } + producerInstrumenter().end(context, request, null, throwable); } } } diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsTracer.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsTracer.java deleted file mode 100644 index f71f9ed173dd..000000000000 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsTracer.java +++ /dev/null @@ -1,156 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.instrumentation.jms; - -import static io.opentelemetry.api.trace.SpanKind.CONSUMER; -import static io.opentelemetry.api.trace.SpanKind.PRODUCER; -import static io.opentelemetry.javaagent.instrumentation.jms.MessageExtractAdapter.GETTER; -import static io.opentelemetry.javaagent.instrumentation.jms.MessageInjectAdapter.SETTER; - -import io.opentelemetry.api.trace.SpanBuilder; -import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.api.tracer.BaseTracer; -import io.opentelemetry.semconv.trace.attributes.SemanticAttributes; -import java.util.concurrent.TimeUnit; -import javax.jms.Destination; -import javax.jms.JMSException; -import javax.jms.Message; -import javax.jms.Queue; -import javax.jms.TemporaryQueue; -import javax.jms.TemporaryTopic; -import javax.jms.Topic; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -public class JmsTracer extends BaseTracer { - private static final Logger log = LoggerFactory.getLogger(JmsTracer.class); - - // From the spec - public static final String TEMP_DESTINATION_NAME = "(temporary)"; - - private static final JmsTracer TRACER = new JmsTracer(); - - public static JmsTracer tracer() { - return TRACER; - } - - public Context startConsumerSpan( - MessageDestination destination, String operation, Message message, long startTime) { - Context parentContext = Context.root(); - if (message != null && "process".equals(operation)) { - parentContext = extract(message, GETTER); - } - - SpanBuilder spanBuilder = - spanBuilder(parentContext, spanName(destination, operation), CONSUMER) - .setStartTimestamp(startTime, TimeUnit.MILLISECONDS) - .setAttribute(SemanticAttributes.MESSAGING_OPERATION, operation); - - afterStart(spanBuilder, destination, message); - return parentContext.with(spanBuilder.startSpan()); - } - - public Context startProducerSpan(MessageDestination destination, Message message) { - Context parentContext = Context.current(); - SpanBuilder span = spanBuilder(parentContext, spanName(destination, "send"), PRODUCER); - afterStart(span, destination, message); - Context context = parentContext.with(span.startSpan()); - inject(context, message, SETTER); - return context; - } - - public String spanName(MessageDestination destination, String operation) { - if (destination.temporary) { - return TEMP_DESTINATION_NAME + " " + operation; - } else { - return destination.destinationName + " " + operation; - } - } - - private static final String TIBCO_TMP_PREFIX = "$TMP$"; - - public MessageDestination extractDestination(Message message, Destination fallbackDestination) { - Destination jmsDestination = null; - try { - jmsDestination = message.getJMSDestination(); - } catch (Exception ignored) { - } - - if (jmsDestination == null) { - jmsDestination = fallbackDestination; - } - - return extractMessageDestination(jmsDestination); - } - - public static MessageDestination extractMessageDestination(Destination destination) { - if (destination instanceof Queue) { - String queueName; - try { - queueName = ((Queue) destination).getQueueName(); - } catch (JMSException e) { - queueName = "unknown"; - } - - boolean temporary = - destination instanceof TemporaryQueue || queueName.startsWith(TIBCO_TMP_PREFIX); - - return new MessageDestination(queueName, "queue", temporary); - } - - if (destination instanceof Topic) { - String topicName; - try { - topicName = ((Topic) destination).getTopicName(); - } catch (JMSException e) { - topicName = "unknown"; - } - - boolean temporary = - destination instanceof TemporaryTopic || topicName.startsWith(TIBCO_TMP_PREFIX); - - return new MessageDestination(topicName, "topic", temporary); - } - - return MessageDestination.UNKNOWN; - } - - private void afterStart(SpanBuilder span, MessageDestination destination, Message message) { - span.setAttribute(SemanticAttributes.MESSAGING_SYSTEM, "jms"); - span.setAttribute(SemanticAttributes.MESSAGING_DESTINATION_KIND, destination.destinationKind); - if (destination.temporary) { - span.setAttribute(SemanticAttributes.MESSAGING_TEMP_DESTINATION, true); - span.setAttribute(SemanticAttributes.MESSAGING_DESTINATION, TEMP_DESTINATION_NAME); - } else { - span.setAttribute(SemanticAttributes.MESSAGING_DESTINATION, destination.destinationName); - } - - if (message != null) { - try { - String messageId = message.getJMSMessageID(); - if (messageId != null) { - span.setAttribute(SemanticAttributes.MESSAGING_MESSAGE_ID, messageId); - } - } catch (Exception e) { - log.debug("Failure getting JMS message id", e); - } - - try { - String correlationId = message.getJMSCorrelationID(); - if (correlationId != null) { - span.setAttribute(SemanticAttributes.MESSAGING_CONVERSATION_ID, correlationId); - } - } catch (Exception e) { - log.debug("Failure getting JMS correlation id", e); - } - } - } - - @Override - protected String getInstrumentationName() { - return "io.opentelemetry.javaagent.jms-1.1"; - } -} diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageDestination.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageDestination.java deleted file mode 100644 index 662913e85d06..000000000000 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageDestination.java +++ /dev/null @@ -1,21 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.instrumentation.jms; - -public class MessageDestination { - public static final MessageDestination UNKNOWN = - new MessageDestination("unknown", "unknown", false); - - public final String destinationName; - public final String destinationKind; - public final boolean temporary; - - public MessageDestination(String destinationName, String destinationKind, boolean temporary) { - this.destinationName = destinationName; - this.destinationKind = destinationKind; - this.temporary = temporary; - } -} diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageExtractAdapter.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessagePropertyGetter.java similarity index 56% rename from instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageExtractAdapter.java rename to instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessagePropertyGetter.java index 64233d942b43..026b2fafb8b0 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageExtractAdapter.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessagePropertyGetter.java @@ -8,27 +8,24 @@ import io.opentelemetry.context.propagation.TextMapGetter; import java.util.Collections; import javax.jms.JMSException; -import javax.jms.Message; -public class MessageExtractAdapter implements TextMapGetter { - - public static final MessageExtractAdapter GETTER = new MessageExtractAdapter(); +public final class MessagePropertyGetter implements TextMapGetter { @Override - public Iterable keys(Message message) { + public Iterable keys(MessageWithDestination message) { try { - return Collections.list(message.getPropertyNames()); + return Collections.list(message.getMessage().getPropertyNames()); } catch (JMSException e) { return Collections.emptyList(); } } @Override - public String get(Message carrier, String key) { - String propName = key.replace("-", MessageInjectAdapter.DASH); + public String get(MessageWithDestination carrier, String key) { + String propName = key.replace("-", MessagePropertySetter.DASH); Object value; try { - value = carrier.getObjectProperty(propName); + value = carrier.getMessage().getObjectProperty(propName); } catch (JMSException e) { throw new RuntimeException(e); } diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageInjectAdapter.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessagePropertySetter.java similarity index 66% rename from instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageInjectAdapter.java rename to instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessagePropertySetter.java index 0a79da5606c1..9da0899e21fd 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageInjectAdapter.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessagePropertySetter.java @@ -7,23 +7,19 @@ import io.opentelemetry.context.propagation.TextMapSetter; import javax.jms.JMSException; -import javax.jms.Message; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -public class MessageInjectAdapter implements TextMapSetter { - - private static final Logger log = LoggerFactory.getLogger(MessageInjectAdapter.class); - - public static final MessageInjectAdapter SETTER = new MessageInjectAdapter(); +public final class MessagePropertySetter implements TextMapSetter { + private static final Logger log = LoggerFactory.getLogger(MessagePropertySetter.class); static final String DASH = "__dash__"; @Override - public void set(Message carrier, String key, String value) { + public void set(MessageWithDestination carrier, String key, String value) { String propName = key.replace("-", DASH); try { - carrier.setStringProperty(propName, value); + carrier.getMessage().setStringProperty(propName, value); } catch (JMSException e) { if (log.isDebugEnabled()) { log.debug("Failure setting jms property: " + propName, e); diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestination.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestination.java new file mode 100644 index 000000000000..39aad4c915a1 --- /dev/null +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestination.java @@ -0,0 +1,108 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.jms; + +import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessageOperation; +import javax.jms.Destination; +import javax.jms.JMSException; +import javax.jms.Message; +import javax.jms.Queue; +import javax.jms.TemporaryQueue; +import javax.jms.TemporaryTopic; +import javax.jms.Topic; + +public final class MessageWithDestination { + private static final String TIBCO_TMP_PREFIX = "$TMP$"; + + private final Message message; + private final MessageOperation messageOperation; + private final String destinationName; + private final String destinationKind; + private final boolean temporaryDestination; + + MessageWithDestination( + Message message, + MessageOperation messageOperation, + String destinationName, + String destinationKind, + boolean temporary) { + this.message = message; + this.messageOperation = messageOperation; + this.destinationName = destinationName; + this.destinationKind = destinationKind; + this.temporaryDestination = temporary; + } + + public Message getMessage() { + return message; + } + + public MessageOperation getMessageOperation() { + return messageOperation; + } + + public String getDestinationName() { + return destinationName; + } + + public String getDestinationKind() { + return destinationKind; + } + + public boolean isTemporaryDestination() { + return temporaryDestination; + } + + public static MessageWithDestination create( + Message message, MessageOperation operation, Destination fallbackDestination) { + Destination jmsDestination = null; + try { + jmsDestination = message.getJMSDestination(); + } catch (Exception ignored) { + } + if (jmsDestination == null) { + jmsDestination = fallbackDestination; + } + + if (jmsDestination instanceof Queue) { + return createMessageWithQueue(message, operation, (Queue) jmsDestination); + } + if (jmsDestination instanceof Topic) { + return createMessageWithTopic(message, operation, (Topic) jmsDestination); + } + return new MessageWithDestination(message, operation, "unknown", "unknown", false); + } + + private static MessageWithDestination createMessageWithQueue( + Message message, MessageOperation operation, Queue destination) { + String queueName; + try { + queueName = destination.getQueueName(); + } catch (JMSException e) { + queueName = "unknown"; + } + + boolean temporary = + destination instanceof TemporaryQueue || queueName.startsWith(TIBCO_TMP_PREFIX); + + return new MessageWithDestination(message, operation, queueName, "queue", temporary); + } + + private static MessageWithDestination createMessageWithTopic( + Message message, MessageOperation operation, Topic destination) { + String topicName; + try { + topicName = destination.getTopicName(); + } catch (JMSException e) { + topicName = "unknown"; + } + + boolean temporary = + destination instanceof TemporaryTopic || topicName.startsWith(TIBCO_TMP_PREFIX); + + return new MessageWithDestination(message, operation, topicName, "topic", temporary); + } +} diff --git a/instrumentation/jms-1.1/javaagent/src/test/groovy/Jms1Test.groovy b/instrumentation/jms-1.1/javaagent/src/test/groovy/Jms1Test.groovy index aa87f6b9877b..fbff9af9f01f 100644 --- a/instrumentation/jms-1.1/javaagent/src/test/groovy/Jms1Test.groovy +++ b/instrumentation/jms-1.1/javaagent/src/test/groovy/Jms1Test.groovy @@ -278,6 +278,7 @@ class Jms1Test extends AgentInstrumentationSpecification { if (destinationName == "(temporary)") { "${SemanticAttributes.MESSAGING_TEMP_DESTINATION.key}" true } + "${SemanticAttributes.MESSAGING_MESSAGE_ID.key}" String } } } From 0030766b924f9e43fafc1853fbe8356d49a4bcb1 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Fri, 16 Apr 2021 11:58:53 +0200 Subject: [PATCH 2/7] Code review comments --- .../messaging/MessageOperation.java | 10 +- .../MessagingAttributesExtractor.java | 4 +- .../messaging/MessagingSpanNameExtractor.java | 4 +- .../messaging/MessageOperationTest.java | 2 +- .../MessagingAttributesExtractorTest.java | 8 +- .../MessagingSpanNameExtractorTest.java | 6 +- .../jms-1.1-javaagent-unit-tests.gradle | 9 ++ .../jms/MessageWithDestinationTest.java | 140 ++++++++++++++++++ .../JmsMessageConsumerInstrumentation.java | 2 +- .../JmsMessageListenerInstrumentation.java | 2 +- .../JmsMessageProducerInstrumentation.java | 4 +- .../jms/MessageWithDestination.java | 5 +- settings.gradle | 1 + 13 files changed, 172 insertions(+), 25 deletions(-) create mode 100644 instrumentation/jms-1.1/javaagent-unit-tests/jms-1.1-javaagent-unit-tests.gradle create mode 100644 instrumentation/jms-1.1/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestinationTest.java diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java index f00de665152e..474206cfcb54 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java @@ -11,11 +11,7 @@ * that may be used in a messaging system. */ public enum MessageOperation { - SEND, - RECEIVE, - PROCESS; - - public String operationName() { - return name().toLowerCase(); - } + send, + receive, + process } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java index e027be5453b7..83f16ee34456 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java @@ -47,8 +47,8 @@ protected final void onStart(AttributesBuilder attributes, REQUEST request) { SemanticAttributes.MESSAGING_MESSAGE_PAYLOAD_COMPRESSED_SIZE_BYTES, messagePayloadCompressedSize(request)); MessageOperation operation = operation(request); - if (operation == MessageOperation.RECEIVE || operation == MessageOperation.PROCESS) { - set(attributes, SemanticAttributes.MESSAGING_OPERATION, operation.operationName()); + if (operation == MessageOperation.receive || operation == MessageOperation.process) { + set(attributes, SemanticAttributes.MESSAGING_OPERATION, operation.name()); } } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractor.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractor.java index c8f71ffb9ded..729560cdb95a 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractor.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractor.java @@ -11,7 +11,7 @@ public final class MessagingSpanNameExtractor implements SpanNameExtrac /** * Returns a {@link SpanNameExtractor} that constructs the span name according to * messaging semantic conventions: {@code }. * * @see MessagingAttributesExtractor#destination(Object) used to extract {@code destinations() { return Stream.of( - Arguments.of(false, "destination", MessageOperation.RECEIVE, "destination"), - Arguments.of(true, null, MessageOperation.PROCESS, "(temporary)")); + Arguments.of(false, "destination", MessageOperation.receive, "destination"), + Arguments.of(true, null, MessageOperation.process, "(temporary)")); } @Test void shouldNotSetSendOperation() { // when AttributesBuilder attributes = Attributes.builder(); - underTest.onStart(attributes, singletonMap("operation", MessageOperation.SEND.name())); + underTest.onStart(attributes, singletonMap("operation", MessageOperation.send.name())); // then assertThat(attributes.build().isEmpty()).isTrue(); diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractorTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractorTest.java index d28f8a8f2380..0b2e4f02232a 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractorTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractorTest.java @@ -49,9 +49,9 @@ void shouldExtractSpanName( static Stream spanNameParams() { return Stream.of( - Arguments.of(false, "destination", MessageOperation.SEND, "destination send"), - Arguments.of(true, null, MessageOperation.PROCESS, "(temporary) process"), - Arguments.of(false, null, MessageOperation.RECEIVE, "unknown receive"), + Arguments.of(false, "destination", MessageOperation.send, "destination send"), + Arguments.of(true, null, MessageOperation.process, "(temporary) process"), + Arguments.of(false, null, MessageOperation.receive, "unknown receive"), Arguments.of(false, "destination", null, "destination")); } diff --git a/instrumentation/jms-1.1/javaagent-unit-tests/jms-1.1-javaagent-unit-tests.gradle b/instrumentation/jms-1.1/javaagent-unit-tests/jms-1.1-javaagent-unit-tests.gradle new file mode 100644 index 000000000000..e66a841cd6f2 --- /dev/null +++ b/instrumentation/jms-1.1/javaagent-unit-tests/jms-1.1-javaagent-unit-tests.gradle @@ -0,0 +1,9 @@ +apply from: "$rootDir/gradle/java.gradle" + +dependencies { + testImplementation group: 'javax.jms', name: 'jms-api', version: '1.1-rev-1' + testImplementation project(':instrumentation:jms-1.1:javaagent') + testImplementation project(':instrumentation-api') + + testImplementation deps.mockito +} diff --git a/instrumentation/jms-1.1/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestinationTest.java b/instrumentation/jms-1.1/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestinationTest.java new file mode 100644 index 000000000000..9d5fd8460743 --- /dev/null +++ b/instrumentation/jms-1.1/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestinationTest.java @@ -0,0 +1,140 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.jms; + +import static io.opentelemetry.javaagent.instrumentation.jms.MessageWithDestination.TIBCO_TMP_PREFIX; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.mockito.BDDMockito.given; + +import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessageOperation; +import java.util.stream.Stream; +import javax.jms.Destination; +import javax.jms.JMSException; +import javax.jms.Message; +import javax.jms.Queue; +import javax.jms.TemporaryQueue; +import javax.jms.TemporaryTopic; +import javax.jms.Topic; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +@ExtendWith(MockitoExtension.class) +class MessageWithDestinationTest { + @Mock Message message; + @Mock Topic topic; + @Mock TemporaryTopic temporaryTopic; + @Mock Queue queue; + @Mock TemporaryQueue temporaryQueue; + @Mock Destination destination; + + @Test + void shouldCreateMessageWithUnknownDestination() throws JMSException { + // given + given(message.getJMSDestination()).willReturn(destination); + + // when + MessageWithDestination result = + MessageWithDestination.create(message, MessageOperation.send, null); + + // then + assertMessage(MessageOperation.send, "unknown", "unknown", false, result); + } + + @Test + void shouldUseFallbackDestinationToCreateMessage() throws JMSException { + // given + given(message.getJMSDestination()).willThrow(JMSException.class); + + // when + MessageWithDestination result = + MessageWithDestination.create(message, MessageOperation.send, destination); + + // then + assertMessage(MessageOperation.send, "unknown", "unknown", false, result); + } + + @ParameterizedTest + @MethodSource("destinations") + void shouldCreateMessageWithQueue( + String queueName, + boolean useTemporaryDestination, + String expectedDestinationName, + boolean expectedTemporary) + throws JMSException { + // given + Queue queue = useTemporaryDestination ? this.temporaryQueue : this.queue; + + given(message.getJMSDestination()).willReturn(queue); + if (queueName == null) { + given(queue.getQueueName()).willThrow(JMSException.class); + } else { + given(queue.getQueueName()).willReturn(queueName); + } + + // when + MessageWithDestination result = + MessageWithDestination.create(message, MessageOperation.receive, null); + + // then + assertMessage( + MessageOperation.receive, "queue", expectedDestinationName, expectedTemporary, result); + } + + @ParameterizedTest + @MethodSource("destinations") + void shouldCreateMessageWithTopic( + String topicName, + boolean useTemporaryDestination, + String expectedDestinationName, + boolean expectedTemporary) + throws JMSException { + // given + Topic topic = useTemporaryDestination ? this.temporaryTopic : this.topic; + + given(message.getJMSDestination()).willReturn(topic); + if (topicName == null) { + given(topic.getTopicName()).willThrow(JMSException.class); + } else { + given(topic.getTopicName()).willReturn(topicName); + } + + // when + MessageWithDestination result = + MessageWithDestination.create(message, MessageOperation.receive, null); + + // then + assertMessage( + MessageOperation.receive, "topic", expectedDestinationName, expectedTemporary, result); + } + + static Stream destinations() { + return Stream.of( + Arguments.of("destination", false, "destination", false), + Arguments.of(null, false, "unknown", false), + Arguments.of(TIBCO_TMP_PREFIX + "dest", false, TIBCO_TMP_PREFIX + "dest", true), + Arguments.of("destination", true, "destination", true)); + } + + private void assertMessage( + MessageOperation expectedMessageOperation, + String expectedDestinationKind, + String expectedDestinationName, + boolean expectedTemporary, + MessageWithDestination actual) { + + assertSame(message, actual.getMessage()); + assertSame(expectedMessageOperation, actual.getMessageOperation()); + assertEquals(expectedDestinationKind, actual.getDestinationKind()); + assertEquals(expectedDestinationName, actual.getDestinationName()); + assertEquals(expectedTemporary, actual.isTemporaryDestination()); + } +} diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java index 48d016dee666..6f94fdda8e2f 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java @@ -68,7 +68,7 @@ public static void stopSpan( Context parentContext = Java8BytecodeBridge.currentContext(); MessageWithDestination request = - MessageWithDestination.create(message, MessageOperation.RECEIVE, null); + MessageWithDestination.create(message, MessageOperation.receive, null); if (consumerInstrumenter().shouldStart(parentContext, request)) { Context context = consumerInstrumenter().start(parentContext, request, startTime); diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageListenerInstrumentation.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageListenerInstrumentation.java index ffd8773b652a..1d971389eb52 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageListenerInstrumentation.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageListenerInstrumentation.java @@ -54,7 +54,7 @@ public static void onEnter( @Advice.Local("otelScope") Scope scope) { Context parentContext = Java8BytecodeBridge.currentContext(); - request = MessageWithDestination.create(message, MessageOperation.PROCESS, null); + request = MessageWithDestination.create(message, MessageOperation.process, null); if (!listenerInstrumenter().shouldStart(parentContext, request)) { return; diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageProducerInstrumentation.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageProducerInstrumentation.java index 6d1e7cc53696..0aba25e19f73 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageProducerInstrumentation.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageProducerInstrumentation.java @@ -78,7 +78,7 @@ public static void onEnter( } Context parentContext = Java8BytecodeBridge.currentContext(); - request = MessageWithDestination.create(message, MessageOperation.SEND, defaultDestination); + request = MessageWithDestination.create(message, MessageOperation.send, defaultDestination); if (!producerInstrumenter().shouldStart(parentContext, request)) { return; } @@ -118,7 +118,7 @@ public static void onEnter( } Context parentContext = Java8BytecodeBridge.currentContext(); - request = MessageWithDestination.create(message, MessageOperation.SEND, destination); + request = MessageWithDestination.create(message, MessageOperation.send, destination); if (!producerInstrumenter().shouldStart(parentContext, request)) { return; } diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestination.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestination.java index 39aad4c915a1..34ae616ba028 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestination.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestination.java @@ -15,7 +15,8 @@ import javax.jms.Topic; public final class MessageWithDestination { - private static final String TIBCO_TMP_PREFIX = "$TMP$"; + // visible for tests + static final String TIBCO_TMP_PREFIX = "$TMP$"; private final Message message; private final MessageOperation messageOperation; @@ -23,7 +24,7 @@ public final class MessageWithDestination { private final String destinationKind; private final boolean temporaryDestination; - MessageWithDestination( + private MessageWithDestination( Message message, MessageOperation messageOperation, String destinationName, diff --git a/settings.gradle b/settings.gradle index 8e83dd7c2ecd..f7f16ce1dc00 100644 --- a/settings.gradle +++ b/settings.gradle @@ -160,6 +160,7 @@ include ':instrumentation:jetty:jetty-8.0:javaagent' include ':instrumentation:jetty:jetty-11.0:javaagent' include ':instrumentation:jetty:jetty-common:javaagent' include ':instrumentation:jms-1.1:javaagent' +include ':instrumentation:jms-1.1:javaagent-unit-tests' include ':instrumentation:jsf:jsf-common:library' include ':instrumentation:jsf:jsf-testing-common' include ':instrumentation:jsf:mojarra-1.2:javaagent' From 56d8a6293f0566a9d20793d60fddb035c77ac2e4 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Wed, 28 Apr 2021 16:43:11 +0200 Subject: [PATCH 3/7] Use Instrumenter in JMS instrumentation - code review comments --- .../api/instrumenter/ClientInstrumenter.java | 31 +---- .../api/instrumenter/CurrentNanoTime.java | 79 +++++++++++++ .../api/instrumenter/EndTimeExtractor.java | 23 ++++ .../api/instrumenter/Instrumenter.java | 54 +++------ .../api/instrumenter/InstrumenterBuilder.java | 107 +++++++----------- .../api/instrumenter/ServerInstrumenter.java | 31 +---- .../api/instrumenter/StartTimeExtractor.java | 23 ++++ .../api/instrumenter/CurrentNanoTimeTest.java | 34 ++++++ .../api/instrumenter/InstrumenterTest.java | 16 ++- .../jms/MessageWithDestinationTest.java | 27 ++++- .../instrumentation/jms/JmsInstrumenters.java | 1 + .../JmsMessageConsumerInstrumentation.java | 7 +- .../jms/JmsStartTimeExtractor.java | 20 ++++ .../jms/MessageWithDestination.java | 34 ++++-- 14 files changed, 311 insertions(+), 176 deletions(-) create mode 100644 instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTime.java create mode 100644 instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/EndTimeExtractor.java create mode 100644 instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/StartTimeExtractor.java create mode 100644 instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTimeTest.java create mode 100644 instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsStartTimeExtractor.java diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ClientInstrumenter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ClientInstrumenter.java index 681c7a1fb2a5..fd0053a1f8a3 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ClientInstrumenter.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ClientInstrumenter.java @@ -5,44 +5,25 @@ package io.opentelemetry.instrumentation.api.instrumenter; -import io.opentelemetry.api.trace.Tracer; import io.opentelemetry.context.Context; import io.opentelemetry.context.propagation.ContextPropagators; import io.opentelemetry.context.propagation.TextMapSetter; -import java.time.Instant; -import java.util.List; -import org.checkerframework.checker.nullness.qual.Nullable; final class ClientInstrumenter extends Instrumenter { private final ContextPropagators propagators; private final TextMapSetter setter; - ClientInstrumenter( - String instrumentationName, - Tracer tracer, - SpanNameExtractor spanNameExtractor, - SpanKindExtractor spanKindExtractor, - SpanStatusExtractor spanStatusExtractor, - List> attributesExtractors, - ErrorCauseExtractor errorCauseExtractor, - ContextPropagators propagators, - TextMapSetter setter) { - super( - instrumentationName, - tracer, - spanNameExtractor, - spanKindExtractor, - spanStatusExtractor, - attributesExtractors, - errorCauseExtractor); - this.propagators = propagators; + public ClientInstrumenter( + InstrumenterBuilder builder, TextMapSetter setter) { + super(builder); + this.propagators = builder.openTelemetry.getPropagators(); this.setter = setter; } @Override - public Context start(Context parentContext, REQUEST request, @Nullable Instant startTime) { - Context newContext = super.start(parentContext, request, startTime); + public Context start(Context parentContext, REQUEST request) { + Context newContext = super.start(parentContext, request); propagators.getTextMapPropagator().inject(newContext, request, setter); return newContext; } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTime.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTime.java new file mode 100644 index 000000000000..b5447548db48 --- /dev/null +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTime.java @@ -0,0 +1,79 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.instrumenter; + +import java.time.Clock; +import java.time.Instant; +import java.time.ZoneId; +import java.time.ZoneOffset; +import java.util.concurrent.TimeUnit; +import java.util.function.LongSupplier; + +/** + * This utility class allows to retrieve an {@link Instant} with the current time measured with + * nanosecond precision. + */ +public final class CurrentNanoTime { + private static final Clock CLOCK; + + static { + Clock clock; + try { + // Runtime#version() was added in Java 9 + Runtime.class.getDeclaredMethod("version"); + // Java 9+ SystemClock has nano precision + clock = Clock.systemUTC(); + } catch (NoSuchMethodException e) { + // Java 8 doesn't, so we have to compute the correct timestamp ourselves + clock = new Java8NanoClock(); + } + CLOCK = clock; + } + + /** Returns an {@link Instant} with the current time measured with nanosecond precision. */ + public static Instant get() { + return CLOCK.instant(); + } + + // reused logic from the SDK MonotonicClock + static final class Java8NanoClock extends Clock { + private final long epochNanos; + private final LongSupplier nanoTimeSupplier; + private final long nanoTimeOffset; + + private Java8NanoClock() { + this(TimeUnit.MILLISECONDS.toNanos(System.currentTimeMillis()), System::nanoTime); + } + + // visible for tests + Java8NanoClock(long epochNanos, LongSupplier nanoTimeSupplier) { + this.epochNanos = epochNanos; + this.nanoTimeSupplier = nanoTimeSupplier; + this.nanoTimeOffset = nanoTimeSupplier.getAsLong(); + } + + @Override + public ZoneId getZone() { + return ZoneOffset.UTC; + } + + @Override + public Clock withZone(ZoneId zone) { + throw new UnsupportedOperationException(); + } + + @Override + public Instant instant() { + long deltaNanos = nanoTimeSupplier.getAsLong() - this.nanoTimeOffset; + long nowNanos = epochNanos + deltaNanos; + long seconds = TimeUnit.NANOSECONDS.toSeconds(nowNanos); + long nanoAdjustment = nowNanos - TimeUnit.SECONDS.toNanos(seconds); + return Instant.ofEpochSecond(seconds, nanoAdjustment); + } + } + + private CurrentNanoTime() {} +} diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/EndTimeExtractor.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/EndTimeExtractor.java new file mode 100644 index 000000000000..31946a30b23a --- /dev/null +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/EndTimeExtractor.java @@ -0,0 +1,23 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.instrumenter; + +import java.time.Instant; + +/** Extractor of the end time of response processing. */ +public interface EndTimeExtractor { + + /** + * Returns the default {@link EndTimeExtractor}, which always returns the current time with + * nanosecond precision. + */ + static EndTimeExtractor getDefault() { + return response -> CurrentNanoTime.get(); + } + + /** Returns the timestamp marking the end of the response processing. */ + Instant extract(RESPONSE response); +} diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java index 6fa445b34e0a..a7f23d670b28 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java @@ -17,7 +17,7 @@ import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics; import io.opentelemetry.instrumentation.api.tracer.ClientSpan; import io.opentelemetry.instrumentation.api.tracer.ServerSpan; -import java.time.Instant; +import java.util.ArrayList; import java.util.List; import org.checkerframework.checker.nullness.qual.Nullable; @@ -57,22 +57,19 @@ public static InstrumenterBuilder newBuil private final SpanStatusExtractor spanStatusExtractor; private final List> extractors; private final ErrorCauseExtractor errorCauseExtractor; - - Instrumenter( - String instrumentationName, - Tracer tracer, - SpanNameExtractor spanNameExtractor, - SpanKindExtractor spanKindExtractor, - SpanStatusExtractor spanStatusExtractor, - List> extractors, - ErrorCauseExtractor errorCauseExtractor) { - this.instrumentationName = instrumentationName; - this.tracer = tracer; - this.spanNameExtractor = spanNameExtractor; - this.spanKindExtractor = spanKindExtractor; - this.spanStatusExtractor = spanStatusExtractor; - this.extractors = extractors; - this.errorCauseExtractor = errorCauseExtractor; + private final StartTimeExtractor startTimeExtractor; + private final EndTimeExtractor endTimeExtractor; + + Instrumenter(InstrumenterBuilder builder) { + this.instrumentationName = builder.instrumentationName; + this.tracer = builder.openTelemetry.getTracer(builder.instrumentationName); + this.spanNameExtractor = builder.spanNameExtractor; + this.spanKindExtractor = builder.spanKindExtractor; + this.spanStatusExtractor = builder.spanStatusExtractor; + this.extractors = new ArrayList<>(builder.attributesExtractors); + this.errorCauseExtractor = builder.errorCauseExtractor; + this.startTimeExtractor = builder.startTimeExtractor; + this.endTimeExtractor = builder.endTimeExtractor; } /** @@ -107,29 +104,14 @@ public boolean shouldStart(Context parentContext, REQUEST request) { * propagated along with the operation and passed to {@link #end(Context, Object, Object, * Throwable)} when it is finished. */ - public final Context start(Context parentContext, REQUEST request) { - return start(parentContext, request, null); - } - - /** - * Starts a new operation to be instrumented. The {@code parentContext} is the parent of the - * resulting instrumented operation and should usually be {@code Context.current()}. The {@code - * request} is the request object of this operation. The {@code startTime} marks the moment when - * the operation has started. If not provided, OpenTelemetry uses current time as the operation - * start. The returned {@link Context} should be propagated along with the operation and passed to - * {@link #end(Context, Object, Object, Throwable)} when it is finished. - */ - public Context start(Context parentContext, REQUEST request, @Nullable Instant startTime) { + public Context start(Context parentContext, REQUEST request) { SpanKind spanKind = spanKindExtractor.extract(request); SpanBuilder spanBuilder = tracer .spanBuilder(spanNameExtractor.extract(request)) .setSpanKind(spanKind) - .setParent(parentContext); - - if (startTime != null) { - spanBuilder.setStartTimestamp(startTime); - } + .setParent(parentContext) + .setStartTimestamp(startTimeExtractor.extract(request)); AttributesBuilder attributes = Attributes.builder(); for (AttributesExtractor extractor : extractors) { @@ -171,6 +153,6 @@ public void end(Context context, REQUEST request, RESPONSE response, @Nullable T span.setStatus(spanStatusExtractor.extract(request, response, error)); - span.end(); + span.end(endTimeExtractor.extract(response)); } } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java index 2a09221eda08..c02a9a63d50a 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java @@ -7,11 +7,8 @@ import io.opentelemetry.api.OpenTelemetry; import io.opentelemetry.api.trace.StatusCode; -import io.opentelemetry.api.trace.Tracer; -import io.opentelemetry.context.propagation.ContextPropagators; import io.opentelemetry.context.propagation.TextMapGetter; import io.opentelemetry.context.propagation.TextMapSetter; -import io.opentelemetry.instrumentation.api.InstrumentationVersion; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -22,16 +19,19 @@ * {@link Instrumenter}. */ public final class InstrumenterBuilder { - private final OpenTelemetry openTelemetry; - private final String instrumentationName; - private final SpanNameExtractor spanNameExtractor; + final OpenTelemetry openTelemetry; + final String instrumentationName; + final SpanNameExtractor spanNameExtractor; - private final List> attributesExtractors = + final List> attributesExtractors = new ArrayList<>(); - private SpanStatusExtractor spanStatusExtractor = + SpanKindExtractor spanKindExtractor; + SpanStatusExtractor spanStatusExtractor = SpanStatusExtractor.getDefault(); - private ErrorCauseExtractor errorCauseExtractor = ErrorCauseExtractor.jdk(); + ErrorCauseExtractor errorCauseExtractor = ErrorCauseExtractor.jdk(); + StartTimeExtractor startTimeExtractor = StartTimeExtractor.getDefault(); + EndTimeExtractor endTimeExtractor = EndTimeExtractor.getDefault(); InstrumenterBuilder( OpenTelemetry openTelemetry, @@ -82,14 +82,33 @@ public InstrumenterBuilder setErrorCauseExtractor( return this; } + /** + * Sets the {@link StartTimeExtractor} to extract the timestamp marking the start of the request + * processing. + */ + public InstrumenterBuilder setStartTimeExtractor( + StartTimeExtractor startTimeExtractor) { + this.startTimeExtractor = startTimeExtractor; + return this; + } + + /** + * Sets the {@link StartTimeExtractor} to extract the timestamp marking the end of the response + * processing. + */ + public InstrumenterBuilder setEndTimeExtractor( + EndTimeExtractor endTimeExtractor) { + this.endTimeExtractor = endTimeExtractor; + return this; + } + /** * Returns a new {@link Instrumenter} which will create client spans and inject context into * requests. */ public Instrumenter newClientInstrumenter(TextMapSetter setter) { return newInstrumenter( - InstrumenterConstructor.propagatingToDownstream(openTelemetry.getPropagators(), setter), - SpanKindExtractor.alwaysClient()); + InstrumenterConstructor.propagatingToDownstream(setter), SpanKindExtractor.alwaysClient()); } /** @@ -98,8 +117,7 @@ public Instrumenter newClientInstrumenter(TextMapSetter newServerInstrumenter(TextMapGetter getter) { return newInstrumenter( - InstrumenterConstructor.propagatingFromUpstream(openTelemetry.getPropagators(), getter), - SpanKindExtractor.alwaysServer()); + InstrumenterConstructor.propagatingFromUpstream(getter), SpanKindExtractor.alwaysServer()); } /** @@ -108,7 +126,7 @@ public Instrumenter newServerInstrumenter(TextMapGetter newProducerInstrumenter(TextMapSetter setter) { return newInstrumenter( - InstrumenterConstructor.propagatingToDownstream(openTelemetry.getPropagators(), setter), + InstrumenterConstructor.propagatingToDownstream(setter), SpanKindExtractor.alwaysProducer()); } @@ -118,7 +136,7 @@ public Instrumenter newProducerInstrumenter(TextMapSetter newConsumerInstrumenter(TextMapGetter getter) { return newInstrumenter( - InstrumenterConstructor.propagatingFromUpstream(openTelemetry.getPropagators(), getter), + InstrumenterConstructor.propagatingFromUpstream(getter), SpanKindExtractor.alwaysConsumer()); } @@ -142,70 +160,25 @@ public Instrumenter newInstrumenter( private Instrumenter newInstrumenter( InstrumenterConstructor constructor, SpanKindExtractor spanKindExtractor) { - return constructor.create( - instrumentationName, - openTelemetry.getTracer(instrumentationName, InstrumentationVersion.VERSION), - spanNameExtractor, - spanKindExtractor, - spanStatusExtractor, - new ArrayList<>(attributesExtractors), - errorCauseExtractor); + this.spanKindExtractor = spanKindExtractor; + return constructor.create(this); } private interface InstrumenterConstructor { - Instrumenter create( - String instrumentationName, - Tracer tracer, - SpanNameExtractor spanNameExtractor, - SpanKindExtractor spanKindExtractor, - SpanStatusExtractor spanStatusExtractor, - List> extractors, - ErrorCauseExtractor errorCauseExtractor); + Instrumenter create(InstrumenterBuilder builder); static InstrumenterConstructor internal() { return Instrumenter::new; } static InstrumenterConstructor propagatingToDownstream( - ContextPropagators propagators, TextMapSetter setter) { - return (instrumentationName, - tracer, - spanName, - spanKind, - spanStatus, - attributes, - errorCauseExtractor) -> - new ClientInstrumenter<>( - instrumentationName, - tracer, - spanName, - spanKind, - spanStatus, - attributes, - errorCauseExtractor, - propagators, - setter); + TextMapSetter setter) { + return builder -> new ClientInstrumenter<>(builder, setter); } static InstrumenterConstructor propagatingFromUpstream( - ContextPropagators propagators, TextMapGetter getter) { - return (instrumentationName, - tracer, - spanName, - spanKind, - spanStatus, - attributes, - errorCauseExtractor) -> - new ServerInstrumenter<>( - instrumentationName, - tracer, - spanName, - spanKind, - spanStatus, - attributes, - errorCauseExtractor, - propagators, - getter); + TextMapGetter getter) { + return builder -> new ServerInstrumenter<>(builder, getter); } } } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java index 4957e76e45f0..fa55d58fd9ca 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/ServerInstrumenter.java @@ -5,47 +5,28 @@ package io.opentelemetry.instrumentation.api.instrumenter; -import io.opentelemetry.api.trace.Tracer; import io.opentelemetry.context.Context; import io.opentelemetry.context.propagation.ContextPropagators; import io.opentelemetry.context.propagation.TextMapGetter; import io.opentelemetry.instrumentation.api.internal.ContextPropagationDebug; -import java.time.Instant; -import java.util.List; -import org.checkerframework.checker.nullness.qual.Nullable; final class ServerInstrumenter extends Instrumenter { private final ContextPropagators propagators; private final TextMapGetter getter; - ServerInstrumenter( - String instrumentationName, - Tracer tracer, - SpanNameExtractor spanNameExtractor, - SpanKindExtractor spanKindExtractor, - SpanStatusExtractor spanStatusExtractor, - List> attributesExtractors, - ErrorCauseExtractor errorCauseExtractor, - ContextPropagators propagators, - TextMapGetter getter) { - super( - instrumentationName, - tracer, - spanNameExtractor, - spanKindExtractor, - spanStatusExtractor, - attributesExtractors, - errorCauseExtractor); - this.propagators = propagators; + public ServerInstrumenter( + InstrumenterBuilder builder, TextMapGetter getter) { + super(builder); + this.propagators = builder.openTelemetry.getPropagators(); this.getter = getter; } @Override - public Context start(Context parentContext, REQUEST request, @Nullable Instant startTime) { + public Context start(Context parentContext, REQUEST request) { ContextPropagationDebug.debugContextLeakIfEnabled(); Context extracted = propagators.getTextMapPropagator().extract(parentContext, request, getter); - return super.start(extracted, request, startTime); + return super.start(extracted, request); } } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/StartTimeExtractor.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/StartTimeExtractor.java new file mode 100644 index 000000000000..8f36abbe43eb --- /dev/null +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/StartTimeExtractor.java @@ -0,0 +1,23 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.instrumenter; + +import java.time.Instant; + +/** Extractor of the start time of request processing. */ +public interface StartTimeExtractor { + + /** + * Returns the default {@link StartTimeExtractor}, which always returns the current time with + * nanosecond precision. + */ + static StartTimeExtractor getDefault() { + return request -> CurrentNanoTime.get(); + } + + /** Returns the timestamp marking the start of the request processing. */ + Instant extract(REQUEST request); +} diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTimeTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTimeTest.java new file mode 100644 index 000000000000..1ad5c0bf6d59 --- /dev/null +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTimeTest.java @@ -0,0 +1,34 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.api.instrumenter; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import java.time.Clock; +import java.time.Instant; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; +import org.junit.jupiter.api.Test; + +class CurrentNanoTimeTest { + @Test + void shouldCreateAnInstantWithNanosecondPrecision() { + // given + long epochNanoseconds = TimeUnit.SECONDS.toNanos(10); + AtomicLong nanoseconds = new AtomicLong(1_000); + + Clock java8Clock = new CurrentNanoTime.Java8NanoClock(epochNanoseconds, nanoseconds::get); + + nanoseconds.set(1_000_002_000); + + // when + Instant result = java8Clock.instant(); + + // then + assertEquals(11, result.getEpochSecond()); + assertEquals(1_000, result.getNano()); + } +} diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java index af93b3a5c310..84b51e0b50bd 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java @@ -319,16 +319,20 @@ void client_parent() { @Test void shouldStartSpanWithGivenStartTime() { - Instrumenter instrumenter = - Instrumenter.newBuilder( - otelTesting.getOpenTelemetry(), "test", request -> request) + // given + Instrumenter instrumenter = + Instrumenter.newBuilder( + otelTesting.getOpenTelemetry(), "test", request -> "test span") + .setStartTimeExtractor(request -> request) + .setEndTimeExtractor(response -> response) .newInstrumenter(); Instant startTime = Instant.ofEpochSecond(100); + Instant endTime = Instant.ofEpochSecond(123); // when - Context context = instrumenter.start(Context.root(), "test span", startTime); - instrumenter.end(context, "test span", null, null); + Context context = instrumenter.start(Context.root(), startTime); + instrumenter.end(context, startTime, endTime, null); // then otelTesting @@ -336,6 +340,6 @@ void shouldStartSpanWithGivenStartTime() { .hasTracesSatisfyingExactly( trace -> trace.hasSpansSatisfyingExactly( - span -> span.hasName("test span").startsAt(startTime))); + span -> span.hasName("test span").startsAt(startTime).endsAt(endTime))); } } diff --git a/instrumentation/jms-1.1/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestinationTest.java b/instrumentation/jms-1.1/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestinationTest.java index 9d5fd8460743..328e3d888541 100644 --- a/instrumentation/jms-1.1/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestinationTest.java +++ b/instrumentation/jms-1.1/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestinationTest.java @@ -11,6 +11,7 @@ import static org.mockito.BDDMockito.given; import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessageOperation; +import java.time.Instant; import java.util.stream.Stream; import javax.jms.Destination; import javax.jms.JMSException; @@ -29,6 +30,8 @@ @ExtendWith(MockitoExtension.class) class MessageWithDestinationTest { + private static final Instant START_TIME = Instant.ofEpochSecond(42); + @Mock Message message; @Mock Topic topic; @Mock TemporaryTopic temporaryTopic; @@ -43,10 +46,10 @@ void shouldCreateMessageWithUnknownDestination() throws JMSException { // when MessageWithDestination result = - MessageWithDestination.create(message, MessageOperation.send, null); + MessageWithDestination.create(message, MessageOperation.send, null, START_TIME); // then - assertMessage(MessageOperation.send, "unknown", "unknown", false, result); + assertMessage(MessageOperation.send, "unknown", "unknown", false, START_TIME, result); } @Test @@ -56,10 +59,10 @@ void shouldUseFallbackDestinationToCreateMessage() throws JMSException { // when MessageWithDestination result = - MessageWithDestination.create(message, MessageOperation.send, destination); + MessageWithDestination.create(message, MessageOperation.send, destination, START_TIME); // then - assertMessage(MessageOperation.send, "unknown", "unknown", false, result); + assertMessage(MessageOperation.send, "unknown", "unknown", false, START_TIME, result); } @ParameterizedTest @@ -86,7 +89,12 @@ void shouldCreateMessageWithQueue( // then assertMessage( - MessageOperation.receive, "queue", expectedDestinationName, expectedTemporary, result); + MessageOperation.receive, + "queue", + expectedDestinationName, + expectedTemporary, + null, + result); } @ParameterizedTest @@ -113,7 +121,12 @@ void shouldCreateMessageWithTopic( // then assertMessage( - MessageOperation.receive, "topic", expectedDestinationName, expectedTemporary, result); + MessageOperation.receive, + "topic", + expectedDestinationName, + expectedTemporary, + null, + result); } static Stream destinations() { @@ -129,6 +142,7 @@ private void assertMessage( String expectedDestinationKind, String expectedDestinationName, boolean expectedTemporary, + Instant expectedStartTime, MessageWithDestination actual) { assertSame(message, actual.getMessage()); @@ -136,5 +150,6 @@ private void assertMessage( assertEquals(expectedDestinationKind, actual.getDestinationKind()); assertEquals(expectedDestinationName, actual.getDestinationName()); assertEquals(expectedTemporary, actual.isTemporaryDestination()); + assertEquals(expectedStartTime, actual.getStartTime()); } } diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsInstrumenters.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsInstrumenters.java index 865218fcb3db..bac5cc5a9cc5 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsInstrumenters.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsInstrumenters.java @@ -35,6 +35,7 @@ public final class JmsInstrumenters { Instrumenter.newBuilder( otel, INSTRUMENTATION_NAME, spanNameExtractor) .addAttributesExtractor(attributesExtractor) + .setStartTimeExtractor(new JmsStartTimeExtractor()) .newInstrumenter(SpanKindExtractor.alwaysConsumer()); LISTENER_INSTRUMENTER = Instrumenter.newBuilder( diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java index 6f94fdda8e2f..301b69cd9228 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java @@ -13,6 +13,7 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArguments; import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.instrumenter.CurrentNanoTime; import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessageOperation; import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; @@ -53,7 +54,7 @@ public static class ConsumerAdvice { @Advice.OnMethodEnter public static Instant onEnter() { - return Instant.now(); + return CurrentNanoTime.get(); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) @@ -68,10 +69,10 @@ public static void stopSpan( Context parentContext = Java8BytecodeBridge.currentContext(); MessageWithDestination request = - MessageWithDestination.create(message, MessageOperation.receive, null); + MessageWithDestination.create(message, MessageOperation.receive, null, startTime); if (consumerInstrumenter().shouldStart(parentContext, request)) { - Context context = consumerInstrumenter().start(parentContext, request, startTime); + Context context = consumerInstrumenter().start(parentContext, request); consumerInstrumenter().end(context, request, null, throwable); } } diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsStartTimeExtractor.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsStartTimeExtractor.java new file mode 100644 index 000000000000..ea6a4c4eae7e --- /dev/null +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsStartTimeExtractor.java @@ -0,0 +1,20 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.jms; + +import io.opentelemetry.instrumentation.api.instrumenter.StartTimeExtractor; +import java.time.Instant; + +public final class JmsStartTimeExtractor implements StartTimeExtractor { + private final StartTimeExtractor fallback = + StartTimeExtractor.getDefault(); + + @Override + public Instant extract(MessageWithDestination message) { + Instant startTime = message.getStartTime(); + return startTime != null ? startTime : fallback.extract(message); + } +} diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestination.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestination.java index 34ae616ba028..5786eeb25f48 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestination.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestination.java @@ -6,6 +6,7 @@ package io.opentelemetry.javaagent.instrumentation.jms; import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessageOperation; +import java.time.Instant; import javax.jms.Destination; import javax.jms.JMSException; import javax.jms.Message; @@ -13,6 +14,7 @@ import javax.jms.TemporaryQueue; import javax.jms.TemporaryTopic; import javax.jms.Topic; +import org.checkerframework.checker.nullness.qual.Nullable; public final class MessageWithDestination { // visible for tests @@ -23,18 +25,21 @@ public final class MessageWithDestination { private final String destinationName; private final String destinationKind; private final boolean temporaryDestination; + private final Instant startTime; private MessageWithDestination( Message message, MessageOperation messageOperation, String destinationName, String destinationKind, - boolean temporary) { + boolean temporary, + Instant startTime) { this.message = message; this.messageOperation = messageOperation; this.destinationName = destinationName; this.destinationKind = destinationKind; this.temporaryDestination = temporary; + this.startTime = startTime; } public Message getMessage() { @@ -57,8 +62,21 @@ public boolean isTemporaryDestination() { return temporaryDestination; } + @Nullable + public Instant getStartTime() { + return startTime; + } + public static MessageWithDestination create( Message message, MessageOperation operation, Destination fallbackDestination) { + return create(message, operation, fallbackDestination, null); + } + + public static MessageWithDestination create( + Message message, + MessageOperation operation, + Destination fallbackDestination, + @Nullable Instant startTime) { Destination jmsDestination = null; try { jmsDestination = message.getJMSDestination(); @@ -69,16 +87,16 @@ public static MessageWithDestination create( } if (jmsDestination instanceof Queue) { - return createMessageWithQueue(message, operation, (Queue) jmsDestination); + return createMessageWithQueue(message, operation, (Queue) jmsDestination, startTime); } if (jmsDestination instanceof Topic) { - return createMessageWithTopic(message, operation, (Topic) jmsDestination); + return createMessageWithTopic(message, operation, (Topic) jmsDestination, startTime); } - return new MessageWithDestination(message, operation, "unknown", "unknown", false); + return new MessageWithDestination(message, operation, "unknown", "unknown", false, startTime); } private static MessageWithDestination createMessageWithQueue( - Message message, MessageOperation operation, Queue destination) { + Message message, MessageOperation operation, Queue destination, @Nullable Instant startTime) { String queueName; try { queueName = destination.getQueueName(); @@ -89,11 +107,11 @@ private static MessageWithDestination createMessageWithQueue( boolean temporary = destination instanceof TemporaryQueue || queueName.startsWith(TIBCO_TMP_PREFIX); - return new MessageWithDestination(message, operation, queueName, "queue", temporary); + return new MessageWithDestination(message, operation, queueName, "queue", temporary, startTime); } private static MessageWithDestination createMessageWithTopic( - Message message, MessageOperation operation, Topic destination) { + Message message, MessageOperation operation, Topic destination, @Nullable Instant startTime) { String topicName; try { topicName = destination.getTopicName(); @@ -104,6 +122,6 @@ private static MessageWithDestination createMessageWithTopic( boolean temporary = destination instanceof TemporaryTopic || topicName.startsWith(TIBCO_TMP_PREFIX); - return new MessageWithDestination(message, operation, topicName, "topic", temporary); + return new MessageWithDestination(message, operation, topicName, "topic", temporary, startTime); } } From 51becf0d4c242315d96bd8ab3c25a11a240e91b1 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Wed, 28 Apr 2021 16:50:14 +0200 Subject: [PATCH 4/7] remove useless test --- .../messaging/MessageOperationTest.java | 19 ------------------- 1 file changed, 19 deletions(-) delete mode 100644 instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperationTest.java diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperationTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperationTest.java deleted file mode 100644 index a7980bc095a2..000000000000 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperationTest.java +++ /dev/null @@ -1,19 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.instrumentation.api.instrumenter.messaging; - -import static org.junit.jupiter.api.Assertions.assertEquals; - -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.EnumSource; - -class MessageOperationTest { - @ParameterizedTest - @EnumSource(MessageOperation.class) - void shouldGetCorrectOperationName(MessageOperation operation) { - assertEquals(operation.name().toLowerCase(), operation.name()); - } -} From a960f2e30a2d6a214cda8ade9a2ac91f76ba4cab Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Thu, 29 Apr 2021 15:10:54 +0200 Subject: [PATCH 5/7] fix missing instrumentation version --- .../instrumentation/api/instrumenter/Instrumenter.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java index a7f23d670b28..62b58a3ae9db 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java @@ -14,6 +14,7 @@ import io.opentelemetry.api.trace.SpanKind; import io.opentelemetry.api.trace.Tracer; import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.InstrumentationVersion; import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics; import io.opentelemetry.instrumentation.api.tracer.ClientSpan; import io.opentelemetry.instrumentation.api.tracer.ServerSpan; @@ -62,7 +63,8 @@ public static InstrumenterBuilder newBuil Instrumenter(InstrumenterBuilder builder) { this.instrumentationName = builder.instrumentationName; - this.tracer = builder.openTelemetry.getTracer(builder.instrumentationName); + this.tracer = + builder.openTelemetry.getTracer(instrumentationName, InstrumentationVersion.VERSION); this.spanNameExtractor = builder.spanNameExtractor; this.spanKindExtractor = builder.spanKindExtractor; this.spanStatusExtractor = builder.spanStatusExtractor; From 9b777c8624cb0e9dcf5067a31483513db239c7e9 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Tue, 4 May 2021 10:08:49 +0200 Subject: [PATCH 6/7] Code review comments --- .../api/instrumenter/CurrentNanoTime.java | 79 ------------------- .../api/instrumenter/EndTimeExtractor.java | 20 +++-- .../api/instrumenter/Instrumenter.java | 16 +++- .../api/instrumenter/InstrumenterBuilder.java | 23 ++---- .../api/instrumenter/StartTimeExtractor.java | 20 +++-- .../messaging/MessageOperation.java | 17 +++- .../MessagingAttributesExtractor.java | 4 +- .../messaging/MessagingSpanNameExtractor.java | 2 +- .../api/instrumenter/CurrentNanoTimeTest.java | 34 -------- .../api/instrumenter/InstrumenterTest.java | 3 +- .../MessagingAttributesExtractorTest.java | 8 +- .../MessagingSpanNameExtractorTest.java | 6 +- .../instrumentation/jms/JmsInstrumenters.java | 3 +- .../JmsMessageConsumerInstrumentation.java | 6 +- .../JmsMessageListenerInstrumentation.java | 2 +- .../JmsMessageProducerInstrumentation.java | 4 +- .../jms/JmsStartTimeExtractor.java | 20 ----- 17 files changed, 84 insertions(+), 183 deletions(-) delete mode 100644 instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTime.java delete mode 100644 instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTimeTest.java delete mode 100644 instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsStartTimeExtractor.java diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTime.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTime.java deleted file mode 100644 index b5447548db48..000000000000 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTime.java +++ /dev/null @@ -1,79 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.instrumentation.api.instrumenter; - -import java.time.Clock; -import java.time.Instant; -import java.time.ZoneId; -import java.time.ZoneOffset; -import java.util.concurrent.TimeUnit; -import java.util.function.LongSupplier; - -/** - * This utility class allows to retrieve an {@link Instant} with the current time measured with - * nanosecond precision. - */ -public final class CurrentNanoTime { - private static final Clock CLOCK; - - static { - Clock clock; - try { - // Runtime#version() was added in Java 9 - Runtime.class.getDeclaredMethod("version"); - // Java 9+ SystemClock has nano precision - clock = Clock.systemUTC(); - } catch (NoSuchMethodException e) { - // Java 8 doesn't, so we have to compute the correct timestamp ourselves - clock = new Java8NanoClock(); - } - CLOCK = clock; - } - - /** Returns an {@link Instant} with the current time measured with nanosecond precision. */ - public static Instant get() { - return CLOCK.instant(); - } - - // reused logic from the SDK MonotonicClock - static final class Java8NanoClock extends Clock { - private final long epochNanos; - private final LongSupplier nanoTimeSupplier; - private final long nanoTimeOffset; - - private Java8NanoClock() { - this(TimeUnit.MILLISECONDS.toNanos(System.currentTimeMillis()), System::nanoTime); - } - - // visible for tests - Java8NanoClock(long epochNanos, LongSupplier nanoTimeSupplier) { - this.epochNanos = epochNanos; - this.nanoTimeSupplier = nanoTimeSupplier; - this.nanoTimeOffset = nanoTimeSupplier.getAsLong(); - } - - @Override - public ZoneId getZone() { - return ZoneOffset.UTC; - } - - @Override - public Clock withZone(ZoneId zone) { - throw new UnsupportedOperationException(); - } - - @Override - public Instant instant() { - long deltaNanos = nanoTimeSupplier.getAsLong() - this.nanoTimeOffset; - long nowNanos = epochNanos + deltaNanos; - long seconds = TimeUnit.NANOSECONDS.toSeconds(nowNanos); - long nanoAdjustment = nowNanos - TimeUnit.SECONDS.toNanos(seconds); - return Instant.ofEpochSecond(seconds, nanoAdjustment); - } - } - - private CurrentNanoTime() {} -} diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/EndTimeExtractor.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/EndTimeExtractor.java index 31946a30b23a..aaeac158cde2 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/EndTimeExtractor.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/EndTimeExtractor.java @@ -6,18 +6,28 @@ package io.opentelemetry.instrumentation.api.instrumenter; import java.time.Instant; +import org.checkerframework.checker.nullness.qual.Nullable; -/** Extractor of the end time of response processing. */ +/** + * Extractor of the end time of response processing. An {@link EndTimeExtractor} should always use + * the same timestamp source as the corresponding {@link StartTimeExtractor} - extracted timestamps + * must be comparable. + */ +@FunctionalInterface public interface EndTimeExtractor { /** - * Returns the default {@link EndTimeExtractor}, which always returns the current time with - * nanosecond precision. + * Returns the default {@link EndTimeExtractor}, which delegates to the OpenTelemetry SDK internal + * clock. */ static EndTimeExtractor getDefault() { - return response -> CurrentNanoTime.get(); + return response -> null; } - /** Returns the timestamp marking the end of the response processing. */ + /** + * Returns the timestamp marking the end of the response processing. If the returned timestamp is + * {@code null} the OpenTelemetry SDK will use its internal clock to determine the end time. + */ + @Nullable Instant extract(RESPONSE response); } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java index 62b58a3ae9db..a4ca2c919682 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java @@ -18,6 +18,7 @@ import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics; import io.opentelemetry.instrumentation.api.tracer.ClientSpan; import io.opentelemetry.instrumentation.api.tracer.ServerSpan; +import java.time.Instant; import java.util.ArrayList; import java.util.List; import org.checkerframework.checker.nullness.qual.Nullable; @@ -112,8 +113,12 @@ public Context start(Context parentContext, REQUEST request) { tracer .spanBuilder(spanNameExtractor.extract(request)) .setSpanKind(spanKind) - .setParent(parentContext) - .setStartTimestamp(startTimeExtractor.extract(request)); + .setParent(parentContext); + + Instant startTime = startTimeExtractor.extract(request); + if (startTime != null) { + spanBuilder.setStartTimestamp(startTime); + } AttributesBuilder attributes = Attributes.builder(); for (AttributesExtractor extractor : extractors) { @@ -155,6 +160,11 @@ public void end(Context context, REQUEST request, RESPONSE response, @Nullable T span.setStatus(spanStatusExtractor.extract(request, response, error)); - span.end(endTimeExtractor.extract(response)); + Instant endTime = endTimeExtractor.extract(response); + if (endTime != null) { + span.end(endTime); + } else { + span.end(); + } } } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java index c02a9a63d50a..f5a76509c373 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java @@ -5,6 +5,8 @@ package io.opentelemetry.instrumentation.api.instrumenter; +import static java.util.Objects.requireNonNull; + import io.opentelemetry.api.OpenTelemetry; import io.opentelemetry.api.trace.StatusCode; import io.opentelemetry.context.propagation.TextMapGetter; @@ -83,22 +85,13 @@ public InstrumenterBuilder setErrorCauseExtractor( } /** - * Sets the {@link StartTimeExtractor} to extract the timestamp marking the start of the request - * processing. - */ - public InstrumenterBuilder setStartTimeExtractor( - StartTimeExtractor startTimeExtractor) { - this.startTimeExtractor = startTimeExtractor; - return this; - } - - /** - * Sets the {@link StartTimeExtractor} to extract the timestamp marking the end of the response - * processing. + * Sets the {@link StartTimeExtractor} and the {@link EndTimeExtractor} to extract the timestamp + * marking the start and end of processing. */ - public InstrumenterBuilder setEndTimeExtractor( - EndTimeExtractor endTimeExtractor) { - this.endTimeExtractor = endTimeExtractor; + public InstrumenterBuilder setTimeExtractors( + StartTimeExtractor startTimeExtractor, EndTimeExtractor endTimeExtractor) { + this.startTimeExtractor = requireNonNull(startTimeExtractor); + this.endTimeExtractor = requireNonNull(endTimeExtractor); return this; } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/StartTimeExtractor.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/StartTimeExtractor.java index 8f36abbe43eb..ccc6a3549f11 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/StartTimeExtractor.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/StartTimeExtractor.java @@ -6,18 +6,28 @@ package io.opentelemetry.instrumentation.api.instrumenter; import java.time.Instant; +import org.checkerframework.checker.nullness.qual.Nullable; -/** Extractor of the start time of request processing. */ +/** + * Extractor of the start time of request processing. A {@link StartTimeExtractor} should always use + * the same timestamp source as the corresponding {@link EndTimeExtractor} - extracted timestamps + * must be comparable. + */ +@FunctionalInterface public interface StartTimeExtractor { /** - * Returns the default {@link StartTimeExtractor}, which always returns the current time with - * nanosecond precision. + * Returns the default {@link StartTimeExtractor}, which delegates to the OpenTelemetry SDK + * internal clock. */ static StartTimeExtractor getDefault() { - return request -> CurrentNanoTime.get(); + return request -> null; } - /** Returns the timestamp marking the start of the request processing. */ + /** + * Returns the timestamp marking the start of the request processing. If the returned timestamp is + * {@code null} the OpenTelemetry SDK will use its internal clock to determine the start time. + */ + @Nullable Instant extract(REQUEST request); } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java index 474206cfcb54..9c1ab7b1aba8 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java @@ -5,13 +5,24 @@ package io.opentelemetry.instrumentation.api.instrumenter.messaging; +import java.util.Locale; + /** * Represents type of operations * that may be used in a messaging system. */ public enum MessageOperation { - send, - receive, - process + SEND, + RECEIVE, + PROCESS; + + /** + * Returns the operation name as defined in the + * specification. + */ + public String operationName() { + return name().toLowerCase(Locale.ROOT); + } } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java index 83f16ee34456..e027be5453b7 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractor.java @@ -47,8 +47,8 @@ protected final void onStart(AttributesBuilder attributes, REQUEST request) { SemanticAttributes.MESSAGING_MESSAGE_PAYLOAD_COMPRESSED_SIZE_BYTES, messagePayloadCompressedSize(request)); MessageOperation operation = operation(request); - if (operation == MessageOperation.receive || operation == MessageOperation.process) { - set(attributes, SemanticAttributes.MESSAGING_OPERATION, operation.name()); + if (operation == MessageOperation.RECEIVE || operation == MessageOperation.PROCESS) { + set(attributes, SemanticAttributes.MESSAGING_OPERATION, operation.operationName()); } } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractor.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractor.java index 729560cdb95a..1807354374c6 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractor.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractor.java @@ -40,6 +40,6 @@ public String extract(REQUEST request) { } MessageOperation operation = attributesExtractor.operation(request); - return operation == null ? destinationName : destinationName + " " + operation.name(); + return operation == null ? destinationName : destinationName + " " + operation.operationName(); } } diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTimeTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTimeTest.java deleted file mode 100644 index 1ad5c0bf6d59..000000000000 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTimeTest.java +++ /dev/null @@ -1,34 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.instrumentation.api.instrumenter; - -import static org.junit.jupiter.api.Assertions.assertEquals; - -import java.time.Clock; -import java.time.Instant; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicLong; -import org.junit.jupiter.api.Test; - -class CurrentNanoTimeTest { - @Test - void shouldCreateAnInstantWithNanosecondPrecision() { - // given - long epochNanoseconds = TimeUnit.SECONDS.toNanos(10); - AtomicLong nanoseconds = new AtomicLong(1_000); - - Clock java8Clock = new CurrentNanoTime.Java8NanoClock(epochNanoseconds, nanoseconds::get); - - nanoseconds.set(1_000_002_000); - - // when - Instant result = java8Clock.instant(); - - // then - assertEquals(11, result.getEpochSecond()); - assertEquals(1_000, result.getNano()); - } -} diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java index 84b51e0b50bd..a092b1a5b8cb 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java @@ -323,8 +323,7 @@ void shouldStartSpanWithGivenStartTime() { Instrumenter instrumenter = Instrumenter.newBuilder( otelTesting.getOpenTelemetry(), "test", request -> "test span") - .setStartTimeExtractor(request -> request) - .setEndTimeExtractor(response -> response) + .setTimeExtractors(request -> request, response -> response) .newInstrumenter(); Instant startTime = Instant.ofEpochSecond(100); diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractorTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractorTest.java index 8ed3c4b5aa1c..938550224205 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractorTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractorTest.java @@ -137,7 +137,7 @@ void shouldExtractAllAvailableAttributes( expectedEntries.add(entry(SemanticAttributes.MESSAGING_MESSAGE_PAYLOAD_SIZE_BYTES, 100L)); expectedEntries.add( entry(SemanticAttributes.MESSAGING_MESSAGE_PAYLOAD_COMPRESSED_SIZE_BYTES, 10L)); - expectedEntries.add(entry(SemanticAttributes.MESSAGING_OPERATION, operation.name())); + expectedEntries.add(entry(SemanticAttributes.MESSAGING_OPERATION, operation.operationName())); assertThat(startAttributes.build()).containsOnly(expectedEntries.toArray(new MapEntry[0])); @@ -147,15 +147,15 @@ void shouldExtractAllAvailableAttributes( static Stream destinations() { return Stream.of( - Arguments.of(false, "destination", MessageOperation.receive, "destination"), - Arguments.of(true, null, MessageOperation.process, "(temporary)")); + Arguments.of(false, "destination", MessageOperation.RECEIVE, "destination"), + Arguments.of(true, null, MessageOperation.PROCESS, "(temporary)")); } @Test void shouldNotSetSendOperation() { // when AttributesBuilder attributes = Attributes.builder(); - underTest.onStart(attributes, singletonMap("operation", MessageOperation.send.name())); + underTest.onStart(attributes, singletonMap("operation", MessageOperation.SEND.name())); // then assertThat(attributes.build().isEmpty()).isTrue(); diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractorTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractorTest.java index 0b2e4f02232a..d28f8a8f2380 100644 --- a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractorTest.java +++ b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractorTest.java @@ -49,9 +49,9 @@ void shouldExtractSpanName( static Stream spanNameParams() { return Stream.of( - Arguments.of(false, "destination", MessageOperation.send, "destination send"), - Arguments.of(true, null, MessageOperation.process, "(temporary) process"), - Arguments.of(false, null, MessageOperation.receive, "unknown receive"), + Arguments.of(false, "destination", MessageOperation.SEND, "destination send"), + Arguments.of(true, null, MessageOperation.PROCESS, "(temporary) process"), + Arguments.of(false, null, MessageOperation.RECEIVE, "unknown receive"), Arguments.of(false, "destination", null, "destination")); } diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsInstrumenters.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsInstrumenters.java index bac5cc5a9cc5..a001bfa8c8b9 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsInstrumenters.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsInstrumenters.java @@ -11,6 +11,7 @@ import io.opentelemetry.instrumentation.api.instrumenter.SpanKindExtractor; import io.opentelemetry.instrumentation.api.instrumenter.SpanNameExtractor; import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessagingSpanNameExtractor; +import java.time.Instant; public final class JmsInstrumenters { private static final String INSTRUMENTATION_NAME = "io.opentelemetry.javaagent.jms-1.1"; @@ -35,7 +36,7 @@ public final class JmsInstrumenters { Instrumenter.newBuilder( otel, INSTRUMENTATION_NAME, spanNameExtractor) .addAttributesExtractor(attributesExtractor) - .setStartTimeExtractor(new JmsStartTimeExtractor()) + .setTimeExtractors(MessageWithDestination::getStartTime, response -> Instant.now()) .newInstrumenter(SpanKindExtractor.alwaysConsumer()); LISTENER_INSTRUMENTER = Instrumenter.newBuilder( diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java index 301b69cd9228..fc0b0ec99832 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java @@ -13,10 +13,10 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArguments; import io.opentelemetry.context.Context; -import io.opentelemetry.instrumentation.api.instrumenter.CurrentNanoTime; import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessageOperation; import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; +import java.time.Clock; import java.time.Instant; import java.util.HashMap; import java.util.Map; @@ -54,7 +54,7 @@ public static class ConsumerAdvice { @Advice.OnMethodEnter public static Instant onEnter() { - return CurrentNanoTime.get(); + return Clock.systemUTC().instant(); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) @@ -69,7 +69,7 @@ public static void stopSpan( Context parentContext = Java8BytecodeBridge.currentContext(); MessageWithDestination request = - MessageWithDestination.create(message, MessageOperation.receive, null, startTime); + MessageWithDestination.create(message, MessageOperation.RECEIVE, null, startTime); if (consumerInstrumenter().shouldStart(parentContext, request)) { Context context = consumerInstrumenter().start(parentContext, request); diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageListenerInstrumentation.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageListenerInstrumentation.java index 1d971389eb52..ffd8773b652a 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageListenerInstrumentation.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageListenerInstrumentation.java @@ -54,7 +54,7 @@ public static void onEnter( @Advice.Local("otelScope") Scope scope) { Context parentContext = Java8BytecodeBridge.currentContext(); - request = MessageWithDestination.create(message, MessageOperation.process, null); + request = MessageWithDestination.create(message, MessageOperation.PROCESS, null); if (!listenerInstrumenter().shouldStart(parentContext, request)) { return; diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageProducerInstrumentation.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageProducerInstrumentation.java index 0aba25e19f73..6d1e7cc53696 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageProducerInstrumentation.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageProducerInstrumentation.java @@ -78,7 +78,7 @@ public static void onEnter( } Context parentContext = Java8BytecodeBridge.currentContext(); - request = MessageWithDestination.create(message, MessageOperation.send, defaultDestination); + request = MessageWithDestination.create(message, MessageOperation.SEND, defaultDestination); if (!producerInstrumenter().shouldStart(parentContext, request)) { return; } @@ -118,7 +118,7 @@ public static void onEnter( } Context parentContext = Java8BytecodeBridge.currentContext(); - request = MessageWithDestination.create(message, MessageOperation.send, destination); + request = MessageWithDestination.create(message, MessageOperation.SEND, destination); if (!producerInstrumenter().shouldStart(parentContext, request)) { return; } diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsStartTimeExtractor.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsStartTimeExtractor.java deleted file mode 100644 index ea6a4c4eae7e..000000000000 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsStartTimeExtractor.java +++ /dev/null @@ -1,20 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.javaagent.instrumentation.jms; - -import io.opentelemetry.instrumentation.api.instrumenter.StartTimeExtractor; -import java.time.Instant; - -public final class JmsStartTimeExtractor implements StartTimeExtractor { - private final StartTimeExtractor fallback = - StartTimeExtractor.getDefault(); - - @Override - public Instant extract(MessageWithDestination message) { - Instant startTime = message.getStartTime(); - return startTime != null ? startTime : fallback.extract(message); - } -} From 0e0cc4c36e1d3ecedbc031a2265cd0ed2f002e98 Mon Sep 17 00:00:00 2001 From: Mateusz Rzeszutek Date: Tue, 4 May 2021 11:27:55 +0200 Subject: [PATCH 7/7] Do not allow conditional delegation to SDK --- .../api/instrumenter/EndTimeExtractor.java | 15 +-------------- .../api/instrumenter/Instrumenter.java | 11 ++++------- .../api/instrumenter/InstrumenterBuilder.java | 7 ++++--- .../api/instrumenter/StartTimeExtractor.java | 15 +-------------- .../jms/MessageWithDestinationTest.java | 16 ++++++++-------- .../jms/JmsMessageConsumerInstrumentation.java | 3 +-- 6 files changed, 19 insertions(+), 48 deletions(-) diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/EndTimeExtractor.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/EndTimeExtractor.java index aaeac158cde2..db1fabe1bf24 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/EndTimeExtractor.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/EndTimeExtractor.java @@ -6,7 +6,6 @@ package io.opentelemetry.instrumentation.api.instrumenter; import java.time.Instant; -import org.checkerframework.checker.nullness.qual.Nullable; /** * Extractor of the end time of response processing. An {@link EndTimeExtractor} should always use @@ -16,18 +15,6 @@ @FunctionalInterface public interface EndTimeExtractor { - /** - * Returns the default {@link EndTimeExtractor}, which delegates to the OpenTelemetry SDK internal - * clock. - */ - static EndTimeExtractor getDefault() { - return response -> null; - } - - /** - * Returns the timestamp marking the end of the response processing. If the returned timestamp is - * {@code null} the OpenTelemetry SDK will use its internal clock to determine the end time. - */ - @Nullable + /** Returns the timestamp marking the end of the response processing. */ Instant extract(RESPONSE response); } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java index a4ca2c919682..2efe077557f0 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java @@ -18,7 +18,6 @@ import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics; import io.opentelemetry.instrumentation.api.tracer.ClientSpan; import io.opentelemetry.instrumentation.api.tracer.ServerSpan; -import java.time.Instant; import java.util.ArrayList; import java.util.List; import org.checkerframework.checker.nullness.qual.Nullable; @@ -115,9 +114,8 @@ public Context start(Context parentContext, REQUEST request) { .setSpanKind(spanKind) .setParent(parentContext); - Instant startTime = startTimeExtractor.extract(request); - if (startTime != null) { - spanBuilder.setStartTimestamp(startTime); + if (startTimeExtractor != null) { + spanBuilder.setStartTimestamp(startTimeExtractor.extract(request)); } AttributesBuilder attributes = Attributes.builder(); @@ -160,9 +158,8 @@ public void end(Context context, REQUEST request, RESPONSE response, @Nullable T span.setStatus(spanStatusExtractor.extract(request, response, error)); - Instant endTime = endTimeExtractor.extract(response); - if (endTime != null) { - span.end(endTime); + if (endTimeExtractor != null) { + span.end(endTimeExtractor.extract(response)); } else { span.end(); } diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java index f5a76509c373..30436a424eca 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java @@ -32,8 +32,8 @@ public final class InstrumenterBuilder { SpanStatusExtractor spanStatusExtractor = SpanStatusExtractor.getDefault(); ErrorCauseExtractor errorCauseExtractor = ErrorCauseExtractor.jdk(); - StartTimeExtractor startTimeExtractor = StartTimeExtractor.getDefault(); - EndTimeExtractor endTimeExtractor = EndTimeExtractor.getDefault(); + StartTimeExtractor startTimeExtractor = null; + EndTimeExtractor endTimeExtractor = null; InstrumenterBuilder( OpenTelemetry openTelemetry, @@ -86,7 +86,8 @@ public InstrumenterBuilder setErrorCauseExtractor( /** * Sets the {@link StartTimeExtractor} and the {@link EndTimeExtractor} to extract the timestamp - * marking the start and end of processing. + * 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 setTimeExtractors( StartTimeExtractor startTimeExtractor, EndTimeExtractor endTimeExtractor) { diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/StartTimeExtractor.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/StartTimeExtractor.java index ccc6a3549f11..c013a3300a73 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/StartTimeExtractor.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/StartTimeExtractor.java @@ -6,7 +6,6 @@ package io.opentelemetry.instrumentation.api.instrumenter; import java.time.Instant; -import org.checkerframework.checker.nullness.qual.Nullable; /** * Extractor of the start time of request processing. A {@link StartTimeExtractor} should always use @@ -16,18 +15,6 @@ @FunctionalInterface public interface StartTimeExtractor { - /** - * Returns the default {@link StartTimeExtractor}, which delegates to the OpenTelemetry SDK - * internal clock. - */ - static StartTimeExtractor getDefault() { - return request -> null; - } - - /** - * Returns the timestamp marking the start of the request processing. If the returned timestamp is - * {@code null} the OpenTelemetry SDK will use its internal clock to determine the start time. - */ - @Nullable + /** Returns the timestamp marking the start of the request processing. */ Instant extract(REQUEST request); } diff --git a/instrumentation/jms-1.1/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestinationTest.java b/instrumentation/jms-1.1/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestinationTest.java index 328e3d888541..1d7a5c68f50d 100644 --- a/instrumentation/jms-1.1/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestinationTest.java +++ b/instrumentation/jms-1.1/javaagent-unit-tests/src/test/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestinationTest.java @@ -46,10 +46,10 @@ void shouldCreateMessageWithUnknownDestination() throws JMSException { // when MessageWithDestination result = - MessageWithDestination.create(message, MessageOperation.send, null, START_TIME); + MessageWithDestination.create(message, MessageOperation.SEND, null, START_TIME); // then - assertMessage(MessageOperation.send, "unknown", "unknown", false, START_TIME, result); + assertMessage(MessageOperation.SEND, "unknown", "unknown", false, START_TIME, result); } @Test @@ -59,10 +59,10 @@ void shouldUseFallbackDestinationToCreateMessage() throws JMSException { // when MessageWithDestination result = - MessageWithDestination.create(message, MessageOperation.send, destination, START_TIME); + MessageWithDestination.create(message, MessageOperation.SEND, destination, START_TIME); // then - assertMessage(MessageOperation.send, "unknown", "unknown", false, START_TIME, result); + assertMessage(MessageOperation.SEND, "unknown", "unknown", false, START_TIME, result); } @ParameterizedTest @@ -85,11 +85,11 @@ void shouldCreateMessageWithQueue( // when MessageWithDestination result = - MessageWithDestination.create(message, MessageOperation.receive, null); + MessageWithDestination.create(message, MessageOperation.RECEIVE, null); // then assertMessage( - MessageOperation.receive, + MessageOperation.RECEIVE, "queue", expectedDestinationName, expectedTemporary, @@ -117,11 +117,11 @@ void shouldCreateMessageWithTopic( // when MessageWithDestination result = - MessageWithDestination.create(message, MessageOperation.receive, null); + MessageWithDestination.create(message, MessageOperation.RECEIVE, null); // then assertMessage( - MessageOperation.receive, + MessageOperation.RECEIVE, "topic", expectedDestinationName, expectedTemporary, diff --git a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java index fc0b0ec99832..139755320695 100644 --- a/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java +++ b/instrumentation/jms-1.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java @@ -16,7 +16,6 @@ import io.opentelemetry.instrumentation.api.instrumenter.messaging.MessageOperation; import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; -import java.time.Clock; import java.time.Instant; import java.util.HashMap; import java.util.Map; @@ -54,7 +53,7 @@ public static class ConsumerAdvice { @Advice.OnMethodEnter public static Instant onEnter() { - return Clock.systemUTC().instant(); + return Instant.now(); } @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)