Add workflow trace #520

sebneira · 2024-04-03T14:04:07Z

What was changed

Ported over trace to the new cli rewrite.

Added a couple of fixes to get this to work:

Moved all the tracer printing logic to the WorkflowTracer (see Minor refactor and fixes to workflow trace #507 for the isolated changes). This was the initial fix I was working on. This should remove the issues when state == nil and the weirdness with the setupUpdateChannels.
Removed ExecutionState's GetDuration and GetStartTime time reference since the events now use durationpb.Duration and timestamppb.Timestamp and the conversion to non-pointer values makes more sense. Adjusted the templates to present this data correctly.

Why?

Ported the command to the cli rewrite and provided fixes for the nil pointer issues reported by Chad.

Checklist

Closes
How was this tested:

All unit tests are running and I've run the tool against a couple different workflow executions with temporalite.

Any docs updates needed?

CLAassistant · 2024-04-03T14:04:13Z

All committers have signed the CLA.

cretz · 2024-04-03T21:48:09Z

This is great! Will review when you mark "ready for review"

cretz

Looks pretty good. Not too concerned with the details of the internal/trace package, so didn't review that closely.

temporalcli/commands.gen.go

temporalcli/commands.workflow_trace.go

temporalcli/internal/trace/execution_state.go

Co-authored-by: Chad Retz <chad.retz@gmail.com>

cretz · 2024-04-04T13:27:14Z

temporalcli/commandsmd/commands.md

+
+#### Options
+
+* `--fold` (string) - Statuses for which Child Workflows will be folded in (this will reduce the number of information fetched and displayed). Case-insensitive and ignored if no-fold supplied. Available values: running, completed, failed, canceled, terminated, timedout, continueasnew. Default: completed,canceled,terminated.


If we supported it, would suggest string-enum[] as the type, but in the meantime, might as well just have string[] as the type. This allows them to specify --fold multiple times instead of using commas. However, default not supported for string[]. You can either add support in commandsmd/code.go or just have something like "Default is completed, canceled, terminated" which won't trigger the parser and let you set the default at runtime.

I'm happy to do this is a later iteration so we can move this (already pretty big thing) forward. Does this work for you? Tbh, I think this is a flag that will very rarely be used, since the provided default is the most reasonable.

Sure, but it will be a backwards-incompatible alteration at that time unfortunately.

Can you pass multiple statuses to a single instance of --fold? The help text says "Statuses", which makes me think this is possible, but we should make it explicit in the help text that commas are needed.

How would y'all feel about not using the flag for now and working this out in a future PR? I think it's a non-trivial decision that we would benefit from putting some good thinking behind (and not delay this PR).

Will defer to @josh-berry. I do think however it is fairly trivial to just change this to string[] and wait until you see an empty slice at runtime to set the default 3 values.

Not having the flag and adding it later is fine by me; so is accepting []string. The only thing I'm not thrilled about is making a breaking change (i.e. commas now and []string later).

👍 @sebneira - can you make this type string[] and set the defaults in code on empty slice when no-fold is not set? You can still write "Defaults are completed, canceled, and terminated." and it won't be read as a special option like Default: is.

I think after this we can merge.

Thanks for the suggestion. I updated the PR to include the string[] flag for fold which defaults to the completed, canceled and terminated if the flag is empty.

Also, as I was moving the options struct around it kinda dawned on me that the pkg made more sense as tracer rather than trace. Let me know if this doesn't make sense.

👍 Not too concerned with internal package name (workflowtrace is probably the best tbh, but doesn't really matter here)

cretz

Just pedantic stuff (would like to see fixed since easy though if you don't mind). Marking approved, LGTM overall.

temporalcli/commands.workflow_trace.go

temporalcli/commands.workflow_trace_test.go

Co-authored-by: Chad Retz <chad.retz@gmail.com>

Add workflow trace

0180e7b

sebneira mentioned this pull request Apr 3, 2024

Minor refactor and fixes to workflow trace #507

Closed

sebneira marked this pull request as ready for review April 4, 2024 11:54

Add minor fixes

caf02e1

cretz reviewed Apr 4, 2024

View reviewed changes

Sebastian Neira and others added 2 commits April 4, 2024 14:36

Disable JSON for trace command

1038e73

Co-authored-by: Chad Retz <chad.retz@gmail.com>

Use command generation

641d07c

cretz reviewed Apr 4, 2024

View reviewed changes

Sebastian Neira added 3 commits April 4, 2024 16:47

Add fixes

7117c26

Add test to trace command

0585992

Change fold flag to string list. Rename trace pkg to tracer.

e3c8d5a

cretz approved these changes Apr 9, 2024

View reviewed changes

Sebastian Neira and others added 3 commits April 9, 2024 15:35

Update temporalcli/commands.workflow_trace.go

dfdd84e

Co-authored-by: Chad Retz <chad.retz@gmail.com>

Update temporalcli/commands.workflow_trace.go

7133e28

Co-authored-by: Chad Retz <chad.retz@gmail.com>

Update temporalcli/commands.workflow_trace.go

b345676

Co-authored-by: Chad Retz <chad.retz@gmail.com>

sebneira force-pushed the seb.neira/add-trace branch from 3f78c09 to 2cb72b3 Compare April 9, 2024 13:49

Minor fixes on getfoldstatus and imports

277447f

sebneira force-pushed the seb.neira/add-trace branch from 16d6c03 to 277447f Compare April 10, 2024 10:03

cretz merged commit 4a14b05 into temporalio:cli-rewrite Apr 10, 2024
6 checks passed

josh-berry mentioned this pull request Apr 29, 2024

temporal 0.12.0 Homebrew/homebrew-core#170352

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add workflow trace #520

Add workflow trace #520

sebneira commented Apr 3, 2024 •

edited

Loading

CLAassistant commented Apr 3, 2024 •

edited

Loading

cretz commented Apr 3, 2024

cretz left a comment

cretz Apr 4, 2024

sebneira Apr 4, 2024 •

edited

Loading

cretz Apr 5, 2024

josh-berry Apr 5, 2024

sebneira Apr 8, 2024 •

edited

Loading

cretz Apr 8, 2024

josh-berry Apr 8, 2024

cretz Apr 8, 2024 •

edited

Loading

sebneira Apr 9, 2024

cretz Apr 9, 2024 •

edited

Loading

cretz left a comment •

edited

Loading


		#### Options

		* `--fold` (string) - Statuses for which Child Workflows will be folded in (this will reduce the number of information fetched and displayed). Case-insensitive and ignored if no-fold supplied. Available values: running, completed, failed, canceled, terminated, timedout, continueasnew. Default: completed,canceled,terminated.

Add workflow trace #520

Add workflow trace #520

Conversation

sebneira commented Apr 3, 2024 • edited Loading

What was changed

Why?

Checklist

CLAassistant commented Apr 3, 2024 • edited Loading

cretz commented Apr 3, 2024

cretz left a comment

Choose a reason for hiding this comment

cretz Apr 4, 2024

Choose a reason for hiding this comment

sebneira Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

cretz Apr 5, 2024

Choose a reason for hiding this comment

josh-berry Apr 5, 2024

Choose a reason for hiding this comment

sebneira Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

cretz Apr 8, 2024

Choose a reason for hiding this comment

josh-berry Apr 8, 2024

Choose a reason for hiding this comment

cretz Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

sebneira Apr 9, 2024

Choose a reason for hiding this comment

cretz Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

cretz left a comment • edited Loading

Choose a reason for hiding this comment

sebneira commented Apr 3, 2024 •

edited

Loading

CLAassistant commented Apr 3, 2024 •

edited

Loading

sebneira Apr 4, 2024 •

edited

Loading

sebneira Apr 8, 2024 •

edited

Loading

cretz Apr 8, 2024 •

edited

Loading

cretz Apr 9, 2024 •

edited

Loading

cretz left a comment •

edited

Loading