OTel Tracing implementation

[sureshbabu-areekara]

Description

PR to enhance tracing with Open Telemetry. RFC :- https://github.com/prestodb/rfcs/pull/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[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: sureshbabu-areekara (9f28d5b31e997cd3c0e58ec73056d9e30a676392)

[sureshbabu-areekara]

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]

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.

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

cc @bentonyjoe191 @ethanyzhang

[sureshbabu-areekara]

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

[ethanyzhang]

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

[sureshbabu-areekara]

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

Hi @ethanyzhang addressed the conflicts. Thanks!!

[ethanyzhang]

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

[agrawalreetika]

@sureshbabu-areekara Please look into CI test failures.

[sureshbabu-areekara]

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]

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

[sureshbabu-areekara]

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!!

[sureshbabu-areekara]

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!!