OTel Tracing implementation #24133

sureshbabu-areekara · 2024-11-25T05:17:21Z

Description

PR to enhance tracing with Open Telemetry.
RFC :- prestodb/rfcs#33

Motivation and Context

Enhancing the tracing with Open Telemetry satisfies the need for observability in Presto so that customer can do Performance monitoring, debugging, Collection and export of telemetry data such as traces to observability dashboards.

Impact

Test Plan

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* ... :pr:`12345`
* ... :pr:`12345`

Hive Connector Changes
* ... :pr:`12345`
* ... :pr:`12345`

If release note is NOT required, use:

== NO RELEASE NOTE ==

Co-Authors

@bentonyjoe191
@siddhuoo7

linux-foundation-easycla · 2024-11-25T05:17:25Z

The committers listed above are authorized under a signed CLA.

✅ login: sureshbabu-areekara (66d4b8f, b04afe0)

sureshbabu-areekara · 2025-01-07T10:23:49Z

Hi @agrawalreetika As of now refactored the code to remove the direct open telemetry sdk dependency from presto-main. But still we added the presto-open-telemetry module in presto-main/pom.xml. Will try to make it as pluggable so that code won't fail if we remove the dependency.

cc @ethanyzhang @bentonyjoe191

sureshbabu-areekara · 2025-01-10T13:14:55Z

Hi @agrawalreetika We are facing issues proceeding with the pluggable approach. As of now we propagate TracingSpan and ScopedSpan (Wrappers of SDK Span) throughout the presto-main code, the module dependency on presto-open-telemetry in presto-main cannot be removed. It would be great if you could advise an alternate solution for this issue. Thanks!!

Or else we could keep the open telemetry sdk in presto-main and go for plugin approach.

So the possible solutions would be.

Remove all open telemetry sdk from main and add presto-open-telemetry module as dependency.
Add open telemetry sdk in presto-main and go for plugin approach.

cc @bentonyjoe191 @ethanyzhang

sureshbabu-areekara · 2025-01-20T09:18:50Z

Hi @agrawalreetika Pushed the changes to implement Plugin approach.
cc @ethanyzhang @bentonyjoe191

ethanyzhang · 2025-01-24T07:31:42Z

@sureshbabu-areekara can you address the conflict with master?

sureshbabu-areekara · 2025-01-27T07:18:26Z

@sureshbabu-areekara can you address the conflict with master?

Hi @ethanyzhang addressed the conflicts. Thanks!!

ethanyzhang · 2025-01-27T15:20:31Z

Hi @sureshbabu-areekara, please check the tests... they all failed.

sureshbabu-areekara · 2025-02-05T13:43:08Z

Hi @ethanyzhang @agrawalreetika All the tests except 1 passed now. The failed one seems not cos of OTel changes and which has passed before. @agrawalreetika could you please help me to rerun the failed test as I don't have options/access.

sureshbabu-areekara · 2025-02-06T05:37:19Z

Hi @agrawalreetika All passed now. Can you start reviewing it?
@ethanyzhang

steveburnett

In order for this new page of doc to appear on https://prestodb.io/docs/current/plugin.html, please update https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/plugin.rst.

sureshbabu-areekara · 2025-02-06T14:59:45Z

In order for this new page of doc to appear on https://prestodb.io/docs/current/plugin.html, please update https://github.com/prestodb/presto/blob/master/presto-docs/src/main/sphinx/plugin.rst.

Hi @steveburnett Added.. Thanks!!

steveburnett

Thanks for the doc! Some nits of punctuation and a couple of minor suggestions of phrasing.

Something I want to suggest that you think about - in the Configuration Properties table, consider adding a third column Default for default values. A property with no default values, such as tracing-backend-url, would have a blank cell in its row of that column. But several of these seem to me to have default values that a reader might find helpful to have them shown here. Just an idea to think about.

presto-docs/src/main/sphinx/plugin/open-telemetry.rst

sureshbabu-areekara · 2025-02-07T05:09:07Z

Thanks for the doc! Some nits of punctuation and a couple of minor suggestions of phrasing.

Something I want to suggest that you think about - in the Configuration Properties table, consider adding a third column Default for default values. A property with no default values, such as tracing-backend-url, would have a blank cell in its row of that column. But several of these seem to me to have default values that a reader might find helpful to have them shown here. Just an idea to think about.

Hi @steveburnett Thanks correction and suggestion. Added default values.. Thanks!!

steveburnett

