integrate and implement open telemetry tracing #18534

achen2289 · 2022-10-20T22:17:51Z

Test plan -

Set Open Telemetry tracer and ensured query state changes are tracked as well as query completes execution successfully.
Ensured spans of same query are grouped under same parent span / traceId and any prior context, if provided, is properly propagated
Ensured baggage propagation is supported as well as set as tags of new spans
Tested trace exporting (export to Datadog agent)
Configured without open telemetry as well as with SimpleTracer / NoopTracer to ensure backwards compatibility
Tested edge cases with system config not matching plugin config

== RELEASE NOTES ==

General Changes
* Introduced a new plugin type for tracing. The legacy tracer module can be replaced by the new plugin for loading customized tracing infrastructure.

Open Telemetry Changes
* Introduced a new Open Telemetry tracer implementation. Users are able to enable the tracer by installing the ``presto-open-telemetry`` plugin and updating the application configuration (``config.properties``). Open Telemetry tracer can take in propagated context (only B3 specification currently supported) and baggage (W3C specification) headers, if provided, and inject into new traces / spans. Traces can be exported to any specified backend with the ``OTEL_EXPORTER_OTLP_ENDPOINT`` environment variable.

linux-foundation-easycla · 2022-10-20T22:17:59Z

The committers listed above are authorized under a signed CLA.

✅ login: achen2289 / name: Alex Chen (21888a73662611aaee0fd34fc6ee20aa48646f34, f1099bde939e1aa1cc1568d93faa9f33a8285ace, 1eab0578bfbad5c047744cf68c9ad5e4fdd828d5, 9c47baad2aaeeca5a41fe418ccfce1400932cfc2)

cliandy · 2022-10-27T19:54:22Z

@highker could we get a first pass on this?

tanjialiang

Reviewed the first commit

presto-main/etc/config.properties

presto-main/src/main/java/com/facebook/presto/tracing/OpenTelemetryTracer.java

tanjialiang · 2022-10-27T23:23:08Z

presto-main/src/main/java/com/facebook/presto/tracing/OpenTelemetryTracer.java

