Conversation
…ecution record stores Implemented a method to retrieve the execution chain of activity records with optional cross-workflow traversal and pagination capabilities. Expanded filtering and mapping functionality to support new attributes.
Introduced properties for metadata, fault aggregation, scheduling, and call stack depth to enhance activity execution tracking and analysis.
Refactored constructors across multiple activity providers to return the full result object from `CreateActivity` method, ensuring consistent object handling.
Added a new FluentMigrator migration to include extra fields in the `ActivityExecutionRecords` table, enabling enhanced metadata, scheduling, and call stack depth tracking.
Added cycle detection using a `HashSet` to prevent infinite loops when traversing activity execution chains in multiple storage implementations. Updated unit tests to validate correct handling of circular references and chain traversal.
Centralized the `GetExecutionChainAsync` method into an extension class to streamline and unify its implementation across stores. Removed redundant implementations from individual stores and updated interfaces to utilize the new extension method. This reduces code duplication and simplifies future maintenance.
There was a problem hiding this comment.
Pull request overview
This PR adds support for tracking the activity call stack during workflow execution to enable better debugging and understanding of workflow execution flow.
Changes:
- Introduces new database fields (
SchedulingActivityExecutionId,SchedulingActivityId,SchedulingWorkflowInstanceId,CallStackDepth,SerializedMetadata,AggregateFaultCount) to track activity scheduling relationships and call stack depth - Implements
GetExecutionChainAsyncmethod in both MongoDB and Dapper persistence stores to retrieve the execution chain with infinite loop protection using a visited set - Updates activity providers to handle the new return type from
CreateActivity, which now returns a result object containing the activity and additional metadata
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/modules/persistence/Elsa.Persistence.Dapper.Migrations/Runtime/V3_7.cs | Adds database migration to create new columns for activity execution tracking |
| src/modules/persistence/Elsa.Persistence.Dapper/Modules/Runtime/Records/ActivityExecutionRecord.cs | Adds new properties for scheduling context, metadata, fault count, and call stack depth |
| src/modules/persistence/Elsa.Persistence.Dapper/Modules/Runtime/Records/ActivityExecutionSummaryRecord.cs | Adds metadata and aggregate fault count to summary record |
| src/modules/persistence/Elsa.Persistence.Dapper/Modules/Runtime/Stores/DapperActivityExecutionRecordStore.cs | Implements GetExecutionChainAsync, updates filter criteria, and adds mapping for new fields |
| src/modules/persistence/Elsa.Persistence.MongoDb/Modules/Runtime/ActivityExecutionLogStore.cs | Implements GetExecutionChainAsync with pagination and cross-workflow chain traversal support |
| src/modules/servicebus/Elsa.ServiceBus.MassTransit/Services/MassTransitActivityTypeProvider.cs | Updates activity constructors to handle new CreateActivity return type |
| src/modules/http/Elsa.Http.Webhooks/ActivityProviders/WebhookEventActivityProvider.cs | Updates activity constructor to handle new CreateActivity return type |
| src/modules/cms/Elsa.OrchardCore/ActivityProviders/OrchardContentItemsEventActivityProvider.cs | Updates activity constructor to handle new CreateActivity return type |
| src/modules/agents/Elsa.Agents.Activities/ActivityProviders/AgentActivityProvider.cs | Updates activity constructor to handle new CreateActivity return type |
Comments suppressed due to low confidence (1)
src/modules/persistence/Elsa.Persistence.MongoDb/Modules/Runtime/ActivityExecutionLogStore.cs:102
- The logic for stopping at workflow boundaries appears to be incorrect. The current condition stops traversal whenever SchedulingWorkflowInstanceId is not null, even if it's the same workflow instance. This would incorrectly stop the traversal within the same workflow when one activity schedules another. The condition should compare SchedulingWorkflowInstanceId with WorkflowInstanceId to detect actual cross-workflow boundaries. Consider changing the condition to: if (!includeCrossWorkflowChain && record.SchedulingWorkflowInstanceId != null && record.SchedulingWorkflowInstanceId != record.WorkflowInstanceId)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public override void Up() | ||
| { | ||
| Alter.Table("ActivityExecutionRecords").AddColumn("SerializedMetadata").AsString(MaxValue).Nullable(); | ||
| Alter.Table("ActivityExecutionRecords").AddColumn("SchedulingActivityExecutionId").AsString().Nullable(); | ||
| Alter.Table("ActivityExecutionRecords").AddColumn("SchedulingActivityId").AsString().Nullable(); | ||
| Alter.Table("ActivityExecutionRecords").AddColumn("SchedulingWorkflowInstanceId").AsString().Nullable(); | ||
| Alter.Table("ActivityExecutionRecords").AddColumn("CallStackDepth").AsInt32().Nullable(); | ||
| } |
There was a problem hiding this comment.
The migration is missing the AggregateFaultCount column. According to the changes in ActivityExecutionRecordRecord.cs, there is an AggregateFaultCount field being added to the entity (lines 88-89), and this field is being mapped in the store implementation (lines 189 and 219 in DapperActivityExecutionRecordStore.cs). However, the migration Up() method does not add this column to the database table. The Down() method should also include removal of this column for consistency.
|
|
||
| if (filter.Completed != null) | ||
| { |
There was a problem hiding this comment.
The logic for stopping at workflow boundaries appears to be incorrect. The current condition stops traversal whenever SchedulingWorkflowInstanceId is not null, even if it's the same workflow instance. This would incorrectly stop the traversal within the same workflow when one activity schedules another. The condition should compare SchedulingWorkflowInstanceId with WorkflowInstanceId to detect actual cross-workflow boundaries. Consider changing the condition to: if (!includeCrossWorkflowChain && mappedRecord.SchedulingWorkflowInstanceId != null && mappedRecord.SchedulingWorkflowInstanceId != mappedRecord.WorkflowInstanceId)
|
@copilot open a new pull request to apply changes based on the comments in this thread. Make sure you understand & agree with the feedback before making changes. |
|
@sfmskywalker I've opened a new pull request, #120, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: sfmskywalker <938393+sfmskywalker@users.noreply.github.com>
Add missing AggregateFaultCount column to V3_7 migration
Adds support for tracking the activity call stack during workflow execution. This allows for better debugging and understanding of workflow execution flow.
ActivityExecutionRecordandActivityExecutionSummaryRecordfor tracking scheduling activity information and call stack depth.GetExecutionChainAsyncmethod to retrieve the chain of activity executions, enabling traversal of the call stack.ActivityExecutionRecordstable.