Skip to content
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

Jump to bottom

Exposing WorkflowExecution Started Event attributes on the Workflow Info #1090

Merged
merged 4 commits into from
Apr 24, 2023
Merged

Exposing WorkflowExecution Started Event attributes on the Workflow Info #1090

merged 4 commits into from
Apr 24, 2023

Conversation

edmondop
Copy link
Contributor

@edmondop edmondop commented Apr 15, 2023 •
edited
Loading

What was changed

This pull requests address issue #1087 to expose additional fields from the WorkflowExecutionStartedEvent into the SDK

Why?

It was a missing feature that the Java SDK already implemented, and that is implemented in Cadence.

Checklist

  • [] Add unit tests

  1. Closes Expose OriginalRunId and FirstRunId  #1087

  2. How was this tested:

Not tested

  1. Any docs updates needed?

Comments describing the fields should be good

@edmondop edmondop requested a review from a team as a code owner April 15, 2023 23:36
@edmondop
Copy link
Contributor Author

edmondop commented Apr 15, 2023 •
edited
Loading

can you confirm that within the WithActivityContext, this information should not be available on the context?

When the activity context is created deserializing the Task here https://github.com/edmondop/sdk-go/blob/master/internal/activity.go#L278 the FirstRunId and OriginalRunId are not available since they are not in the protobuf scheme of https://github.com/temporalio/api/blob/ae312b0724003957b96fb966e3fe25a02abaade4/temporal/api/common/v1/message.proto#L78 and therefore they are not propagated here https://github.com/temporalio/api/blob/ae312b0724003957b96fb966e3fe25a02abaade4/temporal/api/workflowservice/v1/request_response.proto#L381

Clarified with @mfateev on Slack, thanks!

cretz
cretz reviewed Apr 17, 2023
View reviewed changes
internal/workflow.go Outdated
@@ -945,6 +945,8 @@ func (wc *workflowEnvironmentInterceptor) ExecuteChildWorkflow(ctx Context, chil
// WorkflowInfo information about currently executing workflow
type WorkflowInfo struct {
WorkflowExecution WorkflowExecution
OriginalRunID string // The original runID before resetting. Using it instead of current runID can make workflow decision determinstic after reset. See also FirstRunId
FirstRunID string // The very first original RunId of the current Workflow Execution preserved along the chain of ContinueAsNew, Retry, Cron and Reset. Identifies the whole Runs chain of Workflow Execution.
Copy link
Member

Choose a reason for hiding this comment

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

Should document fields above the field

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with b4d8ee5

@edmondop edmondop requested a review from cretz April 18, 2023 16:45
Quinn-With-Two-Ns
Quinn-With-Two-Ns approved these changes Apr 20, 2023
View reviewed changes
Copy link
Contributor

@Quinn-With-Two-Ns Quinn-With-Two-Ns left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the contribution

@cretz cretz merged commit ad1af5e into temporalio:master Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Quinn-With-Two-Ns Quinn-With-Two-Ns Quinn-With-Two-Ns approved these changes

@cretz cretz Awaiting requested review from cretz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose OriginalRunId and FirstRunId
3 participants
@edmondop @cretz @Quinn-With-Two-Ns