Skip to content

Adding missing task run hierarchy to TaskRun table #1332

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
merged 8 commits into from
Sep 20, 2024
Merged

Adding missing task run hierarchy to TaskRun table #1332

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9a5aa58
Add task run hierarchical relationships to the database
ericallam Sep 18, 2024
7b48a7a
Add depth and related runs to the retrieve run API response
ericallam Sep 19, 2024
fc18a71
Remove prisma optimize
ericallam Sep 20, 2024
c1b3d44
restructure the migrations to create the index concurrently
ericallam Sep 20, 2024
8366599
Delete these tsbuildinfo files
ericallam Sep 20, 2024
0b984a5
Fix type error by adding depth to the run list presenter
ericallam Sep 20, 2024
6dac1a2
Cleanup the task hierarchy, share more code
ericallam Sep 20, 2024
013bd47
Remove some fields from the list run response
ericallam Sep 20, 2024
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
Prev Previous commit
Next Next commit
Add depth and related runs to the retrieve run API response
  • Loading branch information
@ericallam
ericallam committed Sep 20, 2024
commit 7b48a7addbf92e6820ee992d92224d221b3f7f68
43 changes: 22 additions & 21 deletions apps/webapp/app/db.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { PrismaClient, Prisma } from "@trigger.dev/database";
import { withOptimize } from "@prisma/extension-optimize";
import { Prisma, PrismaClient } from "@trigger.dev/database";
import invariant from "tiny-invariant";
import { z } from "zod";
import { logger } from "./services/logger.server";
import { env } from "./env.server";
import { singleton } from "./utils/singleton";
import { logger } from "./services/logger.server";
import { isValidDatabaseUrl } from "./utils/db";
import { singleton } from "./utils/singleton";

export type PrismaTransactionClient = Omit<
PrismaClient,
Expand Down Expand Up @@ -94,6 +95,7 @@ function getClient() {
url: databaseUrl.href,
},
},
// @ts-expect-error
log: [
{
emit: "stdout",
Expand All @@ -107,25 +109,16 @@ function getClient() {
emit: "stdout",
level: "warn",
},
// {
// emit: "stdout",
// level: "query",
// },
// {
// emit: "event",
// level: "query",
// },
],
].concat(
process.env.VERBOSE_PRISMA_LOGS === "1"
? [
{ emit: "event", level: "query" },
{ emit: "stdout", level: "query" },
]
: []
),
});

// client.$on("query", (e) => {
// console.log(`Query tooks ${e.duration}ms`, {
// query: e.query,
// params: e.params,
// duration: e.duration,
// });
// });

// connect eagerly
client.$connect();

Expand Down Expand Up @@ -153,6 +146,7 @@ function getReplicaClient() {
url: replicaUrl.href,
},
},
// @ts-expect-error
log: [
{
emit: "stdout",
Expand All @@ -166,7 +160,14 @@ function getReplicaClient() {
emit: "stdout",
level: "warn",
},
],
].concat(
process.env.VERBOSE_PRISMA_LOGS === "1"
? [
{ emit: "event", level: "query" },
{ emit: "stdout", level: "query" },
]
: []
),
});

