Use Instrumenter in JMS instrumentation #2803

mateuszrzeszutek · 2021-04-14T10:04:02Z

And introduce messaging semantic conventions

Part of #2713

iNikem · 2021-04-15T08:41:28Z

.../main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java

+  SEND,
+  RECEIVE,
+  PROCESS;


I will ask my favourite question: why enum cannot be already in lower case?

Are these missing from the generated conventions? If so let's fix it.

Until then we could also go with static final String which is what our generator produces since yesterday

The semconv generated enum is missing the send value:

public enum MessagingOperationValues { /** receive. */ RECEIVE("receive"), /** process. */ PROCESS("process"), ; }

I suppose that's because setting message.operation to send is strictly forbidden in the spec. Still, having send available makes the instrumentation code a bit clearer...

why enum cannot be already in lower case?

Good question - I don't really mind it, but most people tend to dislike anything other than CONSTANT_CASE.

iNikem · 2021-04-15T08:45:06Z

...entelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractorTest.java

+
+  @ParameterizedTest
+  @MethodSource("destinations")
+  void shouldExtractAllAvailableAttributes(


I am a little bit sceptical about the value of this test. It may be useful to test extracting operation name and behaviour in case of temporary destination. But otherwise, maybe it can be replaced with tests of actual implementations of MessagingAttributesExtractor?

I think that the real value is testing the full semantic conventions themselves, not just one implementation of them.

Maybe if we replaced the inheritance with composition too those tests would make more sense: something like MessagingAttributesExtractor.create(new JmsAttributesGetter())

I refactored the attributes extractor and made it not abstract: ecf8ac3
Please tell me WDYT about it - the separation of concerns is nice, but on the other hand it makes setting up an Instrumenter a bit more bloated.

Hmm - I guess I lean towards the abstract version if it keeps using the Instrumenter simpler. Not sure there was a huge problem with the test

I'll revert it for now, we can come back to this discussion in a separate issue/PR. Personally I don't really have a strong opinion about either solution, both have some good and bad sides.

iNikem · 2021-04-15T08:47:48Z

.../io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingSpanNameExtractor.java

+   *     name>}.
+   * @see MessagingAttributesExtractor#operation(Object) used to extract {@code <operation name>}.
+   */
+  public static <REQUEST> SpanNameExtractor<REQUEST> create(


Why does this class have a factory method, while MessagingAttributesExtractor does not? What is so different about this class that requires special construction?

MessagingAttributesExtractor is an abstract class that has to be extended, it cannot have a static factory method.

What is so different about this class that requires special construction?

Hmm, there's nothing special - I've added it here because we use builders & factory methods (instead of plain constructor calls) in the instrumenter API. I guess that's more resistant to future changes? But I may be overthinking this, and maybe it just looks a bit cooler 😄

...ent/src/main/java/io/opentelemetry/javaagent/instrumentation/jms/MessageWithDestination.java

anuraaga

Thanks a lot @mateuszrzeszutek mostly looks good and just some questions

...n/java/io/opentelemetry/javaagent/instrumentation/jms/JmsMessageConsumerInstrumentation.java

...a/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesGetter.java

anuraaga · 2021-04-27T06:14:10Z

...entelemetry/instrumentation/api/instrumenter/messaging/MessagingAttributesExtractorTest.java

+
+  @ParameterizedTest
+  @MethodSource("destinations")
+  void shouldExtractAllAvailableAttributes(


Hmm - I guess I lean towards the abstract version if it keeps using the Instrumenter simpler. Not sure there was a huge problem with the test

mateuszrzeszutek · 2021-04-28T14:47:16Z

Added StartTimeExtractor and EndTimeExtractor, those two should also be useful for metrics. And I've accidentally resolved #2600 for Instrumenter-using instrumentations too 😂

mateuszrzeszutek · 2021-04-28T14:49:23Z

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

+  Instrumenter(InstrumenterBuilder<REQUEST, RESPONSE> 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;
  }


I refactored the Instrumenter constructor and used the same approach as OkHttp builder. I think it makes passing all those extractors a bit easier on the eyes

And introduce messaging semantic conventions

trask

And I've accidentally resolved #2600 for Instrumenter-using instrumentations too 😂

🎉

.../main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java

...ion-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTime.java

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

mateuszrzeszutek · 2021-05-04T08:14:43Z

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

+  public InstrumenterBuilder<REQUEST, RESPONSE> setTimeExtractors(
+      StartTimeExtractor<REQUEST> startTimeExtractor, EndTimeExtractor<RESPONSE> endTimeExtractor) {


These two should probably be always set together - start & end timestamps need to be comparable, so it does not make any sense to set just one and compare with the SDK timestamp.

anuraaga · 2021-05-04T08:21:07Z

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

+   * 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


Do you think there's a use case for a custom extractor to delegate to SDK conditionally? Otherwise maybe we don't need to make this default and can treat that as an implementation detail (we could even change the null check to == getDefault(). This is ok too though.

Do you think there's a use case for a custom extractor to delegate to SDK conditionally?

No, it doesn't make any sense for a custom extractor to delegate to SDK. I'll change that.
Thanks!

I've removed the getDefault() methods from start/end extractors, I thought it would be confusing to have it there: now the default behavior is using SDK for timestamps, and if you pass custom extractors they must be custom implementations.

mateuszrzeszutek requested review from anuraaga, iNikem, jkwatson, pavolloffay, trask and tylerbenson as code owners April 14, 2021 10:04

iNikem reviewed Apr 15, 2021

View reviewed changes

mateuszrzeszutek force-pushed the instrumenter-messaging branch from 73f848a to ecf8ac3 Compare April 16, 2021 15:53

anuraaga reviewed Apr 27, 2021

View reviewed changes

mateuszrzeszutek force-pushed the instrumenter-messaging branch from 18cf049 to 136b656 Compare April 28, 2021 14:43

mateuszrzeszutek requested a review from laurit as a code owner April 28, 2021 14:43

mateuszrzeszutek commented Apr 28, 2021

View reviewed changes

Mateusz Rzeszutek added 4 commits April 29, 2021 08:42

Use Instrumenter in JMS instrumentation

0d3bc70

And introduce messaging semantic conventions

Code review comments

0030766

Use Instrumenter in JMS instrumentation - code review comments

56d8a62

remove useless test

51becf0

mateuszrzeszutek force-pushed the instrumenter-messaging branch from cd54adf to 51becf0 Compare April 29, 2021 06:42

fix missing instrumentation version

a960f2e

trask approved these changes Apr 29, 2021

View reviewed changes

.../main/java/io/opentelemetry/instrumentation/api/instrumenter/messaging/MessageOperation.java Outdated Show resolved Hide resolved

anuraaga reviewed Apr 30, 2021

View reviewed changes

...ion-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/CurrentNanoTime.java Outdated Show resolved Hide resolved

anuraaga reviewed Apr 30, 2021

View reviewed changes

...tation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java Outdated Show resolved Hide resolved

Code review comments

9b777c8

mateuszrzeszutek commented May 4, 2021

View reviewed changes

anuraaga approved these changes May 4, 2021

View reviewed changes

Do not allow conditional delegation to SDK

0e0cc4c

iNikem merged commit e003417 into open-telemetry:main May 5, 2021

iNikem mentioned this pull request May 5, 2021

Agent class loader multi release jar #2912

Merged

mateuszrzeszutek deleted the instrumenter-messaging branch May 6, 2021 08:14

trask mentioned this pull request May 21, 2021

Convert all instrumentation to the new Instrumenter API #2713

Closed

99 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Instrumenter in JMS instrumentation #2803

Use Instrumenter in JMS instrumentation #2803

mateuszrzeszutek commented Apr 14, 2021

iNikem Apr 15, 2021

anuraaga Apr 15, 2021

mateuszrzeszutek Apr 15, 2021

iNikem Apr 15, 2021

mateuszrzeszutek Apr 15, 2021

mateuszrzeszutek Apr 16, 2021 •

edited

Loading

anuraaga Apr 27, 2021

mateuszrzeszutek Apr 27, 2021

iNikem Apr 15, 2021

mateuszrzeszutek Apr 15, 2021

anuraaga left a comment

anuraaga Apr 27, 2021

mateuszrzeszutek commented Apr 28, 2021

mateuszrzeszutek Apr 28, 2021

trask left a comment

mateuszrzeszutek May 4, 2021

anuraaga May 4, 2021

mateuszrzeszutek May 4, 2021

mateuszrzeszutek May 4, 2021 •

edited

Loading

		public InstrumenterBuilder<REQUEST, RESPONSE> setTimeExtractors(
		StartTimeExtractor<REQUEST> startTimeExtractor, EndTimeExtractor<RESPONSE> endTimeExtractor) {

Use Instrumenter in JMS instrumentation #2803

Use Instrumenter in JMS instrumentation #2803

Conversation

mateuszrzeszutek commented Apr 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateuszrzeszutek Apr 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateuszrzeszutek commented Apr 28, 2021

Choose a reason for hiding this comment

trask left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateuszrzeszutek May 4, 2021 • edited Loading

Choose a reason for hiding this comment

mateuszrzeszutek Apr 16, 2021 •

edited

Loading

mateuszrzeszutek May 4, 2021 •

edited

Loading