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.

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

User metadata and workflow metadata support #378

Merged
merged 6 commits into from
Dec 11, 2024

Conversation

cretz
Copy link
Member

@cretz cretz commented Dec 9, 2024

What was changed

  • Added WorkflowOptions.StaticSummary and WorkflowOptions.StaticDetails for setting static summary/details on workflow start, workflow signal with start, and schedule workflow action
  • Added ActivityOptions.Summary for setting a summary for activity executions
  • Added ChildWorkflowOptions.StaticSummary and ChildWorkflowOptions.StaticDetails for setting summary/details on child workflow start
  • Added Workflow.DelayWithOptionsAsync and DelayOptions for an options-based overload that supports timer Summary
    • It was decided to avoid compilation confusion with type inference to name this separately, but it does not affect interceptors
  • Added Workflow.WaitConditionWithOptionsAsync and WaitConditionOptions for an options-based overload that supports timeout timer Summary
    • It was decided to avoid compilation confusion to name this separately
  • Added WorkflowExecutionDescription.StaticSummary and WorkflowExecutionDescription.StaticDetails for viewing summary/details on workflow describe
  • Added Workflow.CurrentDetails property with get/set, so it can be set with a string visible on workflow metadata
  • Added Definition support for signal, query, and update attributes/definitions
  • Added support for __temporal_workflow_metadata query in workflow instance

Can ignore commits prior to the one for this PR, they are in main already.

Checklist

  1. Closes [Feature Request] Support user metadata #359

@cretz cretz requested a review from a team December 9, 2024 17:08
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

Makes sense to me! Not much to think about really. Left a few comment suggestions

@@ -8,11 +9,23 @@ namespace Temporalio.Worker.Interceptors
/// </summary>
/// <param name="Delay">Delay duration.</param>
/// <param name="CancellationToken">Optional cancellation token.</param>
/// <param name="Summary">Summary for the delay.</param>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth saying "which will appear in the workflow history" or something

For new users it might be really nonobvious why a "delay" would have a summary.

Copy link
Member Author

@cretz cretz Dec 9, 2024

Choose a reason for hiding this comment

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

This is the interceptor field (mostly because .NET linting makes me document it). For the one the user calls, in DelayOptions, it states:

Gets or sets a simple string identifying this timer that may be visible in UI/CLI. While it can be normal text, it is best to treat as a timer ID.

/// This can be in single-line Temporal markdown format.
/// </summary>
/// <remarks>WARNING: This setting is experimental.</remarks>
public string? Summary { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Is this one not static?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, and neither is the timer one. And technically in history, neither is the workflow one. We only call the workflow one StaticSummary because we want to use the term StaticDetails and to clarify they are immutable/static and differentiate those from the concept of CurrentDetails which is mutable. So workflows have "static" summary/details even though they are really just details, but we don't want confused with "current" details. We could not find better terminology for these when we designed them.

@@ -37,6 +37,12 @@ public WorkflowQueryAttribute(string name)
/// </summary>
public string? Name { get; }

/// <summary>
/// Gets or sets a short description for this query that may appear in UI/CLI.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth mentioning that this is returned when asking what queries a workflow supports, and that that is what may appear in UI/CLI

@@ -41,6 +41,12 @@ public WorkflowUpdateAttribute()
/// </summary>
public string? Name { get; }

/// <summary>
/// Gets or sets a short description for this update that may appear in UI/CLI.
Copy link
Member

Choose a reason for hiding this comment

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

Same about when this is returned

@cretz cretz merged commit 2546f07 into temporalio:main Dec 11, 2024
8 checks passed
@cretz cretz deleted the user-metadata branch December 11, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Support user metadata
2 participants