// connect eagerly
Expand Down
185 changes: 184 additions & 1 deletion apps/webapp/app/presenters/v3/ApiRetrieveRunPresenter.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export class ApiRetrieveRunPresenter extends BasePresenter {
showSecretDetails: boolean
): Promise<RetrieveRunResponse | undefined> {
return this.traceWithEnv("call", env, async (span) => {
const taskRun = await this._prisma.taskRun.findUnique({
const taskRun = await this._replica.taskRun.findFirst({
where: {
friendlyId,
runtimeEnvironmentId: env.id,
Expand All @@ -36,6 +36,94 @@ export class ApiRetrieveRunPresenter extends BasePresenter {
lockedToVersion: true,
schedule: true,
tags: true,
parentTaskRun: {
select: {
id: true,
friendlyId: true,
status: true,
taskIdentifier: true,
createdAt: true,
startedAt: true,
updatedAt: true,
completedAt: true,
expiredAt: true,
delayUntil: true,
ttl: true,
tags: true,
costInCents: true,
baseCostInCents: true,
usageDurationMs: true,
idempotencyKey: true,
isTest: true,
depth: true,
lockedToVersion: {
select: {
version: true,
},
},
},
},
rootTaskRun: {
select: {
id: true,
friendlyId: true,
status: true,
taskIdentifier: true,
createdAt: true,
startedAt: true,
updatedAt: true,
completedAt: true,
expiredAt: true,
delayUntil: true,
ttl: true,
tags: true,
costInCents: true,
baseCostInCents: true,
usageDurationMs: true,
idempotencyKey: true,
isTest: true,
depth: true,
lockedToVersion: {
select: {
version: true,
},
},
},
},
childRuns: {
select: {
id: true,
taskIdentifier: true,
friendlyId: true,
resumeParentOnCompletion: true,
status: true,
createdAt: true,
startedAt: true,
updatedAt: true,
completedAt: true,
expiredAt: true,
delayUntil: true,
ttl: true,
tags: true,
costInCents: true,
baseCostInCents: true,
usageDurationMs: true,
idempotencyKey: true,
isTest: true,
depth: true,
lockedToVersion: {
select: {
version: true,
},
},
batch: {
select: {
id: true,
friendlyId: true,
},
},
},
},
},
});

Expand Down Expand Up @@ -124,6 +212,7 @@ export class ApiRetrieveRunPresenter extends BasePresenter {
costInCents: taskRun.costInCents,
baseCostInCents: taskRun.baseCostInCents,
durationMs: taskRun.usageDurationMs,
depth: taskRun.depth,
schedule: taskRun.schedule
? {
id: taskRun.schedule.friendlyId,
Expand All @@ -150,6 +239,94 @@ export class ApiRetrieveRunPresenter extends BasePresenter {
completedAt: a.completedAt ?? undefined,
error: ApiRetrieveRunPresenter.apiErrorFromError(a.error),
})),
relatedRuns: {
root: taskRun.rootTaskRun
? {
id: taskRun.rootTaskRun.friendlyId,
taskIdentifier: taskRun.rootTaskRun.taskIdentifier,
idempotencyKey: taskRun.rootTaskRun.idempotencyKey ?? undefined,
version: taskRun.rootTaskRun.lockedToVersion?.version,
status: ApiRetrieveRunPresenter.apiStatusFromRunStatus(taskRun.rootTaskRun.status),
createdAt: taskRun.rootTaskRun.createdAt,
startedAt: taskRun.rootTaskRun.startedAt ?? undefined,
updatedAt: taskRun.rootTaskRun.updatedAt,
finishedAt: taskRun.rootTaskRun.completedAt ?? undefined,
expiredAt: taskRun.rootTaskRun.expiredAt ?? undefined,
delayedUntil: taskRun.rootTaskRun.delayUntil ?? undefined,
ttl: taskRun.rootTaskRun.ttl ?? undefined,
costInCents: taskRun.rootTaskRun.costInCents,
baseCostInCents: taskRun.rootTaskRun.baseCostInCents,
durationMs: taskRun.rootTaskRun.usageDurationMs,
isTest: taskRun.rootTaskRun.isTest,
depth: taskRun.rootTaskRun.depth,
tags: taskRun.rootTaskRun.tags
.map((t) => t.name)
.sort((a, b) => a.localeCompare(b)),
...ApiRetrieveRunPresenter.apiBooleanHelpersFromTaskRunStatus(
taskRun.rootTaskRun.status
),
}
: undefined,
parent: taskRun.parentTaskRun
? {
id: taskRun.parentTaskRun.friendlyId,
taskIdentifier: taskRun.parentTaskRun.taskIdentifier,
idempotencyKey: taskRun.parentTaskRun.idempotencyKey ?? undefined,
version: taskRun.parentTaskRun.lockedToVersion?.version,
status: ApiRetrieveRunPresenter.apiStatusFromRunStatus(
taskRun.parentTaskRun.status
),
createdAt: taskRun.parentTaskRun.createdAt,
startedAt: taskRun.parentTaskRun.startedAt ?? undefined,
updatedAt: taskRun.parentTaskRun.updatedAt,
finishedAt: taskRun.parentTaskRun.completedAt ?? undefined,
expiredAt: taskRun.parentTaskRun.expiredAt ?? undefined,
delayedUntil: taskRun.parentTaskRun.delayUntil ?? undefined,
isRoot: taskRun.parentTaskRun.id === taskRun.rootTaskRun?.id,
Copy link
Contributor

Choose a reason for hiding this comment

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

Safely compare IDs in isRoot property

The expression taskRun.parentTaskRun.id === taskRun.rootTaskRun?.id may cause issues if taskRun.rootTaskRun is undefined. Consider adding a null check to prevent potential runtime errors.

Apply this change for a safer comparison:

- isRoot: taskRun.parentTaskRun.id === taskRun.rootTaskRun?.id,
+ isRoot: taskRun.rootTaskRun
+   ? taskRun.parentTaskRun.id === taskRun.rootTaskRun.id
+   : false,
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
isRoot: taskRun.parentTaskRun.id === taskRun.rootTaskRun?.id,
isRoot: taskRun.rootTaskRun
? taskRun.parentTaskRun.id === taskRun.rootTaskRun.id
: false,

ttl: taskRun.parentTaskRun.ttl ?? undefined,
costInCents: taskRun.parentTaskRun.costInCents,
baseCostInCents: taskRun.parentTaskRun.baseCostInCents,
durationMs: taskRun.parentTaskRun.usageDurationMs,
isTest: taskRun.parentTaskRun.isTest,
depth: taskRun.parentTaskRun.depth,
tags: taskRun.parentTaskRun.tags
.map((t) => t.name)
.sort((a, b) => a.localeCompare(b)),
...ApiRetrieveRunPresenter.apiBooleanHelpersFromTaskRunStatus(
taskRun.parentTaskRun.status
),
}
: undefined,
children: taskRun.childRuns.map((r) => ({
id: r.friendlyId,
taskIdentifier: r.taskIdentifier,
idempotencyKey: r.idempotencyKey ?? undefined,
version: r.lockedToVersion?.version,
triggerFunction: r.batch
? r.resumeParentOnCompletion
? "batchTriggerAndWait"
: "batchTrigger"
: r.resumeParentOnCompletion
? "triggerAndWait"
: "trigger",
batchId: r.batch?.friendlyId,
status: ApiRetrieveRunPresenter.apiStatusFromRunStatus(r.status),
createdAt: r.createdAt,
startedAt: r.startedAt ?? undefined,
updatedAt: r.updatedAt,
finishedAt: r.completedAt ?? undefined,
expiredAt: r.expiredAt ?? undefined,
delayedUntil: r.delayUntil ?? undefined,
ttl: r.ttl ?? undefined,
tags: r.tags.map((t) => t.name).sort((a, b) => a.localeCompare(b)),
costInCents: r.costInCents,
baseCostInCents: r.baseCostInCents,
durationMs: r.usageDurationMs,
depth: r.depth,
isTest: r.isTest,
...ApiRetrieveRunPresenter.apiBooleanHelpersFromTaskRunStatus(r.status),
})),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor duplicate mapping logic in relatedRuns

The mapping logic for root, parent, and children in relatedRuns contains duplicate code. Refactoring this code into a reusable function will improve maintainability and reduce the likelihood of inconsistencies.

Introduce a helper function to map task run data:

+ function mapTaskRunData(taskRun) {
+   return {
+     id: taskRun.friendlyId,
+     taskIdentifier: taskRun.taskIdentifier,
+     idempotencyKey: taskRun.idempotencyKey ?? undefined,
+     version: taskRun.lockedToVersion?.version,
+     status: ApiRetrieveRunPresenter.apiStatusFromRunStatus(taskRun.status),
+     createdAt: taskRun.createdAt,
+     startedAt: taskRun.startedAt ?? undefined,
+     updatedAt: taskRun.updatedAt,
+     finishedAt: taskRun.completedAt ?? undefined,
+     expiredAt: taskRun.expiredAt ?? undefined,
+     delayedUntil: taskRun.delayUntil ?? undefined,
+     ttl: taskRun.ttl ?? undefined,
+     costInCents: taskRun.costInCents,
+     baseCostInCents: taskRun.baseCostInCents,
+     durationMs: taskRun.usageDurationMs,
+     isTest: taskRun.isTest,
+     depth: taskRun.depth,
+     tags: taskRun.tags.map((t) => t.name).sort((a, b) => a.localeCompare(b)),
+     ...ApiRetrieveRunPresenter.apiBooleanHelpersFromTaskRunStatus(taskRun.status),
+   };
+ }

Update relatedRuns to use the helper function:

- root: taskRun.rootTaskRun
-   ? { /* mapping fields */ }
-   : undefined,
+ root: taskRun.rootTaskRun ? mapTaskRunData(taskRun.rootTaskRun) : undefined,

- parent: taskRun.parentTaskRun
-   ? { /* mapping fields */ }
-   : undefined,
+ parent: taskRun.parentTaskRun
+   ? {
+       ...mapTaskRunData(taskRun.parentTaskRun),
+       isRoot: taskRun.parentTaskRun.id === taskRun.rootTaskRun?.id,
+     }
+   : undefined,

- children: taskRun.childRuns.map((r) => ({
-   /* mapping fields */
- })),
+ children: taskRun.childRuns.map((r) => mapTaskRunData(r)),

This refactor reduces code duplication and makes future maintenance easier.

Committable suggestion was skipped due to low confidence.

};
});
}
Expand Down Expand Up @@ -225,6 +402,12 @@ export class ApiRetrieveRunPresenter extends BasePresenter {
}
}

static apiBooleanHelpersFromTaskRunStatus(status: TaskRunStatus) {
return ApiRetrieveRunPresenter.apiBooleanHelpersFromRunStatus(
ApiRetrieveRunPresenter.apiStatusFromRunStatus(status)
);
}

static apiBooleanHelpersFromRunStatus(status: RunStatus) {
const isQueued = status === "QUEUED" || status === "WAITING_FOR_DEPLOY" || status === "DELAYED";
const isExecuting = status === "EXECUTING" || status === "REATTEMPTING" || status === "FROZEN";
Expand Down
13 changes: 12 additions & 1 deletion apps/webapp/app/v3/services/triggerTask.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ export class TriggerTaskService extends BaseService {
status: true,
taskIdentifier: true,
rootTaskRunId: true,
depth: true,
},
},
},
Expand Down Expand Up @@ -145,6 +146,7 @@ export class TriggerTaskService extends BaseService {
status: true,
taskIdentifier: true,
rootTaskRunId: true,
depth: true,
},
},
},
Expand All @@ -163,6 +165,7 @@ export class TriggerTaskService extends BaseService {
status: true,
taskIdentifier: true,
rootTaskRunId: true,
depth: true,
},
},
},
Expand Down Expand Up @@ -281,6 +284,14 @@ export class TriggerTaskService extends BaseService {
}
}