+    @Override
+    public void endTrace(String annotation)
+    {
+        addPoint(annotation);


Do we need to take care of any unfinished Span?

Can you help me understanding the closing logic of opentelemetry here? Thank you

I am looking into this

@tanjialiang Addressed by ending any unended spans at the time of ending trace.

presto-main/src/main/java/com/facebook/presto/tracing/OpenTelemetryTracer.java

presto-main/src/main/java/com/facebook/presto/tracing/OpenTelemetryTracerProvider.java

presto-main/src/main/java/com/facebook/presto/tracing/OpenTelemetryTracer.java

highker

high-level comments only

presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java

presto-spi/src/main/java/com/facebook/presto/spi/tracing/TracerProvider.java

presto-main/src/main/java/com/facebook/presto/tracing/TracingConfig.java

pom.xml

achen2289 · 2022-11-23T19:20:55Z

Hey @highker, I want to try to get my changes pushed through by the end of next week if possible. If there's anything in this PR that still needs further discussion or you still want revised, I'm happy to remove it from this PR and address it as a new one for the future if that works.

highker

Please squash all commits into one.

I have one major comment in the end. Please check that one first.

highker · 2022-11-24T04:45:38Z

presto-client/src/main/java/com/facebook/presto/client/PrestoHeaders.java

+    public static final String PRESTO_W3C_PROPAGATION = "traceparent";
+    public static final String PRESTO_B3_SINGLE_HEADER_PROPAGATION = "b3";
+    public static final String PRESTO_BAGGAGE_HEADER = "baggage";


Let's remove these headers to presto-open-telemetry. Check my other comment

highker · 2022-11-24T04:47:23Z

presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java

+                stringProperty(
+                        CONTEXT_PROPAGATOR,
+                        "Context propagator for Open Telemetry tracing. w3c, b3_single_header, or b3_multiple_header",
+                        tracingConfig.getContextPropagator(),
+                        false),


Can we use the header instead of session property to control the flags?

highker · 2022-11-24T04:48:30Z

presto-main/src/main/java/com/facebook/presto/server/HttpRequestSessionContext.java

        String tunnelTraceId = trimEmptyToNull(servletRequest.getHeader(PRESTO_TRACE_TOKEN));
+        String contextPropagator = sessionPropertyManager
+                .map(manager -> manager.decodeSystemPropertyValue(CONTEXT_PROPAGATOR, null, String.class))
+                .orElse(TracingConfig.ContextPropagator.B3_SINGLE_HEADER);
+        String propagatedContext = getPropagatedContext(contextPropagator, servletRequest);
+        String baggage = servletRequest.getHeader(PRESTO_BAGGAGE_HEADER);


Let's refactor the code a bit so we don't have to use session property or the header explicitly. Check my other comment

highker · 2022-11-24T04:48:46Z

presto-main/src/main/java/com/facebook/presto/server/HttpRequestSessionContext.java

+     * @param servletRequest http request with headers
+     * @return header value extracted from http request based on context propagator
+     */
+    private static String getPropagatedContext(String contextPropagator, HttpServletRequest servletRequest)


Move this to presto-open-telemetry module. Check my other comment

highker · 2022-11-24T04:50:03Z

presto-main/src/main/java/com/facebook/presto/tracing/TracerProviderManager.java

+{
+    private final String tracerType;
+    private final boolean systemTracingEnabled;
+    private @Nullable TracerProvider tracerProvider;


nit

@Nullable private TracerProvider tracerProvider;

highker · 2022-11-24T04:50:51Z

presto-main/src/main/java/com/facebook/presto/tracing/TracerProviderManager.java

+
+    public TracerProvider getTracerProvider()
+    {
+        return tracerProvider;


check non null

highker · 2022-11-24T04:52:00Z

presto-open-telemetry/src/main/java/com/facebook/presto/opentelemetry/OpenTelemetryBuilder.java

+        OpenTelemetry openTelemetry = OpenTelemetrySdk.builder()
+                .setTracerProvider(sdkTracerProvider)
+                .setPropagators(ContextPropagators.create(OpenTelemetryBuilder.getPropagatorInstance(contextPropagator)))
+                .buildAndRegisterGlobal();
+
+        return openTelemetry;


directly return instead of creating openTelemetry

highker · 2022-11-24T05:10:26Z

presto-spi/src/main/java/com/facebook/presto/spi/tracing/TracerProvider.java

 public interface TracerProvider
 {
+    String getName();
+
+    String getTracerType();
    /**
     *
     * @return A @Tracer that should be kept throughout the whole duration of tracing.
     */
-    Tracer getNewTracer();
+    Tracer getNewTracer(String traceToken, String contextPropagator, String propagatedContext, String baggage);


[Major] this is my major comment.

Similar to many other "handle" classes in our codebase, let's create a handle interface in com.facebook.presto.spi.tracing:

public interface TracerHandle { }

Change the signature of getNewTracer to

Tracer getNewTracer(TracerHandle handle);

Create a new method of TracerProvider:

/** * The method returns a function to take a set of HTTP headers and generate the tracer handle */ Function<Set<String>, TracerHandle> getHandleGenerator();

In your OpenTelemetry implementation, you should have

public class OpenTelemetryTracerHandle implements TracerHandle { private final String traceToken; private final String contextPropagator; private final String propagatedContext; private final String baggage; ... }

In your OpenTelemetryTracerProvider, you should have

@Override public Tracer getNewTracer(TracerHandle handle) { OpenTelemetryTracerHandle tracerHandle = (TracerHandle) handle; return new OpenTelemetryTracer(tracerHandle.getXXX, ...); }

In your getHandleGenerator implementation, move all the logics you have in presto-main here so we can hide all those headers and specialized extraction methods.

highker · 2022-11-24T05:11:02Z

presto-spi/src/main/java/com/facebook/presto/spi/Plugin.java

@@ -115,4 +116,9 @@ default Iterable<HistoryBasedPlanStatisticsProvider> getHistoryBasedPlanStatisti
    {
        return emptyList();
    }
+
+    default Iterable<TracerProvider> getTracerProviders()


add javadoc

highker · 2022-11-24T05:11:09Z

presto-spi/src/main/java/com/facebook/presto/spi/tracing/TracerProvider.java

+    String getName();
+
+    String getTracerType();


add javadoc

achen2289 · 2022-11-30T01:26:48Z

@highker Trace handle implementation done

highker

awesome change! All nits. Otherwise, good to go

highker · 2022-11-30T04:44:50Z

presto-main/src/main/java/com/facebook/presto/server/HttpRequestSessionContext.java

@@ -211,19 +211,33 @@ else if (nameParts.size() == 2) {

        this.sessionFunctions = parseSessionFunctionHeader(servletRequest);
        this.sessionPropertyManager = requireNonNull(sessionPropertyManager, "sessionPropertyManager is null");
-        String tunnelTraceId = trimEmptyToNull(servletRequest.getHeader(PRESTO_TRACE_TOKEN));


I know it's a bit anti pattern; but in order to keep the logic so we don't pollute the traceToken below. Check my comment below.

highker · 2022-11-30T04:46:05Z

presto-main/src/main/java/com/facebook/presto/server/HttpRequestSessionContext.java


            // If tunnel trace token is null, we expose the Presto tracing id.
            // Otherwise we preserve the ability of trace token tunneling but
            // still trace Presto internally for aggregation purposes.
-            traceToken = Optional.ofNullable(tunnelTraceId == null ? this.tracer.get().getTracerId() : tunnelTraceId);
+            traceToken = Optional.ofNullable(this.tracer.get().getTracerId());
        }
        else {


Add a new branch here: else if (trimEmptyToNull(servletRequest.getHeader(PRESTO_TRACE_TOKEN)) != null) or something like that. And inside the body, we can either create a SimpleTracerProvider and get the token or directly assign traceToken from PRESTO_TRACE_TOKEN

updated trace token to replicate similar logic as before

highker · 2022-11-30T04:46:23Z

presto-main/src/main/java/com/facebook/presto/server/HttpRequestSessionContext.java

+
+    private static Map<String, String> getRequestHeaderMap(HttpServletRequest servletRequest)
+    {
+        Map<String, String> headerMap = new HashMap<>();


Use immutableMap builder

highker · 2022-11-30T04:47:48Z

presto-main/src/main/java/com/facebook/presto/server/HttpRequestSessionContext.java

+        }
+    }
+
+    private static Map<String, String> getRequestHeaderMap(HttpServletRequest servletRequest)


nit: it's not critical but good to follow:

we don't usually name a function or var with word "Map"; instead, we use getRequestHeaders. Same for the variable headerMap --> headers. And everything else in the PR.

highker · 2022-11-30T04:48:07Z

presto-main/src/main/java/com/facebook/presto/server/HttpRequestSessionContext.java

@@ -211,19 +211,33 @@ else if (nameParts.size() == 2) {

        this.sessionFunctions = parseSessionFunctionHeader(servletRequest);
        this.sessionPropertyManager = requireNonNull(sessionPropertyManager, "sessionPropertyManager is null");
-        String tunnelTraceId = trimEmptyToNull(servletRequest.getHeader(PRESTO_TRACE_TOKEN));
+
+        Map<String, String> requestHeaderMap = getRequestHeaderMap(servletRequest);


nit: requestHeaders

highker · 2022-11-30T04:59:52Z

...n-telemetry/src/main/java/com/facebook/presto/opentelemetry/OpenTelemetryTracerProvider.java

+        if (headers.containsKey(PRESTO_B3_SINGLE_HEADER_PROPAGATION)) {
+            return "b3_single_header";
+        }
+        else if (headers.containsKey(PRESTO_W3C_PROPAGATION)) {


remove else; it's redundant

highker · 2022-11-30T05:00:00Z

...n-telemetry/src/main/java/com/facebook/presto/opentelemetry/OpenTelemetryTracerProvider.java

+                    CONTEXT_PROPAGATOR_ERROR,
+                    "Only b3 single header context propagation mode is currently supported.");
+        }
+        else {


remove else; it's redundant

highker · 2022-11-30T05:00:52Z

...n-telemetry/src/main/java/com/facebook/presto/opentelemetry/OpenTelemetryTracerProvider.java

+            if (contextPropagator.equals(TracingConfig.ContextPropagator.B3_SINGLE_HEADER)) {
+                return headers.get(PRESTO_B3_SINGLE_HEADER_PROPAGATION);
+            }
+            else if (contextPropagator.equals(TracingConfig.ContextPropagator.W3C)) {


remove else; it's redundant

highker · 2022-11-30T05:01:59Z

presto-spi/src/main/java/com/facebook/presto/spi/StandardErrorCode.java

@@ -113,6 +113,7 @@
    QUERY_PLANNING_TIMEOUT(0x0001_0027, INTERNAL_ERROR),
    NATIVE_EXECUTION_TASK_ERROR(0x0001_0028, INTERNAL_ERROR),
    UNSUPPORTED_ANALYZER_TYPE(0x0001_0029, INTERNAL_ERROR),
+    CONTEXT_PROPAGATOR_ERROR(0x0001_002A, INTERNAL_ERROR),


this is not a generic error; check my other comment

highker · 2022-11-30T05:03:25Z

...n-telemetry/src/main/java/com/facebook/presto/opentelemetry/OpenTelemetryTracerProvider.java

+            throw new PrestoException(
+                    CONTEXT_PROPAGATOR_ERROR,
+                    "Only b3 single header context propagation mode is currently supported.");


two options:

you can create your own error code class within the module with a brand new base code; check any other implementation of ErrorCodeSupplier; or

you can just throw runtime error or put generic code to be a catch-all to save the effort, which is not recommended but fine

achen2289 · 2022-11-30T22:53:28Z

@highker Addressed nits, and made one further change for open telemetry baggage propagation. Put it in new commit so it's easier to see and will squash if it looks good.

highker · 2022-11-30T22:58:20Z

Thanks. Please squash all commits into one.

highker

more nits; otherwise LGTM

highker · 2022-11-30T23:52:52Z

presto-main/src/main/java/com/facebook/presto/server/HttpRequestSessionContext.java

@@ -211,19 +212,39 @@ else if (nameParts.size() == 2) {

        this.sessionFunctions = parseSessionFunctionHeader(servletRequest);
        this.sessionPropertyManager = requireNonNull(sessionPropertyManager, "sessionPropertyManager is null");
-        String tunnelTraceId = trimEmptyToNull(servletRequest.getHeader(PRESTO_TRACE_TOKEN));
+
+        ImmutableMap<String, String> requestHeaders = getRequestHeaders(servletRequest);


nit: ImmutableMap<String, String> -> Map<String, String>

highker · 2022-11-30T23:53:25Z

presto-main/src/main/java/com/facebook/presto/server/HttpRequestSessionContext.java

-            traceToken = Optional.ofNullable(tunnelTraceId == null ? this.tracer.get().getTracerId() : tunnelTraceId);
+            String tunnelTraceId = trimEmptyToNull(servletRequest.getHeader(PRESTO_TRACE_TOKEN));
+            if (tunnelTraceId != null) {
+                traceToken = Optional.ofNullable(tunnelTraceId);


nit: Optional.of(tunnelTraceId)

it's not null already

highker · 2022-11-30T23:53:54Z

presto-main/src/main/java/com/facebook/presto/server/HttpRequestSessionContext.java

-            traceToken = Optional.ofNullable(tunnelTraceId);
+    }
+
+    private static ImmutableMap<String, String> getRequestHeaders(HttpServletRequest servletRequest)


nit: ImmutableMap<String, String> -> Map<String, String>

highker · 2022-11-30T23:55:21Z

presto-main/src/main/java/com/facebook/presto/tracing/TracingConfig.java

+    public static class ContextPropagator
+    {
+        public static final String W3C = "w3c";
+        public static final String B3_SINGLE_HEADER = "b3_single_header";
    }


comment not addressed? https://github.com/prestodb/presto/pull/18534/files#r1035527320

highker · 2022-11-30T23:59:05Z

...o-open-telemetry/src/main/java/com/facebook/presto/opentelemetry/OpenTelemetryErrorCode.java

+
+    OpenTelemetryErrorCode(int code, ErrorType type)
+    {
+        errorCode = new ErrorCode(code, name(), type);


We need a base so we don't collide with other error codes. Find a unique one closest to an existing one. I assume it's starting with 0x05... something

highker · 2022-12-01T00:00:46Z

presto-open-telemetry/src/main/java/com/facebook/presto/opentelemetry/OpenTelemetryTracer.java

+            this.openTelemetry = OpenTelemetryBuilder.build(contextPropagator);
+            this.currentContextPropagator = contextPropagator;


I'm not expert in open telemetry but are we sure the logic is correct by modifying static variables?

I believe it should be fine. If we have a different context propagator, we need to rebuild the open telemetry instance for all tracers that may exist.

highker

One last nit

highker · 2022-12-01T05:23:35Z

presto-main/src/main/java/com/facebook/presto/tracing/TracingConfig.java

+        public static final String OTEL = "otel";
+    }
+
+    public static class ContextPropagator


Can we delete this class from this file?

support simple tracer in new open telemetry implementation bump okhttp3 version to 4.5.0 use system val if client val not set add support for context extraction from b3 propagation and overall interface revert back to okhttp3 version 3.9.0 for this PR implement W3C baggage header extraction tracer handle implementation, remove context propagator from system property, general refactor address nits and open telemetry baggage updates

achen2289 · 2022-12-01T21:01:58Z

@highker Thanks for the helpful reviews and merge!

achen2289 requested a review from a team as a code owner October 20, 2022 22:17

achen2289 requested a review from presto-oss October 20, 2022 22:17

achen2289 force-pushed the achen/integrate-and-implement-open-telemetry-tracing branch 8 times, most recently from 119f1c7 to 49d6041 Compare October 26, 2022 00:16

rohanpednekar added the Triaged label Oct 26, 2022

achen2289 force-pushed the achen/integrate-and-implement-open-telemetry-tracing branch from f070e75 to 3fd2443 Compare October 26, 2022 16:52

highker requested review from tanjialiang and highker October 27, 2022 20:46

tanjialiang reviewed Oct 27, 2022

View reviewed changes

achen2289 force-pushed the achen/integrate-and-implement-open-telemetry-tracing branch 2 times, most recently from a2a2097 to e932e63 Compare October 28, 2022 04:30

highker requested changes Oct 31, 2022

View reviewed changes

rohanpednekar removed the Triaged label Oct 31, 2022

achen2289 force-pushed the achen/integrate-and-implement-open-telemetry-tracing branch 9 times, most recently from 5173a23 to 35a6292 Compare November 3, 2022 17:42

achen2289 force-pushed the achen/integrate-and-implement-open-telemetry-tracing branch 2 times, most recently from 936cf9e to ee19b16 Compare November 23, 2022 02:28

highker reviewed Nov 24, 2022

View reviewed changes

achen2289 force-pushed the achen/integrate-and-implement-open-telemetry-tracing branch 6 times, most recently from d171ea9 to 1e44c8f Compare November 29, 2022 19:02

highker self-requested a review November 30, 2022 04:32

highker reviewed Nov 30, 2022

View reviewed changes

highker self-requested a review November 30, 2022 22:57

achen2289 force-pushed the achen/integrate-and-implement-open-telemetry-tracing branch from 11edf44 to 19a4184 Compare November 30, 2022 23:07

highker approved these changes Dec 1, 2022

View reviewed changes

highker self-assigned this Dec 1, 2022

achen2289 force-pushed the achen/integrate-and-implement-open-telemetry-tracing branch 3 times, most recently from d227668 to 6ad17a0 Compare December 1, 2022 04:22

highker approved these changes Dec 1, 2022

View reviewed changes

achen2289 force-pushed the achen/integrate-and-implement-open-telemetry-tracing branch from 6ad17a0 to 54c6530 Compare December 1, 2022 05:32

highker changed the title ~~achen/integrate and implement open telemetry tracing~~ integrate and implement open telemetry tracing Dec 1, 2022

highker merged commit 2c1d402 into prestodb:master Dec 1, 2022

wanglinsong mentioned this pull request Jan 12, 2023

Add release notes for 0.279 #18920

Merged

30 tasks

sureshbabu-areekara mentioned this pull request Nov 7, 2024

Enhance Open Telemetry Implementation in Presto #23975

Open

		this.openTelemetry = OpenTelemetryBuilder.build(contextPropagator);
		this.currentContextPropagator = contextPropagator;

integrate and implement open telemetry tracing #18534

integrate and implement open telemetry tracing #18534

Conversation

achen2289 commented Oct 20, 2022 • edited by highker Loading

linux-foundation-easycla bot commented Oct 20, 2022 • edited Loading

cliandy commented Oct 27, 2022

tanjialiang 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

highker left a comment

Choose a reason for hiding this comment

achen2289 commented Nov 23, 2022

highker 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

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

achen2289 commented Nov 30, 2022

highker 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

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

achen2289 commented Nov 30, 2022

highker commented Nov 30, 2022

highker 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

highker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achen2289 commented Dec 1, 2022

achen2289 commented Oct 20, 2022 •

edited by highker

Loading

linux-foundation-easycla bot commented Oct 20, 2022 •

edited

Loading