Skip to content

BCH-1153: Populate per-step metrics in workflow execution metrics resp #399

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

Open
saltpy-cs wants to merge 4 commits into
base: main
Choose a base branch
from
+220 −10
Open

BCH-1153: Populate per-step metrics in workflow execution metrics resp #399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6b459ff
BCH-1153: Populate per-step metrics in workflow execution metrics res…
saltpy-cs May 20, 2026
f428101
BCH-1153: Address PR review comments on per-step metrics
saltpy-cs May 20, 2026
7b31c6a
BCH-1153: Address PR review comments — nil guards on step definition …
saltpy-cs May 21, 2026
2655cff
BCH-1153: Address PR review comments — fix average step duration deno…
saltpy-cs May 21, 2026
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
31 changes: 30 additions & 1 deletion docs/docs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39234,12 +39234,18 @@ const docTemplate = `{
"duration": {
"$ref": "#/definitions/time.Duration"
},
"executionID": {
"executionId": {
"type": "string"
},
"longestStepDuration": {
"$ref": "#/definitions/time.Duration"
},
"stepMetrics": {
"type": "array",
"items": {
"$ref": "#/definitions/workflow.StepMetric"
}
},
Comment thread
saltpy-cs marked this conversation as resolved.
"totalSteps": {
"type": "integer"
}
Expand Down Expand Up @@ -39292,6 +39298,29 @@ const docTemplate = `{
}
}
},
"workflow.StepMetric": {
"type": "object",
"properties": {
"completedAt": {
"type": "string",
"format": "date-time"
},
"durationMinutes": {
"type": "number"
},
"startedAt": {
"type": "string",
"format": "date-time"
},
"stepDefinitionId": {
"type": "string",
"format": "uuid"
},
"stepName": {
"type": "string"
}
}
},
"workflows.BulkReassignRoleResponse": {
"type": "object",
"properties": {
Expand Down
31 changes: 30 additions & 1 deletion docs/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39228,12 +39228,18 @@
"duration": {
"$ref": "#/definitions/time.Duration"
},
"executionID": {
"executionId": {
"type": "string"
},
"longestStepDuration": {
"$ref": "#/definitions/time.Duration"
},
"stepMetrics": {
"type": "array",
"items": {
"$ref": "#/definitions/workflow.StepMetric"
}
},
Comment thread
saltpy-cs marked this conversation as resolved.
"totalSteps": {
"type": "integer"
}
Expand Down Expand Up @@ -39286,6 +39292,29 @@
}
}
},
"workflow.StepMetric": {
"type": "object",
"properties": {
"completedAt": {
"type": "string",
"format": "date-time"
},
"durationMinutes": {
"type": "number"
},
"startedAt": {
"type": "string",
"format": "date-time"
},
"stepDefinitionId": {
"type": "string",
"format": "uuid"
},
"stepName": {
"type": "string"
}
}
},
"workflows.BulkReassignRoleResponse": {
"type": "object",
"properties": {
Expand Down
22 changes: 21 additions & 1 deletion docs/swagger.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8261,10 +8261,14 @@ definitions:
$ref: '#/definitions/time.Duration'
duration:
$ref: '#/definitions/time.Duration'
executionID:
executionId:
type: string
Comment on lines +8264 to 8265
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

executionID → executionId is a breaking API contract change.

If this endpoint is already consumed, please keep backward compatibility (or version the API) before merging; clients bound to executionID will break.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/swagger.yaml` around lines 8264 - 8265, The OpenAPI change renames the
response/property key from executionID to executionId which breaks clients;
update the swagger.yaml to preserve backward compatibility by supporting both
keys (keep the original executionID property and add executionId as an alias) or
introduce a new versioned endpoint/schema; specifically, restore the original
executionID property in the schema alongside the new executionId (or create a
new schema version) so consumers of executionID continue to work while allowing
the new executionId key to be used going forward.

Copy link
Copy Markdown
Author

@saltpy-cs saltpy-cs May 21, 2026

Choose a reason for hiding this comment

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

@gusfcarvalho is this something I need to be concerned about? AFAICT the UI is still working but I'm conscious that we don't have end to end tests automated so coverage for things like this may be lacking.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 21, 2026

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

longestStepDuration:
$ref: '#/definitions/time.Duration'
stepMetrics:
items:
$ref: '#/definitions/workflow.StepMetric'
type: array
totalSteps:
type: integer
type: object
Expand Down Expand Up @@ -8299,6 +8303,22 @@ definitions:
totalSteps:
type: integer
type: object
workflow.StepMetric:
properties:
completedAt:
format: date-time
type: string
durationMinutes:
type: number
startedAt:
format: date-time
type: string
stepDefinitionId:
format: uuid
type: string
stepName:
type: string
type: object
workflows.BulkReassignRoleResponse:
properties:
data:
Expand Down
48 changes: 41 additions & 7 deletions internal/workflow/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ func (m *Manager) GetExecutionMetrics(ctx context.Context, executionID *uuid.UUI
metrics := &ExecutionMetrics{
ExecutionID: *executionID,
TotalSteps: len(stepExecutions),
StepMetrics: []StepMetric{},
}

// Calculate execution duration
Expand All @@ -324,19 +325,42 @@ func (m *Manager) GetExecutionMetrics(ctx context.Context, executionID *uuid.UUI

// Calculate step metrics
var totalStepDuration time.Duration
var timedStepCount int
for _, step := range stepExecutions {
if step.WorkflowStepDefinitionID == nil {
continue
}

stepName := ""
if step.WorkflowStepDefinition != nil {
stepName = step.WorkflowStepDefinition.Name
}

sm := StepMetric{
StepDefinitionID: *step.WorkflowStepDefinitionID,
StepName: stepName,
StartedAt: step.StartedAt,
CompletedAt: step.CompletedAt,
}

if step.StartedAt != nil && step.CompletedAt != nil {
stepDuration := step.CompletedAt.Sub(*step.StartedAt)
totalStepDuration += stepDuration
timedStepCount++

if stepDuration > metrics.LongestStepDuration {
metrics.LongestStepDuration = stepDuration
}

d := stepDuration.Minutes()
sm.DurationMinutes = &d
}

metrics.StepMetrics = append(metrics.StepMetrics, sm)
}
Comment thread
saltpy-cs marked this conversation as resolved.

if len(stepExecutions) > 0 {
metrics.AverageStepDuration = totalStepDuration / time.Duration(len(stepExecutions))
if timedStepCount > 0 {
metrics.AverageStepDuration = totalStepDuration / time.Duration(timedStepCount)
}

return metrics, nil
Expand All @@ -360,11 +384,21 @@ type ExecutionStatus struct {
FailureReason string
}

// StepMetric holds per-step timing data for the metrics response
type StepMetric struct {
StepDefinitionID uuid.UUID `json:"stepDefinitionId" swaggertype:"string" format:"uuid"`
StepName string `json:"stepName"`
StartedAt *time.Time `json:"startedAt,omitempty" swaggertype:"string" format:"date-time"`
CompletedAt *time.Time `json:"completedAt,omitempty" swaggertype:"string" format:"date-time"`
DurationMinutes *float64 `json:"durationMinutes,omitempty"`
}

// ExecutionMetrics represents metrics for a workflow execution
type ExecutionMetrics struct {
ExecutionID uuid.UUID
TotalSteps int
Duration time.Duration
AverageStepDuration time.Duration
LongestStepDuration time.Duration
ExecutionID uuid.UUID `json:"executionId"`
TotalSteps int `json:"totalSteps"`
Duration time.Duration `json:"duration"`
AverageStepDuration time.Duration `json:"averageStepDuration"`
LongestStepDuration time.Duration `json:"longestStepDuration"`
StepMetrics []StepMetric `json:"stepMetrics"`
}
98 changes: 98 additions & 0 deletions internal/workflow/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,3 +179,101 @@ func TestManager_CancelExecution_CancelsOverdueSteps(t *testing.T) {
mockWorkflowExecService.AssertExpectations(t)
mockStepExecService.AssertExpectations(t)
}

func TestManager_GetExecutionMetrics_PopulatesStepMetrics(t *testing.T) {
ctx := context.Background()
logger := zap.NewNop().Sugar()

executionID := uuid.New()
stepDefID := uuid.New()
startedAt := time.Now().Add(-30 * time.Minute)
completedAt := time.Now()

mockClient := &MockRiverClient{}
mockWorkflowExecService := &MockWorkflowExecutionService{}
mockWorkflowInstService := &MockWorkflowInstanceService{}
mockStepExecService := &MockStepExecutionService{}

manager := NewManager(
mockClient,
mockWorkflowExecService,
mockWorkflowInstService,
mockStepExecService,
logger,
nil,
)

mockWorkflowExecService.On("GetByID", &executionID).Return(&workflows.WorkflowExecution{
UUIDModel: relational.UUIDModel{ID: &executionID},
Status: workflows.WorkflowStatusCompleted.String(),
StartedAt: &startedAt,
CompletedAt: &completedAt,
}, nil).Once()

mockStepExecService.On("GetByWorkflowExecutionID", &executionID).Return([]workflows.StepExecution{
{
WorkflowStepDefinitionID: &stepDefID,
WorkflowStepDefinition: &workflows.WorkflowStepDefinition{
UUIDModel: relational.UUIDModel{ID: &stepDefID},
Name: "Review Documentation",
},
StartedAt: &startedAt,
CompletedAt: &completedAt,
},
}, nil).Once()

metrics, err := manager.GetExecutionMetrics(ctx, &executionID)
require.NoError(t, err)
require.Len(t, metrics.StepMetrics, 1)
assert.Equal(t, stepDefID, metrics.StepMetrics[0].StepDefinitionID)
assert.Equal(t, "Review Documentation", metrics.StepMetrics[0].StepName)
assert.NotNil(t, metrics.StepMetrics[0].DurationMinutes)
assert.Equal(t, startedAt, *metrics.StepMetrics[0].StartedAt)
assert.Equal(t, completedAt, *metrics.StepMetrics[0].CompletedAt)
}

func TestManager_GetExecutionMetrics_NilWorkflowStepDefinition(t *testing.T) {
ctx := context.Background()
logger := zap.NewNop().Sugar()

executionID := uuid.New()
stepDefID := uuid.New()
startedAt := time.Now().Add(-30 * time.Minute)
completedAt := time.Now()

mockClient := &MockRiverClient{}
mockWorkflowExecService := &MockWorkflowExecutionService{}
mockWorkflowInstService := &MockWorkflowInstanceService{}
mockStepExecService := &MockStepExecutionService{}

manager := NewManager(
mockClient,
mockWorkflowExecService,
mockWorkflowInstService,
mockStepExecService,
logger,
nil,
)

mockWorkflowExecService.On("GetByID", &executionID).Return(&workflows.WorkflowExecution{
UUIDModel: relational.UUIDModel{ID: &executionID},
Status: workflows.WorkflowStatusCompleted.String(),
StartedAt: &startedAt,
CompletedAt: &completedAt,
}, nil).Once()

mockStepExecService.On("GetByWorkflowExecutionID", &executionID).Return([]workflows.StepExecution{
{
WorkflowStepDefinitionID: &stepDefID,
WorkflowStepDefinition: nil,
StartedAt: &startedAt,
CompletedAt: &completedAt,
},
}, nil).Once()

metrics, err := manager.GetExecutionMetrics(ctx, &executionID)
require.NoError(t, err)
require.Len(t, metrics.StepMetrics, 1)
assert.Equal(t, stepDefID, metrics.StepMetrics[0].StepDefinitionID)
assert.Equal(t, "", metrics.StepMetrics[0].StepName)
}
Loading