const depth = dependentAttempt
? dependentAttempt.taskRun.depth + 1
: parentAttempt
? parentAttempt.taskRun.depth + 1
: dependentBatchRun?.dependentTaskAttempt
? dependentBatchRun.dependentTaskAttempt.taskRun.depth + 1
: 0;

Comment on lines +287 to +294
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider including parentBatchRun in the depth calculation

The depth calculation currently doesn't account for parentBatchRun. If parentBatchRun is part of the task hierarchy, include it to accurately compute the depth.

Suggested update:

                  const depth = dependentAttempt
                    ? dependentAttempt.taskRun.depth + 1
                    : parentAttempt
                    ? parentAttempt.taskRun.depth + 1
                    : dependentBatchRun?.dependentTaskAttempt
                    ? dependentBatchRun.dependentTaskAttempt.taskRun.depth + 1
+                   : parentBatchRun?.dependentTaskAttempt?.taskRun.depth + 1
                    : 0;
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const depth = dependentAttempt
? dependentAttempt.taskRun.depth + 1
: parentAttempt
? parentAttempt.taskRun.depth + 1
: dependentBatchRun?.dependentTaskAttempt
? dependentBatchRun.dependentTaskAttempt.taskRun.depth + 1
: 0;
const depth = dependentAttempt
? dependentAttempt.taskRun.depth + 1
: parentAttempt
? parentAttempt.taskRun.depth + 1
: dependentBatchRun?.dependentTaskAttempt
? dependentBatchRun.dependentTaskAttempt.taskRun.depth + 1
: parentBatchRun?.dependentTaskAttempt?.taskRun.depth + 1
: 0;

const taskRun = await tx.taskRun.create({
data: {
status: delayUntil ? "DELAYED" : "PENDING",
Expand Down Expand Up @@ -325,9 +336,9 @@ export class TriggerTaskService extends BaseService {
parentAttempt?.taskRun.id ??
dependentBatchRun?.dependentTaskAttempt?.taskRun.rootTaskRunId ??
dependentBatchRun?.dependentTaskAttempt?.taskRun.id,

batchId: dependentBatchRun?.id ?? parentBatchRun?.id,
resumeParentOnCompletion: !!(dependentAttempt ?? dependentBatchRun),
depth,
},
});

Expand Down
2 changes: 1 addition & 1 deletion apps/webapp/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,4 +247,4 @@
"engines": {
"node": ">=16.0.0"
}
}
}
Loading