Associate child runs with the parent span ID #1352
Conversation
🦋 Changeset detectedLatest commit: a4c9a3f The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe changes introduce enhancements to tracing capabilities across various components of the application. Key modifications include the addition of OpenTelemetry propagation headers, updates to function signatures to accommodate tracing information, and the introduction of new fields and indices in the database schema to better track task run hierarchies. These changes collectively aim to improve the telemetry data representation and facilitate better tracking of parent-child relationships in task executions. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (7)
.changeset/short-tomatoes-beam.md (1)
1-5: LGTM! Changeset file correctly documents the patch update.The changeset file accurately describes the patch update to the
@trigger.dev/corepackage. The change aligns well with the PR objectives, specifically addressing the association of child runs with the parent span ID.Consider expanding the description to include:
- The expected impact of this change on system behavior or performance.
- Any potential side effects or considerations for users of the package.
This additional information could provide more context for future reference and for other developers reviewing the changes.packages/database/README.md (2)
5-18: LGTM: Comprehensive instructions for adding a new index.The step-by-step guide effectively outlines the process for safely adding a new index to a large table. The emphasis on using CONCURRENTLY and IF NOT EXISTS is crucial for preventing issues during deployment.
Consider adding a brief explanation of why CONCURRENTLY is important (e.g., "CONCURRENTLY allows the index to be built without locking out writes"). This would provide additional context for developers who might be unfamiliar with the concept.
27-38: LGTM: Valuable SQL commands for index management.The provided SQL commands are extremely useful for managing and monitoring the index creation process. The explanation at the end effectively reinforces the importance of following this process to avoid deployment issues.
Consider adding a brief comment about the expected output or interpretation of the results for each SQL command. This would help developers understand what to look for when running these commands.
packages/cli-v3/src/config.ts (1)
231-231: Approve changes to warning messages with a minor suggestion.The updates to the warning messages for deprecated options (
additionalFiles,additionalPackages, andresolveEnvVars) are good improvements. The new URLs point to specific sections in the configuration file documentation, which will be more helpful for users.Consider adding the word "instead" to the end of each message to emphasize the recommended alternative. For example:
- `The "additionalFiles" option is deprecated and will be removed. Use the "additionalFiles" build extension instead. See https://trigger.dev/docs/config/config-file#additionalfiles for more information.` + `The "additionalFiles" option is deprecated and will be removed. Use the "additionalFiles" build extension instead. See https://trigger.dev/docs/config/config-file#additionalfiles for more information instead.`This small change would reinforce the idea of replacing the deprecated option with the recommended alternative.
Also applies to: 242-242, 278-278
apps/webapp/app/v3/eventRepository.server.ts (2)
838-839: LGTM! Consider using a TypeScript interface fortraceparent.The addition of the
traceparentparameter to thecallbackfunction is a good improvement that aligns with the PR objective of associating child runs with the parent span ID. This change enhances the tracing capabilities of the system.For improved type safety and code readability, consider defining a TypeScript interface for the
traceparentobject. Here's a suggested implementation:interface TraceParent { traceId: string; spanId: string; } // Then update the method signature: callback: ( e: EventBuilder, traceContext: Record<string, string | undefined>, traceparent?: TraceParent ) => Promise<TResult>This change would make the code more self-documenting and provide better type checking.
896-896: LGTM! Consider adding a null check forpropagatedContext.The addition of
propagatedContext?.traceparentas an argument to the callback function is a good change. It ensures that the tracing context is properly propagated, which is essential for maintaining the tracing chain across asynchronous operations.To improve robustness, consider adding a null check for
propagatedContextbefore accessing itstraceparentproperty. This can prevent potential runtime errors ifpropagatedContextis unexpectedlynullorundefined. Here's a suggested implementation:const result = await callback( eventBuilder, traceContext, propagatedContext && propagatedContext.traceparent );This change ensures that
traceparentis only passed ifpropagatedContextexists, providing an additional layer of safety.packages/database/prisma/schema.prisma (1)
1751-1752: Approved: New field added to associate child runs with parent span ID.The addition of the
parentSpanIdfield to theTaskRunmodel is a good implementation of the PR objectives. This will enable better tracking of task run hierarchies and improve the user experience when viewing triggered runs.A minor suggestion for improvement:
Consider adding an index to this field if you anticipate frequent queries based on theparentSpanId. This could improve query performance when fetching child runs for a given parent span./// The span ID of the "trigger" span in the parent task run parentSpanId String? + @@index([parentSpanId])
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- .changeset/short-tomatoes-beam.md (1 hunks)
- apps/webapp/app/v3/eventRepository.server.ts (2 hunks)
- apps/webapp/app/v3/services/triggerTask.server.ts (2 hunks)
- packages/cli-v3/src/config.ts (3 hunks)
- packages/core/src/v3/apiClient/core.ts (3 hunks)
- packages/core/src/v3/apiClient/index.ts (0 hunks)
- packages/database/README.md (1 hunks)
- packages/database/prisma/migrations/20240924125845_add_root_task_run_id_index/migration.sql (1 hunks)
- packages/database/prisma/migrations/20240924130558_add_parent_span_id_to_task_run/migration.sql (1 hunks)
- packages/database/prisma/schema.prisma (1 hunks)
Files not reviewed due to no reviewable changes (1)
- packages/core/src/v3/apiClient/index.ts
Additional comments not posted (10)
packages/database/prisma/migrations/20240924130558_add_parent_span_id_to_task_run/migration.sql (1)
1-2: LGTM! Consider additional optimizations.The addition of the "parentSpanId" column to the "TaskRun" table aligns with the PR objective of associating child runs with their parent span ID. The TEXT type is appropriate for storing span IDs.
Consider the following suggestions:
- Add the index creation for "rootTaskRunId" as mentioned in the PR objectives:
CREATE INDEX CONCURRENTLY IF NOT EXISTS "TaskRun_rootTaskRunId_idx" ON "TaskRun"("rootTaskRunId");
- Evaluate if an index on the new "parentSpanId" column would be beneficial for query performance:
CREATE INDEX CONCURRENTLY IF NOT EXISTS "TaskRun_parentSpanId_idx" ON "TaskRun"("parentSpanId");
- If appropriate, consider adding a foreign key constraint to ensure referential integrity:
ALTER TABLE "TaskRun" ADD CONSTRAINT "fk_TaskRun_parentSpanId" FOREIGN KEY ("parentSpanId") REFERENCES "TaskRun"("spanId");Note: This assumes there's a "spanId" column in the "TaskRun" table. Adjust the reference as needed.
To ensure the "TaskRun" table structure and verify the existence of the "spanId" column, run:
Verification successful
The index for "rootTaskRunId" already exists.
While the
parentSpanIdcolumn has been successfully added to the "TaskRun" table, consider adding an index and a foreign key constraint to this new column to enhance query performance and ensure referential integrity.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the structure of the TaskRun table # Expected result: Display the table structure, including the new parentSpanId column and any existing spanId column # Search for the TaskRun table definition rg --type sql -A 20 'CREATE TABLE.*TaskRun' # Search for any existing index on rootTaskRunId rg --type sql 'CREATE INDEX.*TaskRun.*rootTaskRunId' # Search for any existing foreign key constraints on TaskRun rg --type sql 'FOREIGN KEY.*TaskRun'Length of output: 27778
packages/database/prisma/migrations/20240924125845_add_root_task_run_id_index/migration.sql (1)
1-2: Excellent implementation of the index creation!The SQL statement for creating the index is well-structured and aligns perfectly with the PR objectives. Here are the key strengths of this implementation:
- Use of
CREATE INDEX CONCURRENTLY: This minimizes the impact on the production environment by allowing writes to the table during index creation.IF NOT EXISTSclause: Ensures idempotency, preventing errors if the migration is run multiple times.- Descriptive index name: "TaskRun_rootTaskRunId_idx" clearly indicates its purpose and follows good naming conventions.
- Exact match with PR objectives: The SQL command in the migration file is identical to the one recommended in the PR description, ensuring consistency.
This index will significantly improve query performance for operations involving the
rootTaskRunId, which is crucial for the new feature of associating child runs with their parent span ID.packages/database/README.md (1)
1-3: LGTM: Clear and concise package introduction.The introduction effectively describes the purpose of the @trigger.dev/database package and its main functionality.
packages/cli-v3/src/config.ts (1)
Line range hint
1-324: Verify the relevance of this file to the PR objectives.While the changes in this file improve the documentation references for deprecated options, they don't seem to directly address the main PR objective of associating child runs with parent span IDs or the database optimization mentioned in the PR summary.
Could you clarify how the changes in this file contribute to the overall PR objectives? If they're not directly related, consider explaining the rationale for including these changes in this PR.
To help verify the relevance, you can run the following script to check for any mentions of span IDs or database indexing in the entire codebase:
This will help determine if the main PR objectives are implemented in other files.
Verification successful
The
packages/cli-v3/src/config.tsfile is not directly related to the PR objectives concerning span IDs or database indexing.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for mentions of span IDs and database indexing in the codebase echo "Searching for mentions of span IDs:" rg --type-add 'code:*.{ts,js,sql}' --type code 'span.*ID' echo "\nSearching for mentions of database indexing:" rg --type-add 'code:*.{ts,js,sql}' --type code '(CREATE INDEX|ADD INDEX)'Length of output: 7364
apps/webapp/app/v3/eventRepository.server.ts (1)
Line range hint
1-1293: Overall, excellent changes that enhance tracing capabilities!The modifications to the
EventRepositoryclass, particularly in thetraceEventmethod, successfully implement the PR objective of associating child runs with the parent span ID. The changes are well-structured and appropriately integrated into the existing codebase.Key improvements:
- Addition of the
traceparentparameter to thecallbackfunction signature.- Proper propagation of the trace context to the callback function.
These changes will significantly enhance the system's ability to track and manage task runs, as outlined in the PR objectives.
While the implementation is solid, consider the minor suggestions in the previous comments for further refinement:
- Using a TypeScript interface for the
traceparentobject.- Adding a null check when passing
propagatedContext?.traceparentto the callback function.These small improvements would further enhance type safety and robustness of the code.
packages/core/src/v3/apiClient/core.ts (3)
7-7: Approved: Importing necessary OpenTelemetry modulesThe import statement correctly includes
propagationfrom@opentelemetry/api, which is needed for context propagation.
190-190: Approved: Injecting propagation headers into request initializationThe call to
injectPropagationHeadersIfInWorker($requestInit)appropriately injects the propagation headers when thex-trigger-workerheader is present, ensuring correct context propagation in worker-triggered requests.
586-601: Verify usage ofx-trigger-workerheader across the codebaseEnsure that the
x-trigger-workerheader is set correctly in all requests that should trigger worker context propagation. This verification confirms thatinjectPropagationHeadersIfInWorkerfunctions as intended.Run the following script to locate all instances where the
x-trigger-workerheader is set to"true":This script searches JavaScript and TypeScript files for instances where the
x-trigger-workerheader is explicitly set to"true".apps/webapp/app/v3/services/triggerTask.server.ts (2)
243-243: Addition oftraceparentparameter enhances tracing capabilitiesThe inclusion of the
traceparentparameter in the function signature allows for improved association of child runs with their parent span ID, aligning with the PR objectives to enhance tracing functionality.
310-311: Correct assignment ofparentSpanIdbased onparentAsLinkTypeThe logic correctly assigns
parentSpanIdby checking ifoptions.parentAsLinkTypeis"replay". If it is,parentSpanIdis set toundefined; otherwise, it usestraceparent?.spanId. This ensures that the child runs are accurately associated with the appropriate parent span.
| ## @trigger.dev/database | ||
|
|
||
| This is the internal database package for the Trigger.dev project. It exports a generated prisma client that can be instantiated with a connection string. | ||
|
|
||
| ### How to add a new index on a large table | ||
|
|
||
| 1. Modify the Prisma.schema with a single index change (no other changes, just one index at a time) | ||
| 2. Create a Prisma migration using `cd packages/database && pnpm run db:migrate:dev --create-only` | ||
| 3. Modify the SQL file: add IF NOT EXISTS to it and CONCURRENTLY: | ||
|
|
||
| ```sql | ||
| CREATE INDEX CONCURRENTLY IF NOT EXISTS "JobRun_eventId_idx" ON "JobRun" ("eventId"); | ||
| ``` | ||
|
|
||
| 4. Don’t apply the Prisma migration locally yet. This is a good opportunity to test the flow. | ||
| 5. Manually apply the index to your database, by running the index command. | ||
| 6. Then locally run `pnpm run db:migrate:deploy` | ||
|
|
||
| #### Before deploying | ||
|
|
||
| Run the index creation before deploying | ||
|
|
||
| ```sql | ||
| CREATE INDEX CONCURRENTLY IF NOT EXISTS "JobRun_eventId_idx" ON "JobRun" ("eventId"); | ||
| ``` | ||
|
|
||
| These commands are useful: | ||
|
|
||
| ```sql | ||
| -- creates an index safely, this can take a long time (2 mins maybe) | ||
| CREATE INDEX CONCURRENTLY IF NOT EXISTS "JobRun_eventId_idx" ON "JobRun" ("eventId"); | ||
| -- checks the status of an index | ||
| SELECT * FROM pg_stat_progress_create_index WHERE relid = '"JobRun"'::regclass; | ||
| -- checks if the index is there | ||
| SELECT * FROM pg_indexes WHERE tablename = 'JobRun' AND indexname = 'JobRun_eventId_idx'; | ||
| ``` | ||
|
|
||
| Now, when you deploy and prisma runs the migration, it will skip the index creation because it already exists. If you don't do this, there will be pain. |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency in index naming found in documentation and migrations.
The verification script revealed multiple instances of the unexpected index name "JobRun_eventId_idx" in the following files:
packages/database/README.mdpackages/database/prisma/migrations/20240916155127_added_job_run_event_id_index/migration.sql
To maintain consistency with the PR objectives, please update these occurrences to "TaskRun_rootTaskRunId_idx".
Analysis chain
Overall, excellent documentation for database index management.
This README provides comprehensive and well-structured guidance for managing database indexes in the @trigger.dev/database package. The instructions are clear, follow best practices, and align well with the PR objectives for database optimization.
To ensure consistency throughout the documentation and codebase, please run the following script to verify the correct index name usage:
After running this script, ensure that all occurrences of "JobRun_eventId_idx" are replaced with "TaskRun_rootTaskRunId_idx" to maintain consistency with the PR objectives.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the correct index name is used consistently.
# Test: Search for both index names. Expect: Only occurrences of "TaskRun_rootTaskRunId_idx".
echo "Searching for 'TaskRun_rootTaskRunId_idx' (expected):"
rg --type sql --type md "TaskRun_rootTaskRunId_idx"
echo "\nSearching for 'JobRun_eventId_idx' (unexpected):"
rg --type sql --type md "JobRun_eventId_idx"
Length of output: 1167
| function injectPropagationHeadersIfInWorker(requestInit?: RequestInit): RequestInit | undefined { | ||
| const headers = new Headers(requestInit?.headers); | ||
|
|
||
| if (headers.get("x-trigger-worker") !== "true") { | ||
| return requestInit; | ||
| } | ||
|
|
||
| const headersObject = Object.fromEntries(headers.entries()); | ||
|
|
||
| propagation.inject(context.active(), headersObject); | ||
|
|
||
| return { | ||
| ...requestInit, | ||
| headers: new Headers(headersObject), | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify context propagation by injecting directly into Headers
Instead of converting Headers to an object and back, you can inject directly into the Headers instance. This simplifies the code and avoids potential issues with header case sensitivity.
Apply this diff to inject directly into the Headers object:
function injectPropagationHeadersIfInWorker(requestInit?: RequestInit): RequestInit | undefined {
const headers = new Headers(requestInit?.headers);
if (headers.get("x-trigger-worker") !== "true") {
return requestInit;
}
- const headersObject = Object.fromEntries(headers.entries());
- propagation.inject(context.active(), headersObject);
+ propagation.inject(context.active(), headers);
return {
...requestInit,
- headers: new Headers(headersObject),
+ headers,
};
}By injecting directly into headers, you leverage the built-in behavior of propagation.inject when working with Headers objects.
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.
| function injectPropagationHeadersIfInWorker(requestInit?: RequestInit): RequestInit | undefined { | |
| const headers = new Headers(requestInit?.headers); | |
| if (headers.get("x-trigger-worker") !== "true") { | |
| return requestInit; | |
| } | |
| const headersObject = Object.fromEntries(headers.entries()); | |
| propagation.inject(context.active(), headersObject); | |
| return { | |
| ...requestInit, | |
| headers: new Headers(headersObject), | |
| }; | |
| } | |
| function injectPropagationHeadersIfInWorker(requestInit?: RequestInit): RequestInit | undefined { | |
| const headers = new Headers(requestInit?.headers); | |
| if (headers.get("x-trigger-worker") !== "true") { | |
| return requestInit; | |
| } | |
| propagation.inject(context.active(), headers); | |
| return { | |
| ...requestInit, | |
| headers, | |
| }; | |
| } |
This is needed so we can show which runs were triggered when selecting the "triggering" span in the parent run.
This PR also adds an index to the
rootTaskRunId, and the followingCREATE INDEXsql should be run before deploying:Note
Before this can be used, an INDEX will need to be added to the
parentSpanIdcolumn on TaskRun, but that must be done in a separate PR/deploySummary by CodeRabbit
New Features
traceparentparameter in event handling to improve context for tracing.parentSpanIdfield in the TaskRun model for better task tracking.Bug Fixes
Documentation
@trigger.dev/databasepackage outlining usage and migration steps.