LGTM! (docs)

Pull updated branch, new local doc build, looks great. Thanks!

agrawalreetika · 2025-02-07T17:55:01Z

presto-native-sidecar-plugin/pom.xml

+                    <groupId>org.jetbrains.kotlin</groupId>
+                    <artifactId>kotlin-stdlib</artifactId>
+                </exclusion>
+            </exclusions>


This is causing test failure about related class not found, please check - https://github.com/prestodb/presto/actions/runs/13199217100/job/36866051461?pr=24133

java.lang.NoClassDefFoundError: kotlin/jvm/internal/Intrinsics

This is causing test failure about related class not found, please check - https://github.com/prestodb/presto/actions/runs/13199217100/job/36866051461?pr=24133

java.lang.NoClassDefFoundError: kotlin/jvm/internal/Intrinsics

Hi @agrawalreetika Resolved. https://github.com/prestodb/presto/actions/runs/13238120590/job/36947725249?pr=24133

agrawalreetika · 2025-02-16T19:01:23Z

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

@@ -21,7 +21,6 @@ public final class PrestoHeaders
    public static final String PRESTO_SCHEMA = "X-Presto-Schema";
    public static final String PRESTO_TIME_ZONE = "X-Presto-Time-Zone";
    public static final String PRESTO_LANGUAGE = "X-Presto-Language";
-    public static final String PRESTO_TRACE_TOKEN = "X-Presto-Trace-Token";


Why are we removing the trace token client option?

Older OTel implementation cannot extend to support the features implemented in new version. So we removed the older implementation completely and gone for a new one. The X-Presto-Trace-Token was part of older one and which is not used now.

agrawalreetika · 2025-02-16T19:12:27Z

...t/src/main/java/com/facebook/presto/client/okhttp3/internal/tls/DistinguishedNameParser.java

+    private final int length;
+    private int pos;
+    private int beg;
+    private int end;


Please use descriptive names for variables in this class, these are not clear.
Also we can take out extra comments in different places.

agrawalreetika · 2025-02-16T19:24:29Z

...t/src/main/java/com/facebook/presto/client/okhttp3/internal/tls/DistinguishedNameParser.java

