Add customTraceAttributes to session #22179

mduggan-starburst · 2024-05-28T20:45:58Z

Description

This change is support custom trace attributes for both the QueryMonitor logging of completed queries and OpenTelemetry Trace objects for queries. It has no effect other than the additional information logged for queries that are run on Trino.

The attributes will be printed with a prefix "custom:" so that there will not be a collision with any existing attributes.

Additional context and related issues

When a query is created, it has an associated Session. This session contains context information regarding the query and additionally is part of the QueryInfo object passed to the QueryMonitor, which logs information regarding completed queries. By creating a member in the Session (as well as the associated SessionContext and SessionRepresentation) the custom trace attributes are available downstream in the query lifecycle.

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Add custom trace attributes to query session. ({issue}`22178`)

cla-bot · 2024-05-28T20:46:01Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

mduggan-starburst · 2024-05-29T12:47:21Z

[email protected]

CTA sent.

losipiuk · 2024-05-29T14:04:52Z

core/trino-main/src/main/java/io/trino/Session.java

@@ -118,7 +120,8 @@ public Session(
 SessionPropertyManager sessionPropertyManager,
 Map<String, String> preparedStatements,
 ProtocolHeaders protocolHeaders,
- Optional<Slice> exchangeEncryptionKey)
+ Optional<Slice> exchangeEncryptionKey,
+ Map<String, String> customTraceAttributes)


can you call it extraTraceAttributes or extraTracingAttributes?

I originally used the term "extra" but when it came to adding a prefix to avoid collisions with other top level names "extra" seemed confusing. I figured "custom" was less confusing so I used that and followed with this name too.

What was the confusing place with extra?

When I try to put myself in the place of an admin looking at some attribute for a query with a prefix "extra:..." I think what "extra" is referring to is ambiguous. I don't think that it would be evident that "extra" is referring to the fact that it's an extra attribute in addition to the typical ones and that it could mean that the extra is in reference to the name of the attribute. "custom", however, I think is much more clear that the attribute itself is custom and the semantic is more clear. That might just be me though.

core/trino-main/src/main/java/io/trino/Session.java

losipiuk · 2024-05-29T14:07:12Z

core/trino-main/src/main/java/io/trino/Session.java

@@ -930,6 +951,31 @@ public SessionBuilder setProtocolHeaders(ProtocolHeaders protocolHeaders)
 return this;
 }

+ @CanIgnoreReturnValue
+ public SessionBuilder addCustomTraceAttribute(String name, String value)


addExtraTraceAttribute

losipiuk · 2024-05-29T14:07:28Z

core/trino-main/src/main/java/io/trino/Session.java

+ }
+
+ @CanIgnoreReturnValue
+ public SessionBuilder addAllCustomTraceAttribute(Map<String, String> customTraceAttributes)


addExtraTraceAttributes

core/trino-main/src/main/java/io/trino/dispatcher/QueuedStatementResource.java

losipiuk · 2024-05-29T14:09:33Z

