[4.x] - MicroProfile Telemetry Support #6493

dalexandrov · 2023-03-27T09:03:37Z

Resolves #5817

Currently depends on opentelemetry-sdk-extension-autoconfigure 1.22.0-alpha.
This should be fixed with OTEL MP Telemetry committee.

microprofile/telemetry/src/main/java/io/helidon/microprofile/telemetry/AgentDetection.java

.../telemetry/src/main/java/io/helidon/microprofile/telemetry/HelidonTelemetryClientFilter.java

...profile/telemetry/src/main/java/io/helidon/microprofile/telemetry/OpenTelemetryProducer.java

microprofile/telemetry/src/main/java/io/helidon/microprofile/telemetry/WithSpanInterceptor.java

Verdent · 2023-03-27T15:51:13Z

microprofile/telemetry/src/main/java/io/helidon/microprofile/telemetry/WithSpanInterceptor.java

+        WithSpan annotation = context.getMethod().getAnnotation(WithSpan.class);
+
+        Context parentContext = Context.current();
+        Scope scope = null;


There is no point in making this variable here and setting it to null, since it is not used till line 80 and set to value directly.

I am missing the change.

microprofile/telemetry/src/main/java/io/helidon/microprofile/telemetry/AgentDetection.java

...profile/telemetry/src/main/java/io/helidon/microprofile/telemetry/OpenTelemetryProducer.java

...profile/telemetry/src/main/java/io/helidon/microprofile/telemetry/TelemetryCdiExtension.java

microprofile/telemetry/src/main/java/io/helidon/microprofile/telemetry/WithSpanInterceptor.java

...profile/telemetry/src/test/java/io/helidon/microprofile/telemetry/TelemetryDisabledTest.java

microprofile/telemetry/src/test/java/io/helidon/microprofile/telemetry/TelemetryTest.java

...tests/arquillian/src/main/java/io/helidon/microprofile/arquillian/HelidonMethodExecutor.java

dalexandrov · 2023-04-26T07:52:47Z

Had to fallback to:

HashMap<String, String> telemetryProperties = new HashMap<>();
for (String propertyName : mpConfig.getPropertyNames()) {
          if (propertyName.startsWith("otel.")) {
              mpConfig.getOptionalValue(propertyName, String.class).ifPresent(
                      value -> telemetryProperties.put(propertyName, value));
     }
}

Otherwise fails with:

Caused by: java.util.NoSuchElementException: Property "env.GITHUB_BASE_REF" is not available in configuration
	at io.helidon.config.mp.MpConfigImpl.lambda$getValue$5(MpConfigImpl.java:122)
	at java.base/java.util.Optional.orElseThrow(Optional.java:403)
	at io.helidon.config.mp.MpConfigImpl.getValue(MpConfigImpl.java:122)
	at io.helidon.config.mp.SeConfig.asMap(SeConfig.java:257)
	at io.helidon.config.mp.MpConfigProviderResolver$ConfigDelegate.asMap(MpConfigProviderResolver.java:341)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
	at java.base/java.lang.reflect.Method.invoke(Method.java:578)
	at org.jboss.weld.bean.proxy.AbstractBeanInstance.invoke(AbstractBeanInstance.java:38)
	at org.jboss.weld.bean.proxy.ProxyMethodHandler.invoke(ProxyMethodHandler.java:106)
	at io.helidon.config.Config$Config$_$$_WeldClientProxy.asMap(Unknown Source)

if using:

HashMap<String, String> telemetryProperties = new HashMap<>(config
                .asMap()
                .orElseGet(Map::of)
                .entrySet()
                .stream()
                .filter(k -> k.getKey().contains("otel"))
                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue))
);

Verdent · 2023-04-27T09:36:59Z

...profile/telemetry/src/main/java/io/helidon/microprofile/telemetry/OpenTelemetryProducer.java

+            if (LOGGER.isLoggable(System.Logger.Level.TRACE)) {
+                LOGGER.log(System.Logger.Level.TRACE, "Telemetry Disabled");
+            }
+            openTelemetry = LazyValue.create(() -> OpenTelemetry.noop());