+
+        // attribute type chars
+        pos++;
+        for (; pos < length && chars[pos] != '=' && chars[pos] != ' '; pos++) {


Nit: I think readability wise using while is better than this.

agrawalreetika · 2025-02-16T19:24:36Z

...t/src/main/java/com/facebook/presto/client/okhttp3/internal/tls/DistinguishedNameParser.java

+        end = pos;
+
+        if (chars[pos] == ' ') {
+            for (; pos < length && chars[pos] != '=' && chars[pos] == ' '; pos++) {


Nit: I think readability wise using while is better than this?

agrawalreetika · 2025-02-16T19:24:42Z

...t/src/main/java/com/facebook/presto/client/okhttp3/internal/tls/DistinguishedNameParser.java

+
+        pos++; //skip '=' char
+
+        for (; pos < length && chars[pos] == ' '; pos++) {


Nit: I think readability wise using while is better than this.

agrawalreetika · 2025-02-16T19:52:59Z

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

@@ -530,13 +530,14 @@ public String toString()
    {
        return toStringHelper(this)
                .add("queryId", queryId)
+                .add("querySpan", TracingManager.spanString(querySpan).orElse(null))


use static import

agrawalreetika · 2025-02-16T19:54:15Z

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

@@ -562,21 +563,21 @@ public static SessionBuilder builder(Session session)
    public static class SessionBuilder
    {
        private QueryId queryId;
+        private BaseSpan querySpan = TracingManager.getInvalidSpan(); //do not initialize with null


use static import

agrawalreetika · 2025-02-16T19:54:22Z

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

@@ -562,21 +563,21 @@ public static SessionBuilder builder(Session session)
    public static class SessionBuilder
    {
        private QueryId queryId;
+        private BaseSpan querySpan = TracingManager.getInvalidSpan(); //do not initialize with null
+        private BaseSpan rootSpan = TracingManager.getInvalidSpan();  //do not initialize with null


use static import

agrawalreetika · 2025-02-16T19:55:25Z

presto-common/src/main/java/com/facebook/presto/common/TelemetryConfig.java

+    private Integer scheduleDelay;
+    private Double samplingRatio;
+    private Boolean tracingEnabled = false;
+    private Boolean spanSampling = false;


Explicit initialization is not needed

agrawalreetika · 2025-02-16T19:58:07Z

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

@@ -246,7 +245,6 @@ public final class SystemSessionProperties
    public static final String RESOURCE_AWARE_SCHEDULING_STRATEGY = "resource_aware_scheduling_strategy";
    public static final String HEAP_DUMP_ON_EXCEEDED_MEMORY_LIMIT_ENABLED = "heap_dump_on_exceeded_memory_limit_enabled";
    public static final String EXCEEDED_MEMORY_LIMIT_HEAP_DUMP_FILE_DIRECTORY = "exceeded_memory_limit_heap_dump_file_directory";
-    public static final String DISTRIBUTED_TRACING_MODE = "distributed_tracing_mode";


Why are we removing existing tracing mode?

Older OTel implementation cannot extend to support the features implemented in new version. So we removed the older implementation completely and gone for a new one. The distributed_tracing_mode was part of older one and which is not used now.

aaneja

Did a very quick first pass. I want to get a better handle on the SPI changes, before doing a full review

aaneja · 2025-02-17T16:48:05Z

presto-spi/src/main/java/com/facebook/presto/spi/telemetry/TelemetryFactory.java


-public interface TracerHandle
+/**


The Javadocs are not adding any real value. They simply are spelling out the function names. Please either add a valid description, or remove

Will be updated.

aaneja · 2025-02-17T16:48:50Z

presto-spi/src/main/java/com/facebook/presto/spi/telemetry/TelemetryTracing.java

+import java.util.Optional;
+
+/**
+ * The interface Telemetry tracing.


These Javadocs are not adding any value IMO. Please add more detailed descriptions

Will be updated.

aaneja · 2025-02-17T17:04:58Z

presto-spi/src/main/java/com/facebook/presto/spi/eventlistener/QueryMetadata.java

@@ -25,7 +25,6 @@ public class QueryMetadata
 {
    private final String queryId;
    private final Optional<String> transactionId;
-    private final Optional<String> tracingId;


I don't think we should remove support for this mechanism of tracing - this is unrelated to OTel ?

This is related to old OTel implementation. The old implementation was removed in the PR and redesigned as the old one is Map based and cannot be extended to support new features like inner spans. Also previously there were only less number of spans(<10) for particular query. Now there are 150+ spans which cannot be handled in a Map based implementation.

aaneja · 2025-02-17T17:50:22Z

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

+import io.opentelemetry.api.trace.Span;
+
+/**
+ * The type Tracing span.


What is a TracingSpan and how does it differ from a ScopedSpan conceptually ?

TracingSpan and ScopedSpan is just a wrapper class for the actual SDK span. TracingSpan we can use it where we know the scope and can manually start and end. ScopedSpan implements AutoCloseable and can be used within a try-with-resources block.

aaneja · 2025-02-17T17:53:26Z

presto-spi/src/main/java/com/facebook/presto/spi/telemetry/TelemetryTracing.java

+    /**
+     * Load configured open telemetry.
+     */
+    void loadConfiguredOpenTelemetry();


Rename to loadTelemetryPlugin ?

TelemetryPlugin is actually loaded by PluginManager before this method call. loadConfiguredOpenTelemetry() is for creating the opentelemetry and tracer instance using the loaded plugin. So I think the name can be change to something like setOpenTelemetry() or createOpenTelemetry().

OSS Presto OTel Tracing changes

Review comments.

sureshbabu-areekara requested review from elharo, a team, shrinidhijoshi, jaystarshot, feilong-liu and ClarenceThreepwood as code owners November 25, 2024 05:17

sureshbabu-areekara requested a review from presto-oss November 25, 2024 05:17

prestodb-ci added the from:IBM PR from IBM label Nov 25, 2024

prestodb-ci requested review from a team, pdabre12 and czentgr and removed request for a team November 25, 2024 05:17

sureshbabu-areekara marked this pull request as draft November 25, 2024 05:20

sureshbabu-areekara mentioned this pull request Nov 25, 2024

RFC for Open Telemetry Implementation for Presto. prestodb/rfcs#33

Open

sureshbabu-areekara force-pushed the otel-impl-oss branch from edeed24 to 65a4e2a Compare November 25, 2024 09:08

sureshbabu-areekara force-pushed the otel-impl-oss branch 2 times, most recently from 93d1192 to 3eaccc6 Compare January 6, 2025 18:59

sureshbabu-areekara force-pushed the otel-impl-oss branch from 81757d8 to 8884a54 Compare January 20, 2025 12:54

sureshbabu-areekara force-pushed the otel-impl-oss branch 3 times, most recently from bcd7aeb to 406c6c1 Compare January 27, 2025 03:39

sureshbabu-areekara force-pushed the otel-impl-oss branch from 87f0140 to e19b091 Compare January 27, 2025 13:14

sureshbabu-areekara force-pushed the otel-impl-oss branch from e19b091 to 58de25d Compare January 28, 2025 17:18

sureshbabu-areekara force-pushed the otel-impl-oss branch from 2c53382 to 31414a8 Compare February 4, 2025 19:17

sureshbabu-areekara force-pushed the otel-impl-oss branch from eb1d811 to 14f4e67 Compare February 6, 2025 13:55

steveburnett requested changes Feb 6, 2025

View reviewed changes

sureshbabu-areekara force-pushed the otel-impl-oss branch from 14f4e67 to 17c4623 Compare February 6, 2025 14:58

steveburnett requested changes Feb 6, 2025

View reviewed changes

sureshbabu-areekara force-pushed the otel-impl-oss branch 2 times, most recently from 5b70260 to e32d0c5 Compare February 7, 2025 05:07

sureshbabu-areekara marked this pull request as ready for review February 7, 2025 05:10

sureshbabu-areekara force-pushed the otel-impl-oss branch from e32d0c5 to 4cc6153 Compare February 7, 2025 11:48

steveburnett previously approved these changes Feb 7, 2025

View reviewed changes

agrawalreetika reviewed Feb 7, 2025

View reviewed changes

sureshbabu-areekara dismissed steveburnett’s stale review via 5fc77b5 February 10, 2025 07:37

sureshbabu-areekara force-pushed the otel-impl-oss branch from 9ebb179 to bea510d Compare February 10, 2025 12:11

agrawalreetika requested changes Feb 16, 2025

View reviewed changes

ethanyzhang requested a review from aaneja February 17, 2025 03:31

sureshbabu-areekara force-pushed the otel-impl-oss branch 3 times, most recently from 78ae2d9 to 893aab8 Compare February 17, 2025 15:29

aaneja reviewed Feb 17, 2025

View reviewed changes

sureshbabu-areekara force-pushed the otel-impl-oss branch 2 times, most recently from b568c3d to eb9c96f Compare February 18, 2025 14:34

sureshbabu-areekara requested a review from agrawalreetika February 19, 2025 11:55

sureshbabu-areekara force-pushed the otel-impl-oss branch from eb9c96f to 0c630ce Compare February 20, 2025 06:25

sureshbabu-areekara added 2 commits February 20, 2025 19:32

OSS Presto OTel Tracing changes

66d4b8f

OSS Presto OTel Tracing changes

Review comments.

b04afe0

Review comments.

sureshbabu-areekara force-pushed the otel-impl-oss branch from 0c630ce to b04afe0 Compare February 20, 2025 14:02


		pos++; //skip '=' char

		for (; pos < length && chars[pos] == ' '; pos++) {

OTel Tracing implementation #24133

Are you sure you want to change the base?

OTel Tracing implementation #24133

Conversation

sureshbabu-areekara commented Nov 25, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Co-Authors

linux-foundation-easycla bot commented Nov 25, 2024 • edited Loading

sureshbabu-areekara commented Jan 7, 2025

sureshbabu-areekara commented Jan 10, 2025 • edited Loading

sureshbabu-areekara commented Jan 20, 2025

ethanyzhang commented Jan 24, 2025

sureshbabu-areekara commented Jan 27, 2025

ethanyzhang commented Jan 27, 2025

sureshbabu-areekara commented Feb 5, 2025

sureshbabu-areekara commented Feb 6, 2025

steveburnett left a comment

Choose a reason for hiding this comment

sureshbabu-areekara commented Feb 6, 2025

steveburnett left a comment

Choose a reason for hiding this comment

sureshbabu-areekara commented Feb 7, 2025

steveburnett left a comment

Choose a reason for hiding this comment

agrawalreetika Feb 7, 2025 • 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

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

aaneja 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

sureshbabu-areekara Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

sureshbabu-areekara commented Nov 25, 2024 •

edited

Loading

linux-foundation-easycla bot commented Nov 25, 2024 •

edited

Loading

sureshbabu-areekara commented Jan 10, 2025 •

edited

Loading

agrawalreetika Feb 7, 2025 •

edited

Loading

sureshbabu-areekara Feb 18, 2025 •

edited

Loading