-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Query debugging tracer #2010
Query debugging tracer #2010
Conversation
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jiapeng Tao.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jtao15 One high level comment before reviewing, can you please separate the work into separate commits? eg. introduction of Tracer, propagation in the engine, propagation to connectors, separate commits for different categories of events etc
94a955d
to
19308a7
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jiapeng Tao.
|
19308a7
to
5c991a5
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jiapeng Tao.
|
a9cfbec
to
d942c9e
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jiapeng Tao.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. I've added some comments.
presto-main/src/main/java/io/prestosql/eventlistener/EventListenerManager.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/SystemSessionProperties.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/tracer/TracerFactory.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/server/PrestoServer.java
Outdated
Show resolved
Hide resolved
presto-spi/src/main/java/io/prestosql/spi/eventlistener/TracerEvent.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/server/remotetask/HttpRemoteTask.java
Outdated
Show resolved
Hide resolved
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jiapeng Tao.
|
1 similar comment
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jiapeng Tao.
|
d326106
to
fffe58a
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jiapeng Tao.
|
1 similar comment
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jiapeng Tao.
|
6446a42
to
bd85aa1
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jiapeng Tao.
|
presto-main/src/main/java/io/prestosql/execution/SqlTaskExecution.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/execution/SqlTaskExecution.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/execution/SqlTaskExecution.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/io/prestosql/execution/TestMemoryRevokingScheduler.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/io/prestosql/execution/TestSqlTask.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/execution/executor/TaskExecutor.java
Outdated
Show resolved
Hide resolved
private final String mockProperty; | ||
|
||
@JsonCreator | ||
public MockSplit(@JsonProperty("mockProperty") String mockProperty) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JsonCodec throws errors when it converts an empty split to json payload, so I added a dummy field here.
page = page.getLoadedPage(); | ||
if (tracer.isEnabled()) { | ||
Map<String, Object> payload = new HashMap<>(); | ||
payload.put("page", page.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the page here. Remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the page.toString() will not include the extra data, and we may be interested which page it is when an event listener receive a PageLoaded
event.
bd85aa1
to
f111c9d
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jiapeng Tao.
|
f111c9d
to
d2311a9
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jiapeng Tao.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I've some top level and inline comments.
- Restricting access to tracer object
We want the connectors to be able to emit events, with limited access to the engine side tracer framework. In the PR right now, we have ConnectorTracerFactory::createTracer
method that takes in the engine tracer itself. I see that the ConnectorTracer has only one constructor which requires the private engine tracer object. But this pattern does not prevent connectors from storing tracer as another object in the class. And then they can invoke methods on the engine tracer. For example, HiveConnectorTracer
can store the engine tracer as another member variable, and use it to create new tracer, or emit events that are not engine specific without proper namespacing.
I was thikning about another approach to circumvent this issue. Instead of ConnectorTracerFactory::createTracer
taking in the tracer as an argument, it'd take a ConnectorEventEmitter, which encapsulates Tracer object and restricts access to it. It also takes care of namespacing the events.
It can look like the following:
ConnectorEventEmitter
{
String connectorName; // I think you have this as "source" right now
Tracer tracer;
ConnectorEventEmitter(String connectorName, Tracer tracer)
{
this.connectorName = connectorName;
this.tracer = tracer;
}
public emitEvent(string actionType, payload)
{
String eventName = concatenate(connectorName, actionType);
tracer.emitEvent(eventName, payload);
}
// if required
public boolean isEnabled()
{
return tracer.isEnabled();
}
}
The ConnectorTracerFactory would look like the following:
public class ConnectorTracerFactory
{
public ConnectorTracer createConnectorTracer(ConnectorEventEmitter emitter);
}
public interface ConnectorTracer() {}
In this case, the connector tracers can store a reference to ConnectorEventEmitter
, and invoke emitEvent
when required. This also helps namespace the types.
- Encapsulating the connector tracer providers:
It seems that we are adding ConnectorTracerFactories to the SplitManager for the coordinator side execution. I think there is a better way to do this. Presto's codebase follows this design pattern where engine specific "manager" objects register and keep track of connector specific objects. For example, SplitManager
stores a mapping for catalog specific ConnectorSplitManager
objects. Similarly is a map for ConnectorPageSourceProvider
s in PageSourceManager
. We can maintain that pattern here as well. We should register the connector specific factories/providers (i.e. a map from catalogName
to ConnectorTracerProvider
) in another engine specific object, say "ConnectorTracerManager", and have this object injected whenever required. For instance, we can inject it as a constructor to the SplitManager or PageSourceManager.
- Thanks for posting the event details, I've added my thoughts on what we should add in this first pass, would love to hear others' comments.
Events dependent on state-changes: let's use .name()
and .values()
methods for enum instead of manually mapping all states cleanly.
-
QUERY:
(prepareQuery
parses the query. Logical and distributed planning happens in SqlQueryExecution. We should have the following events.)
parsing start and end
logical plan start and end
distributed planning start and end
Query state change events -
STAGE:
Stage state change events
Task sending updates
(ADD_SPLITS_TO_TASK and SCHEDULE_TASK_WITH_SPLITS <-- not sure about the how useful they are, given how the updates are scheduled.) -
TASK:
Task state change events
also record when the task execution is created
LocalExecution Plan start and end -
PIPELINE:
none
(I'd drop the event for adding pipeline context: I don't think there is anything latency-critical here) -
SPLIT RUNNER:
Driver Creation
Scheduled, blocked, finished, destroyed etc -
Operator:
We're mostly interested inpagesources
. I'd say the implementations in TableScanOperator may just load/propagate lazy blocks, and we can skip tracking that information for now. -
Hive Connector
- Directory listing events (start and end looks good. We should notice that there's also a
CachingDirectoryLister
, but it's okay not to care about it here. - I believe we're yet to add the page source events in the PR.
- Directory listing events (start and end looks good. We should notice that there's also a
Also, please look at travis failures
presto-benchmark/src/main/java/io/prestosql/benchmark/AbstractOperatorBenchmark.java
Outdated
Show resolved
Hide resolved
presto-spi/src/main/java/io/prestosql/spi/tracer/NoOpTracer.java
Outdated
Show resolved
Hide resolved
presto-spi/src/main/java/io/prestosql/spi/tracer/DefaultTracer.java
Outdated
Show resolved
Hide resolved
presto-spi/src/main/java/io/prestosql/spi/tracer/DefaultTracer.java
Outdated
Show resolved
Hide resolved
presto-spi/src/main/java/io/prestosql/spi/tracer/DefaultTracer.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/tracer/TracerActionType.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/tracer/TracerActionType.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/tracer/TracerActionType.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/util/HiveFileIterator.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/BackgroundHiveSplitLoader.java
Outdated
Show resolved
Hide resolved
d2311a9
to
0163686
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Jiapeng Tao.
|
9f46bb8
to
cb595b0
Compare
presto-hive/src/test/java/io/prestosql/plugin/hive/TestBackgroundHiveSplitLoader.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/split/PageSourceManager.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/HivePageSourceProvider.java
Outdated
Show resolved
Hide resolved
presto-hive/src/main/java/io/prestosql/plugin/hive/util/HiveFileIterator.java
Outdated
Show resolved
Hide resolved
presto-orc/src/test/java/io/prestosql/orc/TestOrcReaderPositions.java
Outdated
Show resolved
Hide resolved
Block block = blockReader.readBlock(); | ||
if (loadFully) { | ||
block = block.getLoadedBlock(); | ||
} | ||
if (block != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last commit: we can inline toString
into toJson() call, and remove block != null check from here. Btw, we'll just get a Block object address here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the check. BlockInfo
is needed here since only final fields can be referenced in lamda expression.
presto-hive/src/test/java/io/prestosql/plugin/hive/TestBackgroundHiveSplitLoader.java
Outdated
Show resolved
Hide resolved
presto-hive/src/test/java/io/prestosql/plugin/hive/TestBackgroundHiveSplitLoader.java
Outdated
Show resolved
Hide resolved
e6abebe
to
60442d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments for the "Tracer introduction" commit.
presto-main/src/main/java/io/prestosql/tracer/DefaultTracerFactory.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/tracer/NoOpTracerFactory.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/tracer/TracerFactory.java
Outdated
Show resolved
Hide resolved
public class TracerEvent | ||
{ | ||
private final String nodeId; | ||
private final String uri; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What URI does this represent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the URI of the node where the tracer events are sent.
presto-spi/src/main/java/io/prestosql/spi/eventlistener/TracerEvent.java
Outdated
Show resolved
Hide resolved
@@ -26,4 +26,8 @@ default void queryCompleted(QueryCompletedEvent queryCompletedEvent) | |||
default void splitCompleted(SplitCompletedEvent splitCompletedEvent) | |||
{ | |||
} | |||
|
|||
default void tracerEventOccurred(TracerEvent tracerEvent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method name sounds funny (I don't have any suggestions for alternate names, yet -- let me think about it).
08e2f38
to
a44de48
Compare
@jtao15 can you please fix the failing test? |
a44de48
to
f52cdbe
Compare
@jtao15 conflicts |
@jtao15 This can be awesome!!! is there any chance you will continue this? |
👋 @jtao15 - this PR is inactive and doesn't seem to be under development, and it might already be implemented. If you'd like to continue work on this at any point in the future, feel free to re-open. |
Add query debugging tracer for Presto as #1826
This pr includes tracer creation and propagations and some basic tracer events.
Tracer high level design
Objectives
Key design points
Tracers track interesting execution actions per query
Context dependency
Tracers keep the execution contexts tracked, such as
queryId
,stageId
,taskId
,worker nodeId
, etc. such that tracers events can be uniquely identified.Introduction of tracers
EventListenerManger
is injected in tracer as an event consumer (emitter) when the tracer is created byTracerFactroy
.Tracer defined in SPI package are wrapped in
ConnectorEmitter
such thatConnectorTracer
can be created with emitters without explicitly referencing an engine tracer.Hierarchical
As query executes hierarchically, tracers are created and propagated correspondingly. QueryTracer is created before the query is planned on coordinator.
StageTracers
are created based on the context of their parentQueryTracer
with stageId assigned.TaskTracers
are then created following the parent StageTracers.On workers,
TaskTracers
are created from scratch with task context (queryId, stageId, taskId) assigned.DriverTracers
andOperatorTracers
are created accordingly as the execution propagates.ConnectorTracers
are created byConnectorTracerFactory
withConnectorTracerEmitter
(wrapper of engine tracer). In connectors theConnectorTracer
is propagated through connector context, the context is extensible to include other information for future development so there will not be an interface change.Tracer events are semi-structured. Structured fields include execution object ids, event type, worker node id, timestamp, etc. Other information is kept as generic payload(implementation yet to be decided).
TracerEventType
enums andConnectorEventType
enums share the same interface:EventTypeSupplier
, so that connectors are able to define their own event types with connector name prefix.Proposed events details