Verdent · 2023-04-27T09:41:50Z

...profile/telemetry/src/main/java/io/helidon/microprofile/telemetry/OpenTelemetryProducer.java

+    @PostConstruct
+    private void init() {
+
+        telemetryProperties  = getTelemetryProperties();


Is this intentional to use mutable Map here?

Wrapped into Collections.unmodifiableMap

Verdent · 2023-04-27T09:50:54Z

...lemetry/src/main/java/io/helidon/microprofile/telemetry/HelidonTelemetryContainerFilter.java

+    private static final String REST_RESOURCE_CLASS = "rest.resource.class";
+    private static final String REST_RESOURCE_METHOD = "rest.resource.method";


I am not sure why did you mark this as resolved without any response. Are these values stored in ContainerRequestContext actually used anywhere?

Verdent · 2023-04-27T10:47:59Z

...profile/telemetry/src/main/java/io/helidon/microprofile/telemetry/OpenTelemetryProducer.java

+    /**
+     * Provides an instance of the current OpenTelemetry Tracer.
+     *
+     * @param openTelemetry


missing description

Redundant items deleted

Verdent · 2023-04-27T10:52:08Z

...ile/telemetry/src/main/java/io/helidon/microprofile/telemetry/TelemetryAutoDiscoverable.java

+     * Used to register {@code HelidonTelemetryContainerFilter} and {@code HelidonTelemetryClientFilter}
+     * filers.
+     *
+     * @param ctx


missing description

tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/HelidonOpenTelemetry.java

tracing/opentelemetry/src/test/java/io/helidon/tracing/opentelemetry/AgentDetectorTest.java

tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/HelidonOpenTelemetry.java

...lemetry/src/main/java/io/helidon/microprofile/telemetry/HelidonTelemetryContainerFilter.java

Verdent · 2023-05-04T08:42:06Z

...profile/telemetry/src/main/java/io/helidon/microprofile/telemetry/OpenTelemetryProducer.java

+    private Config config;
+
+    private org.eclipse.microprofile.config.Config mpConfig;


these two could be final.

Verdent · 2023-05-04T08:44:52Z

...lemetry/src/main/java/io/helidon/microprofile/telemetry/HelidonTelemetryContainerFilter.java

+    static {
+        CONTEXT_HEADER_INJECTOR =
+                new TextMapGetter<>() {
+                    @Override
+                    public String get(ContainerRequestContext containerRequestContext, String s) {
+                        Objects.requireNonNull(containerRequestContext);
+                        return containerRequestContext.getHeaderString(s);
+                    }
+
+                    @Override
+                    public Iterable<String> keys(ContainerRequestContext containerRequestContext) {
+                        return List.copyOf(containerRequestContext.getHeaders().keySet());
+                    }
+                };
+    }


Would not this static block be better placed under the constant line it initializes?

Verdent · 2023-05-04T08:48:55Z

.../telemetry/src/main/java/io/helidon/microprofile/telemetry/HelidonTelemetryClientFilter.java

+    private static final String HTTP_STATUS_CODE = "http.status_code";
+    private static final String HTTP_METHOD = "http.method";
+    private static final String HTTP_SCHEME = "http.scheme";


These constants are also created in HelidonTelemetryContainerFilter.

Verdent · 2023-05-04T08:54:56Z

.../telemetry/src/main/java/io/helidon/microprofile/telemetry/HelidonTelemetryClientFilter.java

+            LOGGER.log(System.Logger.Level.TRACE, "Starting Span in a Client Request");
+        }
+
+        Context parentContext = Context.current();


Since this is not used on other places, the only usage could be replaced with Context.current() directly. This would make sense only it it was used in other places. But since on all places in this method, where Context is used, Context.current() is used instead, this is redundant.

Verdent · 2023-05-04T09:08:14Z

microprofile/telemetry/src/main/java/io/helidon/microprofile/telemetry/WithSpanInterceptor.java

+            return context.proceed();
+        } catch (Exception e) {
+            span.recordException(e);
+            return context.proceed();


This line should be throw e

Verdent · 2023-05-04T11:07:11Z

tracing/opentelemetry/src/test/java/io/helidon/tracing/opentelemetry/AgentDetectorTest.java

+
+    @Test
+    @AddConfig(key = HelidonOpenTelemetry.OTEL_AGENT_PRESENT_PROPERTY, value = "true")
+    void shouldBeNoOpTelemetry(){


between ) and { should be space

Verdent · 2023-05-04T11:07:15Z

tracing/opentelemetry/src/test/java/io/helidon/tracing/opentelemetry/AgentDetectorTest.java

+
+    @Test
+    @AddConfig(key = HelidonOpenTelemetry.OTEL_AGENT_PRESENT_PROPERTY, value = "false")
+    void shouldNotBeNoOpTelemetry(){


between ) and { should be space

Verdent · 2023-05-04T11:07:19Z

tracing/opentelemetry/src/test/java/io/helidon/tracing/opentelemetry/AgentDetectorTest.java

+    }
+
+    @Test
+    void checkEnvVariable(){


between ) and { should be space

Verdent · 2023-05-04T11:16:31Z

tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/HelidonOpenTelemetry.java

+     * If the value is not explicitly set, the detector does best effort to estimate indirect means if the agent is present.
+     * This detector may stop working if OTEL changes the indirect indicators.
+     */
+    public static class AgentDetector {


this is basically just a util class. It has to have private constructor and it should probably be final as well.

Verdent · 2023-05-04T11:19:19Z

tracing/opentelemetry/src/main/java/io/helidon/tracing/opentelemetry/HelidonOpenTelemetry.java

+        }
+
+        private static boolean checkContext() {
+            return io.opentelemetry.context.Context.current().getClass().getName().contains("agent");


Context should be imported and used without fully qualified package.

Verdent

Two more additional findings.

...profile/telemetry/src/main/java/io/helidon/microprofile/telemetry/OpenTelemetryProducer.java

...pentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetryTracerProvider.java

Approved by David

dalexandrov added MP 4.x Version 4.x TCK labels Mar 27, 2023

dalexandrov requested review from danielkec, barchetta and tomas-langer March 27, 2023 09:03

dalexandrov self-assigned this Mar 27, 2023

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Mar 27, 2023

Verdent requested changes Mar 27, 2023

View reviewed changes

tomas-langer previously requested changes Mar 27, 2023

View reviewed changes

dalexandrov requested review from Verdent and tomas-langer March 28, 2023 11:58

dalexandrov force-pushed the 5817_MP_Telemetry branch from bf6b9f3 to 33731b7 Compare April 8, 2023 08:10

dalexandrov changed the base branch from main to helidon-3.x April 8, 2023 08:51

oracle-contributor-agreement bot added OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. and removed OCA Verified All contributors have signed the Oracle Contributor Agreement. labels Apr 8, 2023

dalexandrov changed the base branch from helidon-3.x to main April 8, 2023 08:51

oracle-contributor-agreement bot added OCA Verified All contributors have signed the Oracle Contributor Agreement. and removed OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. labels Apr 8, 2023

dalexandrov marked this pull request as draft April 8, 2023 08:52

dalexandrov force-pushed the 5817_MP_Telemetry branch from ac9b6de to de430ef Compare April 10, 2023 11:41

dalexandrov marked this pull request as ready for review April 10, 2023 12:08

dalexandrov force-pushed the 5817_MP_Telemetry branch 2 times, most recently from 02f4f91 to f5ef39c Compare April 19, 2023 13:30

dalexandrov mentioned this pull request Apr 25, 2023

[3.x] - Make Jaeger Tracer OpenTelemetry Agent aware. #6537

Merged

Verdent requested changes Apr 27, 2023

View reviewed changes

dalexandrov requested a review from Verdent April 27, 2023 13:48

dalexandrov added 2 commits May 3, 2023 15:56

Add MP OpenTelemetry support

77b139e

Remove redundant AgentDetection class

63d4a18

dalexandrov added 13 commits May 3, 2023 15:56

Add Agent Detector and tests

2bb1c1e

Minor properties fix

c7a9fbe

Properties filter applied

d3d109f

Filter reworked

1fa8d19

Filter reworked

1c25d61

Filter fixed

09c2db1

Filter fixed

c78cbe8

Filter fixed

21fb7d4

Filtering extended

b6ce874

Rollback to MPConfig

6148d0f

OpenTelemetry apply review comments

304f3e5

Minor code comments

c9b3fa0

Config in Agent detector may be null

0184110

dalexandrov force-pushed the 5817_MP_Telemetry branch from 85d6dc8 to 4c84dac Compare May 3, 2023 13:01

Fix dependencies

63fbd1f

dalexandrov force-pushed the 5817_MP_Telemetry branch from 4c84dac to 63fbd1f Compare May 3, 2023 13:08

Verdent requested changes May 4, 2023

View reviewed changes

dalexandrov added 2 commits May 4, 2023 15:13

Apply comments

1d4b1d1

Constants reuse.

4581ca3

dalexandrov requested a review from Verdent May 4, 2023 12:34

dalexandrov added 2 commits May 4, 2023 15:47

Externalize constants

a502f0b

Minor cleanup

00cd9f6

Verdent requested changes May 11, 2023

View reviewed changes

...profile/telemetry/src/main/java/io/helidon/microprofile/telemetry/OpenTelemetryProducer.java Show resolved Hide resolved

...pentelemetry/src/main/java/io/helidon/tracing/opentelemetry/OpenTelemetryTracerProvider.java Outdated Show resolved Hide resolved

dalexandrov requested a review from Verdent May 11, 2023 09:12

Minor fix

30432ed

Verdent approved these changes May 11, 2023

View reviewed changes

dalexandrov merged commit 5f9eb47 into helidon-io:main May 11, 2023

dalexandrov deleted the 5817_MP_Telemetry branch May 11, 2023 10:32

barchetta mentioned this pull request May 11, 2023

4.x: microprofile/telemetry might have unnecessary dependencies #6811

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4.x] - MicroProfile Telemetry Support #6493

[4.x] - MicroProfile Telemetry Support #6493

dalexandrov commented Mar 27, 2023

Verdent Mar 27, 2023

dalexandrov Mar 28, 2023

Verdent Apr 27, 2023

dalexandrov commented Apr 26, 2023

Verdent Apr 27, 2023

Verdent Apr 27, 2023

dalexandrov Apr 27, 2023

Verdent Apr 27, 2023

Verdent Apr 27, 2023

dalexandrov Apr 27, 2023

dalexandrov Apr 27, 2023

Verdent Apr 27, 2023

dalexandrov Apr 27, 2023

Verdent May 4, 2023

dalexandrov May 4, 2023

Verdent May 4, 2023

dalexandrov May 4, 2023

Verdent May 4, 2023

dalexandrov May 4, 2023

Verdent May 4, 2023

dalexandrov May 4, 2023

Verdent May 4, 2023

dalexandrov May 4, 2023

Verdent May 4, 2023

dalexandrov May 4, 2023

Verdent May 4, 2023

dalexandrov May 4, 2023

Verdent May 4, 2023

dalexandrov May 4, 2023

Verdent May 4, 2023

dalexandrov May 4, 2023

Verdent May 4, 2023

dalexandrov May 4, 2023

Verdent left a comment

		private static final String REST_RESOURCE_CLASS = "rest.resource.class";
		private static final String REST_RESOURCE_METHOD = "rest.resource.method";

		private Config config;

		private org.eclipse.microprofile.config.Config mpConfig;

[4.x] - MicroProfile Telemetry Support #6493

[4.x] - MicroProfile Telemetry Support #6493

Conversation

dalexandrov commented Mar 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalexandrov commented Apr 26, 2023

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

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

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

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

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

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

Verdent left a comment

Choose a reason for hiding this comment