core/trino-main/src/main/java/io/trino/event/QueryMonitor.java

 {
- log.info("TIMELINE: Query %s :: %s%s :: elapsed %sms :: planning %sms :: waiting %sms :: scheduling %sms :: running %sms :: finishing %sms :: begin %s :: end %s",
+ String customAttributeSection = customTraceAttributes.entrySet().stream()


why do we need to change this log message?

This was the goal of the tracking, along with adding to the trace objects. It allows for profiling query performance for specific client types from these results along with the traces.

I see it quite arbitrary to put this specific information in this log message. I would keep it just in opentelemetry for now.

cla-bot · 2024-05-29T15:51:24Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-05-29T19:31:14Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-05-30T15:08:58Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-05-30T15:20:53Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-05-30T15:50:08Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

losipiuk · 2024-06-03T13:39:24Z

core/trino-main/src/main/java/io/trino/dispatcher/QueuedStatementResource.java

+ SpanBuilder spanBuilder = tracer.spanBuilder("query")
+ .setAttribute(TrinoAttributes.QUERY_ID, queryId.toString());
+ sessionContext.getCustomTraceAttributes().forEach((key, value) ->
+ spanBuilder.setAttribute("custom:%s".formatted(key), value));


docs suggest:

Note: It is strongly recommended to use setAttribute(AttributeKey, Object), and pre-allocate your keys, if possible.

consider having a map String->AttributeKey so your keys are ~singletons.

losipiuk · 2024-06-03T13:57:19Z

core/trino-main/src/main/java/io/trino/ExtraTraceAttributes.java

+ /**
+ * @throws java.util.NoSuchElementException if the query is not found
+ */
+ public String getClientTags(QueryId queryId)
+ {
+ return getExtraTraceAttributes(queryId).get(CLIENT_TAGS);
+ }


not used drop

losipiuk · 2024-06-03T13:57:26Z

core/trino-main/src/main/java/io/trino/ExtraTraceAttributes.java

+ PriorityQueue<String> valQueue = new PriorityQueue<>(vals);
+ return valQueue.stream().collect(Collectors.joining(","));


return vals.stream().sorted().collect(joining(","));

github-actions · 2024-06-24T17:02:53Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-06-24T17:21:22Z

@cla-bot check

cla-bot · 2024-06-24T17:21:25Z

The cla-bot has been summoned, and re-checked this pull request!

mosabua · 2024-06-24T17:21:59Z

@mduggan-starburst can you address comments?

@electrum and @mattstep can you chime in?

mduggan-starburst · 2024-06-24T18:54:57Z

Closed as unnecessary.

losipiuk reviewed May 29, 2024

View reviewed changes

core/trino-main/src/main/java/io/trino/Session.java Outdated Show resolved Hide resolved

losipiuk reviewed May 29, 2024

View reviewed changes

core/trino-main/src/main/java/io/trino/Session.java Outdated Show resolved Hide resolved

losipiuk reviewed May 29, 2024

View reviewed changes

core/trino-main/src/main/java/io/trino/dispatcher/QueuedStatementResource.java Outdated Show resolved Hide resolved

losipiuk reviewed May 29, 2024

View reviewed changes

mduggan-starburst force-pushed the duggan/add-session-properties-query-monitor branch from f3bc68c to f2e91cc Compare May 29, 2024 15:51

mduggan-starburst force-pushed the duggan/add-session-properties-query-monitor branch from f2e91cc to 05c17ad Compare May 29, 2024 19:31

mduggan-starburst requested a review from losipiuk May 29, 2024 19:31

mduggan-starburst force-pushed the duggan/add-session-properties-query-monitor branch from 05c17ad to c15fdd9 Compare May 30, 2024 15:08

mduggan-starburst force-pushed the duggan/add-session-properties-query-monitor branch from c15fdd9 to 002892e Compare May 30, 2024 15:20

Add customTraceAttributes to session

22857a7

mduggan-starburst force-pushed the duggan/add-session-properties-query-monitor branch from 002892e to 22857a7 Compare May 30, 2024 15:50

losipiuk reviewed Jun 3, 2024

View reviewed changes

github-actions bot added the stale label Jun 24, 2024

cla-bot bot added the cla-signed label Jun 24, 2024

mduggan-starburst closed this Jun 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add customTraceAttributes to session #22179

Add customTraceAttributes to session #22179

mduggan-starburst commented May 28, 2024

cla-bot bot commented May 28, 2024

mduggan-starburst commented May 29, 2024

losipiuk May 29, 2024

mduggan-starburst May 29, 2024

losipiuk Jun 3, 2024

mduggan-starburst Jun 3, 2024 •

edited

Loading

losipiuk May 29, 2024

losipiuk May 29, 2024

losipiuk May 29, 2024

mduggan-starburst May 29, 2024

losipiuk Jun 3, 2024

cla-bot bot commented May 29, 2024

cla-bot bot commented May 29, 2024

cla-bot bot commented May 30, 2024

cla-bot bot commented May 30, 2024

cla-bot bot commented May 30, 2024

losipiuk Jun 3, 2024

losipiuk Jun 3, 2024

losipiuk Jun 3, 2024 •

edited

Loading

github-actions bot commented Jun 24, 2024

mosabua commented Jun 24, 2024

cla-bot bot commented Jun 24, 2024

mosabua commented Jun 24, 2024

mduggan-starburst commented Jun 24, 2024

		PriorityQueue<String> valQueue = new PriorityQueue<>(vals);
		return valQueue.stream().collect(Collectors.joining(","));

Add customTraceAttributes to session #22179

Add customTraceAttributes to session #22179

Conversation

mduggan-starburst commented May 28, 2024

Description

Additional context and related issues

Release notes

cla-bot bot commented May 28, 2024

mduggan-starburst commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mduggan-starburst Jun 3, 2024 • 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

cla-bot bot commented May 29, 2024

cla-bot bot commented May 29, 2024

cla-bot bot commented May 30, 2024

cla-bot bot commented May 30, 2024

cla-bot bot commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jun 24, 2024

mosabua commented Jun 24, 2024

cla-bot bot commented Jun 24, 2024

mosabua commented Jun 24, 2024

mduggan-starburst commented Jun 24, 2024

mduggan-starburst Jun 3, 2024 •

edited

Loading

losipiuk Jun 3, 2024 •

edited

Loading