HPCC-586 Add Otel Tracing support #714

rpastrana · 2024-05-31T03:12:56Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change is a breaking change (fix or feature that will cause existing behavior to change).

Checklist:

Testing:

rpastrana · 2024-05-31T14:27:37Z

wsclient/src/main/java/org/hpccsystems/ws/client/BaseHPCCWsClient.java

@@ -164,6 +202,89 @@ private String getTargetHPCCBuildVersionString() throws Exception

    }

+    public SpanBuilder getSpanBuilder(String spanName)
+    {
+        Tracer tracer = getTracer(INSTRUMENTED_LIBRARY_NAME);


use member tracer instead.

rpastrana · 2024-05-31T14:35:00Z

wsclient/src/main/java/org/hpccsystems/ws/client/BaseHPCCWsClient.java

+             })).build();
+             */
+        }
+        tracer = GlobalOpenTelemetry.getTracer(tracerName);


tracer should be static

it's preferred to track opentelemetry singleton and fetch tracer from it

rpastrana · 2024-05-31T14:42:12Z

wsclient/src/main/java/org/hpccsystems/ws/client/BaseHPCCWsClient.java

+        if (options != null)
+        {
+            if (currentSpan != null && currentSpan.getSpanContext().isValid())
+                W3CTraceContextPropagator.getInstance().inject(Context.current(), options, Options::setProperty);


should we propagate the Context.current(), or the currentSpan.getContext() passed in?

I think we should use currentSpan.getContext() for consistency. We could also have an override of injectTraceParentHeader() that doesn't require any input and grabs the current span via Span.current()

rpastrana · 2024-05-31T14:45:23Z

wsclient/src/main/java/org/hpccsystems/ws/client/HPCCWsWorkUnitsClient.java

        {
-            ((WsWorkunitsStub) stub).ping(request);
+            verifyStub();


pass in span

rpastrana · 2024-05-31T14:53:51Z

wsclient/src/main/java/org/hpccsystems/ws/client/HPCCWsWorkUnitsClient.java

-        Ping request = new Ping();
-
-        try
+        Span span = getSpanBuilder("WsWUClient_Ping").startSpan();


how to disable if tracing not desirable?

jpmcmu

@rpastrana look good, I don't see any major issues, but do have a few questions / comments

jpmcmu · 2024-05-31T19:03:49Z

wsclient/src/main/java/org/hpccsystems/ws/client/BaseHPCCWsClient.java

+        if (options != null)
+        {
+            if (currentSpan != null && currentSpan.getSpanContext().isValid())
+                W3CTraceContextPropagator.getInstance().inject(Context.current(), options, Options::setProperty);


I think we should use currentSpan.getContext() for consistency. We could also have an override of injectTraceParentHeader() that doesn't require any input and grabs the current span via Span.current()

jpmcmu · 2024-05-31T19:08:24Z

wsclient/src/main/java/org/hpccsystems/ws/client/BaseHPCCWsClient.java

+        {
+            Map<String, String> carrier = new HashMap<>();
+            TextMapSetter<Map<String, String>> setter = Map::put;
+            W3CTraceContextPropagator.getInstance().inject(Context.current(), carrier, setter);


Should we be using currentSpawn.getContext() here as well?

inject actually takes a Context rather than SpanContext

jpmcmu · 2024-05-31T19:16:19Z

wsclient/src/main/java/org/hpccsystems/ws/client/BaseHPCCWsClient.java

@@ -582,6 +726,10 @@ static public Stub setStubOptions(Stub thestub, Connection connection) throws Ax

        opt.setProperty(HTTPConstants.CHUNKED, Boolean.FALSE);

+        //only do this if tracing enabled?
+        //injectCurrentSpanTraceParentHeader(opt);
+        //opt.setProperty("traceparent", getTraceParentHeader());


Do we still need these comments?

jpmcmu · 2024-05-31T19:19:18Z

wsclient/src/test/java/org/hpccsystems/ws/client/BaseRemoteTest.java

@@ -240,7 +279,16 @@ public boolean verify(String hostname,javax.net.ssl.SSLSession sslSession)
        // Run the generate-datasets.ecl script if present in the project resources
        try
        {
-            executeECLScript("generate-datasets.ecl");
+             //Span eclscriptspan = tracer.spanBuilder("generate-datasets.ecl").startSpan();


I think it could be interesting to add tracing into our tests. This could be a good way to better understanding how the tracing works in practice, and it could help us track down issues. So, I think lets keep this trace code

rpastrana · 2024-05-31T21:01:33Z

@jpmcmu please review

jpmcmu

@rpastrana looks good

jpmcmu · 2024-06-04T14:01:05Z

pom.xml

+        </dependency>
+        <dependency>
+          <groupId>io.opentelemetry</groupId>
+          <artifactId>opentelemetry-exporter-otlp</artifactId>


@rpastrana Based on our discussion it probably makes sense to trim these dependencies. We can probably rely on the downstream client applications to provide the exporter jars they need / use.

jpmcmu · 2024-06-04T14:25:59Z

wsclient/src/main/java/org/hpccsystems/ws/client/BaseHPCCWsClient.java

+        //    -D.otel.logs.exporter=none
+        //    -Dotel.java.global-autoconfigure.enabled=true
+
+        globalOTel = GlobalOpenTelemetry.get();


How do we handle defaults here? Should we check if defaults have been set and if not default to no-op and / or logging exporter?

rpastrana · 2024-06-04T23:49:32Z

@jpmcmu see latest commit

rpastrana · 2024-06-05T14:14:59Z

wsclient/src/main/java/org/hpccsystems/ws/client/utils/Connection.java

+                .spanBuilder(method.toUpperCase() + " " + url.toExternalForm())
+                .setAttribute(ServerAttributes.SERVER_ADDRESS, getHost())
+                .setAttribute(ServerAttributes.SERVER_PORT, Long.getLong(getPort()))
+                .setAttribute(HttpAttributes.HTTP_REQUEST_METHOD, HttpAttributes.HttpRequestMethodValues.GET)


this won't always be an http get

jpmcmu

A few comments and I would also be curious to see what the trace output looks like from the ESP logging perspective.

jpmcmu · 2024-06-05T14:03:12Z

wsclient/src/main/java/org/hpccsystems/ws/client/BaseHPCCWsClient.java

+         * WARNING: Due to the inherent complications around initialization order involving this classand its single global instance, we strongly recommend *not* using GlobalOpenTelemetry unless youhave a use-case that absolutely requires it. Please favor using instances of OpenTelemetrywherever possible. 
+         *  If you are using the OpenTelemetry javaagent, it is generally best to only callGlobalOpenTelemetry.get() once, and then pass the resulting reference where you need to use it.
+        */
+


Based on our conversation lets remove this documentation about default config. Maybe also add a comment specifying what happens if the client application hasn't setup Otel.

jpmcmu · 2024-06-05T14:07:10Z

wsclient/src/test/java/org/hpccsystems/ws/client/BaseRemoteTest.java

+            }
+            catch (Exception e)
+            {
+                System.out.println("OpenTelemetry autoconfiguration failed: " + e.getMessage());


Will globalOTel be null if we get an exception here?

jpmcmu · 2024-06-05T14:18:17Z

wsclient/src/main/java/org/hpccsystems/ws/client/utils/Connection.java

-         int responseCode = httpURLConnection.getResponseCode(); //throws IOException
+        try (Scope scope = sendHTTPReqSpan.makeCurrent())
+        {
+            Span currentSpan = Span.current();


Should we use sendHTTPReqSpan here since currentSpan should always be sendHTTPReqSpan. Otherwise it is unclear why we would be expecting a different span.

jpmcmu · 2024-06-05T14:20:04Z

wsclient/src/main/java/org/hpccsystems/ws/client/utils/Connection.java

+                Map<String, String> carrier = new HashMap<>();
+                TextMapSetter<Map<String, String>> setter = Map::put;
+                W3CTraceContextPropagator.getInstance().inject(Context.current(), carrier, setter);
+                String traceparent = carrier.getOrDefault("traceparent", "00-" + currentSpan.getSpanContext().getTraceId() + "-" + currentSpan.getSpanContext().getSpanId() + "-00");


Would this make sense to move this functionality to create trace into a utility so that dfsclient can use this as well?

rpastrana · 2024-06-05T16:19:54Z

Sample trace output via console/log trace exporter:

INFO: 'FetchHPCCVersion' : 40d980b918457f3e20f1af6246069339 91d81b3e8a171805 CLIENT [tracer: WsClient:] AttributesMap{data={server.address=localhost, http.request.method=GET}, capacity=128, totalAddedValues=2}

INFO: 'GET http://localhost:8010/WsSMC/Activity?rawxml_' : 40d980b918457f3e20f1af6246069339 4ab0bf9585b5eb78 CLIENT [tracer: WsClient:] AttributesMap{data={server.address=localhost, hasCredentials=false, http.response.status_code=200, http.request.method=GET}, capacity=128, totalAddedValues=4}

Followed by corresponding ESP log output including server trace output:

00001051 PRG 2024-06-04 18:10:09.886  1648  3899 "GET /WsSMC/Activity, from 127.0.0.1"
00001052 PRG 2024-06-04 18:10:09.887  1648  3899 "TxSummary[activeReqs=1;auth=NA;contLen=-1;rcv=0;handleHttp=1;[email protected];req=GET wssmc.ACTIVITY v1.27;total=1;]"
00001053 MON 2024-06-04 18:10:09.888  1648  3899 "{ "type": "span", "name": "wssmc/activity", "trace_id": "40d980b918457f3e20f1af6246069339", "span_id": "d3bcbdcdb87e9088", "start": 1717539009887314522, "duration": 357500, "parent_span_id": "4ab0bf9585b5eb78", "attributes": {"http.request.method": "GET","id.local": "JmbS4Eu3dnweegcRerhoLz","id.global": "JmbS4Eu6bgXmd7i3C8nFw8" } }"

rpastrana · 2024-06-05T16:23:15Z

Sample JVM arguments used to autoconfigure Junit tests and/or platformtester

-Dotel.traces.exporter=console
-Dotel.metrics.exporter=none
-Dotel.logs.exporter=none
-Dotel.java.global-autoconfigure.enabled=true

jpmcmu

@rpastrana looks good

- Injects trace context in traceparent http header - Integrates autoconfigure otel SDK - Adds manual WsWUClient.ping span - Adds manual WsWUClientTest.ping span - Adds manual span around getHPCCver and getHPCCContainerizedMode - Adds manual http span on Utils.Connection.sendHTTPRequest - Injects traceparent http header on outgoing Utils.Connection.sendHTTPRequest - Only attempts to autoconfig remotetest if otel.java.global-autoconfig enabled - Adds root span around platform tester Signed-off-by: Rodrigo Pastrana <[email protected]>

rpastrana · 2024-06-06T12:53:40Z

@drealeed please take a look and let's discuss how your projects are/will be using OTEL for tracing etc.
Also, a similar change will applied to all rowservice calls immediately after this change in merged.

rpastrana requested a review from jpmcmu May 31, 2024 03:13

rpastrana commented May 31, 2024

View reviewed changes

jpmcmu approved these changes May 31, 2024

View reviewed changes

rpastrana force-pushed the HPCC4J-586 branch 2 times, most recently from fb5e881 to ad53e15 Compare May 31, 2024 20:58

rpastrana requested a review from jpmcmu May 31, 2024 20:58

jpmcmu approved these changes Jun 4, 2024

View reviewed changes

jpmcmu reviewed Jun 4, 2024

View reviewed changes

rpastrana force-pushed the HPCC4J-586 branch 2 times, most recently from 15cce48 to f763fda Compare June 4, 2024 22:31

rpastrana requested a review from jpmcmu June 4, 2024 23:28

rpastrana commented Jun 5, 2024

View reviewed changes

jpmcmu reviewed Jun 5, 2024

View reviewed changes

rpastrana requested a review from jpmcmu June 5, 2024 16:15

rpastrana force-pushed the HPCC4J-586 branch from 0a0ecb5 to bc84157 Compare June 5, 2024 16:22

rpastrana force-pushed the HPCC4J-586 branch from bc84157 to 9168faf Compare June 5, 2024 16:36

jpmcmu approved these changes Jun 6, 2024

View reviewed changes

rpastrana force-pushed the HPCC4J-586 branch from 9168faf to cd8a465 Compare June 6, 2024 12:49

rpastrana requested a review from drealeed June 6, 2024 12:51

rpastrana merged commit ce6160c into hpcc-systems:candidate-9.6.x Jun 7, 2024
4 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-586 Add Otel Tracing support #714

HPCC-586 Add Otel Tracing support #714

rpastrana commented May 31, 2024 •

edited

Loading

rpastrana May 31, 2024

rpastrana May 31, 2024

rpastrana May 31, 2024

rpastrana May 31, 2024

jpmcmu May 31, 2024

rpastrana May 31, 2024

rpastrana May 31, 2024

jpmcmu left a comment

jpmcmu May 31, 2024

jpmcmu May 31, 2024

rpastrana May 31, 2024

jpmcmu May 31, 2024

jpmcmu May 31, 2024

rpastrana commented May 31, 2024

jpmcmu left a comment

jpmcmu Jun 4, 2024

jpmcmu Jun 4, 2024

rpastrana commented Jun 4, 2024

rpastrana Jun 5, 2024

jpmcmu left a comment

jpmcmu Jun 5, 2024

jpmcmu Jun 5, 2024

jpmcmu Jun 5, 2024

jpmcmu Jun 5, 2024

rpastrana commented Jun 5, 2024

rpastrana commented Jun 5, 2024

jpmcmu left a comment

rpastrana commented Jun 6, 2024

HPCC-586 Add Otel Tracing support #714

HPCC-586 Add Otel Tracing support #714

Conversation

rpastrana commented May 31, 2024 • edited Loading

Type of change:

Checklist:

Testing:

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

jpmcmu 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpastrana commented May 31, 2024

jpmcmu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpastrana commented Jun 4, 2024

Choose a reason for hiding this comment

jpmcmu 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

Choose a reason for hiding this comment

rpastrana commented Jun 5, 2024

rpastrana commented Jun 5, 2024

jpmcmu left a comment

Choose a reason for hiding this comment

rpastrana commented Jun 6, 2024

rpastrana commented May 31, 2024 •

edited

Loading