Add scheduler span under query that aggregates all stage spans #20096
Conversation
wweiss-starburst
force-pushed
the
wweiss/add-runner-span
branch
3 times, most recently
from
8702503 to
3838e1a
Compare
core/trino-main/src/main/java/io/trino/exchange/ExchangeContextInstance.java
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/exchange/LazyExchangeDataSource.java
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/exchange/LazyExchangeDataSource.java
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/operator/DirectExchangeClientSupplier.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/execution/scheduler/PipelinedQueryScheduler.java
Show resolved
Hide resolved
| @@ -835,6 +844,7 @@ public void run() | |||
| failure = closeAndAddSuppressed(failure, nodeAllocator); | |||
|
|
|||
| failure.ifPresent(queryStateMachine::transitionToFailed); | |||
| schedulerSpan.end(); | |||
There was a problem hiding this comment.
should we attach failure to schedulerSpan if present?
There was a problem hiding this comment.
I added a new event that will write the exception's getMessage() to a new FAILURE attribute key
core/trino-main/src/main/java/io/trino/operator/DirectExchangeClientFactory.java
Outdated
Show resolved
Hide resolved
...ilesystem/src/test/java/io/trino/plugin/exchange/filesystem/AbstractTestExchangeManager.java
Outdated
Show resolved
Hide resolved
.../test/java/io/trino/plugin/exchange/filesystem/local/TestLocalFileSystemExchangeManager.java
Outdated
Show resolved
Hide resolved
wweiss-starburst
force-pushed
the
wweiss/add-runner-span
branch
2 times, most recently
from
25cb072 to
938be6d
Compare
| @Experimental(eta = "2023-09-01") | ||
| public class ExchangeContext | ||
| public interface ExchangeContext |
There was a problem hiding this comment.
Based on the current usages, we could have leave ExchangeContext as a concrete class.
What's the reason to make it an interface?
There was a problem hiding this comment.
Adding the Span ends up affecting all the OpenTelemetry dependencies changing them from runtime to provided. That ends up rippling into all the other pom files. It makes the PR much larger.
There was a problem hiding this comment.
When adding a span to ExchangeContext, it creates a new dependency on opentelemetry-context. The build error is:
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project trino-spi: Compilation failure
[ERROR] /Users/walter.weiss/git/opensource/os_trino/core/trino-spi/src/main/java/io/trino/spi/exchange/ExchangeContext.java:[33,27] cannot access io.opentelemetry.context.ImplicitContextKeyed
[ERROR] class file for io.opentelemetry.context.ImplicitContextKeyed not found
To get around this, I changed the dependency scope of opentelemetry-context from runtime to provided. However, that forces a change in scope for nearly all the other projects as well, so to minimize the blast radius it seemed more straightforward to change ExchangeContext to an interface. If you have an alternative suggestion, I am open to it.
| default OpenTelemetry getOpenTelemetry() | ||
| { | ||
| throw new UnsupportedOperationException(); | ||
| } | ||
|
|
||
| default Tracer getTracer() | ||
| { | ||
| throw new UnsupportedOperationException(); | ||
| } |
There was a problem hiding this comment.
remove default implementations. the implementations are supposed to be complete
There was a problem hiding this comment.
This is the same pattern ConnectorContext. The reason for having this as an interface (as well as migrating ExchangeContext to interface) is that otherwise we would need to make trino-spi depend on parts of opentelemetry implementation opentelemetry-context. Not only opentelemetry-api.
core/trino-spi/src/main/java/io/trino/spi/exchange/ExchangeContext.java
Outdated
Show resolved
Hide resolved
wweiss-starburst
force-pushed
the
wweiss/add-runner-span
branch
2 times, most recently
from
aefa4a6 to
abd4cb0
Compare
JSON serialzability of ExchangeSourceHandles is required in some contexts.
The Trino spans below the the Query span are Dispatch, Analyzer, Planner and then a sequence of Stage spans. Placing the set of Stage spans under a new Scheduler span allows all execution spans to be grouped together. In addition, new Exchange spans are also being added to instrument the exchanges that operate across stages. Normalize getter in ExchangeContext interface.
The experimental designation for exchanges had an ETA of 9/1/23 and can now be removed.
losipiuk
force-pushed
the
wweiss/add-runner-span
branch
from
abd4cb0 to
1bf2ee6
Compare
|
Does this need a release note? |
|
@colebow no |
Description
The Trino spans below the the Query span are Dispatch, Analyzer, Planner and then a sequence of Stage spans. Placing the set of Stage spans under a new Scheduler span allows all execution spans to be grouped together. In
addition, new Exchange spans are also being added to instrument the exchanges that operate across stages.
Additional context and related issues
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: