Skip to content

Collect separate statistics for failed tasks #11327

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.

Sign up for GitHub

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

Merged
losipiuk merged 3 commits into from
Mar 8, 2022
Merged

Collect separate statistics for failed tasks #11327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
edcc5ae
Collect separate statistics for failed tasks
losipiuk Mar 4, 2022
8c4009f
Pass aggregated failed task stats to query completion event
losipiuk Mar 7, 2022
a574043
Aggregated cpu/scheduled time and cumlative memory for failed tasks i…
losipiuk Mar 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions core/trino-main/src/main/java/io/trino/dispatcher/FailedDispatchQuery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,8 @@ private static QueryStats immediateFailureQueryStats()
0,
0,
0,
0,
0,
DataSize.ofBytes(0),
DataSize.ofBytes(0),
DataSize.ofBytes(0),
Expand All @@ -283,19 +285,33 @@ private static QueryStats immediateFailureQueryStats()
new Duration(0, MILLISECONDS),
new Duration(0, MILLISECONDS),
new Duration(0, MILLISECONDS),
new Duration(0, MILLISECONDS),
new Duration(0, MILLISECONDS),
false,
ImmutableSet.of(),
DataSize.ofBytes(0),
DataSize.ofBytes(0),
0,
0,
new Duration(0, MILLISECONDS),
new Duration(0, MILLISECONDS),
DataSize.ofBytes(0),
DataSize.ofBytes(0),
0,
0,
DataSize.ofBytes(0),
DataSize.ofBytes(0),
0,
0,
DataSize.ofBytes(0),
DataSize.ofBytes(0),
0,
0,
DataSize.ofBytes(0),
DataSize.ofBytes(0),
0,
0,
DataSize.ofBytes(0),
DataSize.ofBytes(0),
ImmutableList.of(),
DynamicFiltersStats.EMPTY,
Expand Down
6 changes: 6 additions & 0 deletions core/trino-main/src/main/java/io/trino/event/QueryMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ public void queryImmediateFailureEvent(BasicQueryInfo queryInfo, ExecutionFailur
Optional.empty(),
Optional.empty()),
new QueryStatistics(
ofMillis(0),
ofMillis(0),
ofMillis(0),
ofMillis(queryInfo.getQueryStats().getQueuedTime().toMillis()),
Expand All @@ -174,6 +175,8 @@ public void queryImmediateFailureEvent(BasicQueryInfo queryInfo, ExecutionFailur
Optional.empty(),
Optional.empty(),
Optional.empty(),
Optional.empty(),
0,
0,
0,
0,
Expand Down Expand Up @@ -252,9 +255,11 @@ private QueryStatistics createQueryStatistics(QueryInfo queryInfo)
QueryStats queryStats = queryInfo.getQueryStats();
return new QueryStatistics(
ofMillis(queryStats.getTotalCpuTime().toMillis()),
ofMillis(queryStats.getFailedCpuTime().toMillis()),
ofMillis(queryStats.getElapsedTime().toMillis()),
ofMillis(queryStats.getQueuedTime().toMillis()),
Optional.of(ofMillis(queryStats.getTotalScheduledTime().toMillis())),
Optional.of(ofMillis(queryStats.getFailedScheduledTime().toMillis())),
Optional.of(ofMillis(queryStats.getResourceWaitingTime().toMillis())),
Optional.of(ofMillis(queryStats.getAnalysisTime().toMillis())),
Optional.of(ofMillis(queryStats.getPlanningTime().toMillis())),
Expand All @@ -273,6 +278,7 @@ private QueryStatistics createQueryStatistics(QueryInfo queryInfo)
queryStats.getLogicalWrittenDataSize().toBytes(),
queryStats.getWrittenPositions(),
queryStats.getCumulativeUserMemory(),
queryStats.getFailedCumulativeUserMemory(),
queryStats.getStageGcStatistics(),
queryStats.getCompletedDrivers(),
queryInfo.isCompleteInfo(),
Expand Down
36 changes: 36 additions & 0 deletions core/trino-main/src/main/java/io/trino/execution/BasicStageStats.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,13 @@ public class BasicStageStats
DataSize.ofBytes(0),
0,

0,
0,
DataSize.ofBytes(0),
DataSize.ofBytes(0),

new Duration(0, MILLISECONDS),
new Duration(0, MILLISECONDS),
new Duration(0, MILLISECONDS),
new Duration(0, MILLISECONDS),

Expand All @@ -72,10 +75,13 @@ public class BasicStageStats
private final DataSize rawInputDataSize;
private final long rawInputPositions;
private final long cumulativeUserMemory;
private final long failedCumulativeUserMemory;
private final DataSize userMemoryReservation;
private final DataSize totalMemoryReservation;
private final Duration totalCpuTime;
private final Duration failedCpuTime;
Copy link
Copy Markdown
Contributor

@arhimondr arhimondr Mar 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe "wasted" (here and in other places)?

Copy link
Copy Markdown
Member Author

@losipiuk losipiuk Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally like failed more as it is more precise and does not add emotional meaning to json field. But I do not feel super strongly. @martint / @findepi opinion?

Copy link
Copy Markdown
Member

@findepi findepi Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd prefer "failed" too.

btw do you plan anything like speculative execution, where work could become "wasted" without being "failed"?

Copy link
Copy Markdown
Member

@findepi findepi Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eg "cancelled"

Copy link
Copy Markdown
Member Author

@losipiuk losipiuk Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah - that will come up. But even then "wasted" is wrong term really. It has negative connotation, and if we are up to kill some tasks to speed up the execution of whole query it should not be seen as we are doing something wrong. And "wasting" resources feels wrong. So maybe some other term - neither "failed" nor "wasted" - but nothing comes to my mind.

Copy link
Copy Markdown
Member

@martint martint Mar 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I think "failed" is ok. The failure could be intrinsic (cluster / resource / system issues) or induced (task was forced to abort). In either case, it failed to complete.

private final Duration totalScheduledTime;
private final Duration failedScheduledTime;
private final boolean fullyBlocked;
private final Set<BlockedReason> blockedReasons;
private final OptionalDouble progressPercentage;
Expand All @@ -99,11 +105,14 @@ public BasicStageStats(
long rawInputPositions,

long cumulativeUserMemory,
long failedCumulativeUserMemory,
DataSize userMemoryReservation,
DataSize totalMemoryReservation,

Duration totalCpuTime,
Duration failedCpuTime,
Duration totalScheduledTime,
Duration failedScheduledTime,

boolean fullyBlocked,
Set<BlockedReason> blockedReasons,
Expand All @@ -123,10 +132,13 @@ public BasicStageStats(
this.rawInputDataSize = requireNonNull(rawInputDataSize, "rawInputDataSize is null");
this.rawInputPositions = rawInputPositions;
this.cumulativeUserMemory = cumulativeUserMemory;
this.failedCumulativeUserMemory = failedCumulativeUserMemory;
this.userMemoryReservation = requireNonNull(userMemoryReservation, "userMemoryReservation is null");
this.totalMemoryReservation = requireNonNull(totalMemoryReservation, "totalMemoryReservation is null");
this.totalCpuTime = requireNonNull(totalCpuTime, "totalCpuTime is null");
this.failedCpuTime = requireNonNull(failedCpuTime, "failedCpuTime is null");
this.totalScheduledTime = requireNonNull(totalScheduledTime, "totalScheduledTime is null");
this.failedScheduledTime = requireNonNull(failedScheduledTime, "failedScheduledTime is null");
this.fullyBlocked = fullyBlocked;
this.blockedReasons = ImmutableSet.copyOf(requireNonNull(blockedReasons, "blockedReasons is null"));
this.progressPercentage = requireNonNull(progressPercentage, "progressPercentage is null");
Expand Down Expand Up @@ -197,6 +209,11 @@ public long getCumulativeUserMemory()
return cumulativeUserMemory;
}

public long getFailedCumulativeUserMemory()
{
return failedCumulativeUserMemory;
}

public DataSize getUserMemoryReservation()
{
return userMemoryReservation;
Expand All @@ -212,11 +229,21 @@ public Duration getTotalCpuTime()
return totalCpuTime;
}

public Duration getFailedCpuTime()
{
return failedCpuTime;
}

public Duration getTotalScheduledTime()
{
return totalScheduledTime;
}

public Duration getFailedScheduledTime()
{
return failedScheduledTime;
}

public boolean isFullyBlocked()
{
return fullyBlocked;
Expand All @@ -240,11 +267,14 @@ public static BasicStageStats aggregateBasicStageStats(Iterable<BasicStageStats>
int completedDrivers = 0;

long cumulativeUserMemory = 0;
long failedCumulativeUserMemory = 0;
long userMemoryReservation = 0;
long totalMemoryReservation = 0;

long totalScheduledTimeMillis = 0;
long failedScheduledTimeMillis = 0;
long totalCpuTime = 0;
long failedCpuTime = 0;

long physicalInputDataSize = 0;
long physicalInputPositions = 0;
Expand All @@ -268,11 +298,14 @@ public static BasicStageStats aggregateBasicStageStats(Iterable<BasicStageStats>
completedDrivers += stageStats.getCompletedDrivers();

cumulativeUserMemory += stageStats.getCumulativeUserMemory();
failedCumulativeUserMemory += stageStats.getFailedCumulativeUserMemory();
userMemoryReservation += stageStats.getUserMemoryReservation().toBytes();
totalMemoryReservation += stageStats.getTotalMemoryReservation().toBytes();

totalScheduledTimeMillis += stageStats.getTotalScheduledTime().roundTo(MILLISECONDS);
failedScheduledTimeMillis += stageStats.getFailedScheduledTime().roundTo(MILLISECONDS);
totalCpuTime += stageStats.getTotalCpuTime().roundTo(MILLISECONDS);
failedCpuTime += stageStats.getFailedCpuTime().roundTo(MILLISECONDS);

isScheduled &= stageStats.isScheduled();

Expand Down Expand Up @@ -314,11 +347,14 @@ public static BasicStageStats aggregateBasicStageStats(Iterable<BasicStageStats>
rawInputPositions,

cumulativeUserMemory,
failedCumulativeUserMemory,
succinctBytes(userMemoryReservation),
succinctBytes(totalMemoryReservation),

new Duration(totalCpuTime, MILLISECONDS).convertToMostSuccinctTimeUnit(),
new Duration(failedCpuTime, MILLISECONDS).convertToMostSuccinctTimeUnit(),
new Duration(totalScheduledTimeMillis, MILLISECONDS).convertToMostSuccinctTimeUnit(),
new Duration(failedScheduledTimeMillis, MILLISECONDS).convertToMostSuccinctTimeUnit(),

fullyBlocked,
blockedReasons,
Expand Down
Loading