-
Notifications
You must be signed in to change notification settings - Fork 126
User metadata #1657
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
base: main
Are you sure you want to change the base?
User metadata #1657
Conversation
…es field to add a summary to an activity
* General fixed details for this workflow execution that may appear in UI/CLI. | ||
* This can be in Temporal markdown format and can span multiple lines. | ||
* | ||
* @experimental |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a short comment on all @experimental
tags (see what we did elsewhere). Benefit isn't huge, but at least, it identifies which "feature" the experimental API is linked with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe i've done so in all the relevant places
@@ -80,6 +80,7 @@ export interface ActivityInput { | |||
readonly options: ActivityOptions; | |||
readonly headers: Headers; | |||
readonly seq: number; | |||
readonly cmdOpts?: WorkflowCommandOptions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should have this WorkflowCommandOptions
type. I even wonder if we should inline summary
and details
directly in ActivityInput
(without the extra UserMetadata
nesting). Or add these two to ActivityOptions
, though that might pose some minor challenge regarding the type provided to the proxyActivities()
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What did we do in other SDKs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically they are inline, I've changed it as such (aside from using ActivityOptions
for summary/details for an activity, and TimerOptions
as you suggested below)
packages/workflow/src/workflow.ts
Outdated
*/ | ||
export function sleep(ms: Duration): Promise<void> { | ||
export function sleep(ms: Duration, summary?: string): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried this will evolve badly. What if we then need to add description
, and then other properties? Instead, I think I'd define a TimerOptions
type, and take this as a second argument here. Then, we can easily add whatever properties we need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, what do we do in other SDKs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typically we inline, but went with TimerOptions
here given your reasoning
packages/workflow/src/workflow.ts
Outdated
* @param summaries Record mapping activity names to their summary descriptions | ||
* @returns A new proxy with the provided summaries | ||
*/ | ||
withSummaries(summaries: Record<string, string>): ActivityInterfaceFor<A>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 That feels a bit awkward, and would prevent having an activity named withSummaries
(ok, that's arguably very unlikely, but still means this approach would evolve badly).
I think we could get a better DX by using something similar to this, but at the activity-level rather than at the activities-level (mind the plural). That would also allow any extra option to be set at the call site, which is a recurring request.
For example, we could have:
const { echo } = proxyActivities<MyActivities>({ startToCloseTimeout: '5m' });
// No extra options
await echo(myArg1, myArg2);
// With extra options, higher order function style
await echo.withOptions({ summary: "..." })(myArg1, myArg2);
// Some other possible variants to be considered
await echo.withOptions({ summary: "..." }, myArg1, myArg2);
await echo.withOptions({ summary: "..." }, [myArg1, myArg2]);
I'll let you think you a bit about this, but we should probably include others in this discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opted for:
await echo.runWithOptions(options, args);
to allow users to override their default activity options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed to executeWithOptions
also quickly added lazy decoding summary/details from workflow description can't do so (or at least annoying to do so) for schedule description because the description type has an invariant where the description must be a superclass of schedule options (which necessitate summary/details being strings) the alternative being to add new fields that lazily populate the existing fields, but that feels like poor ux |
packages/workflow/src/workflow.ts
Outdated
startToFireTimeout: msToTs(durationMs), | ||
}, | ||
userMetadata: options && { | ||
summary: options.summary ? activator.payloadConverter.toPayload(options.summary) : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if summary
is an empty string? As it is now, it will be replaced by undefined
. Not sure what would be the desired result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to keep empty strings, then it might be cleaner to use some toOptionalPayload
helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if we don't care about empty strings, then should instead move the terniary out of this object:
userMetadata: options?.summary
? {
summary: activator.payloadConverter.toPayload(options.summary),
}
: undefined,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think having an empty summary
string replaced with undefined
is reasonable. I don't see a reason why having an empty summary would be useful (or any more useful than undefined).
packages/workflow/src/workflow.ts
Outdated
@@ -130,8 +134,9 @@ function timerNextHandler(input: TimerInput) { | |||
* | |||
* @param ms sleep duration - number of milliseconds or {@link https://www.npmjs.com/package/ms | ms-formatted string}. | |||
* If given a negative number or 0, value will be set to 1. | |||
* @param summary a short summary/description of the timer. Can serve as a timer ID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@param summary
is boggus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup from old commit
replaced with * @param options optional timer options for additional configuration
not sure if that's entirely descriptive..
packages/workflow/src/workflow.ts
Outdated
@@ -301,7 +315,8 @@ export function scheduleActivity<R>(activityType: string, args: any[], options: | |||
export async function scheduleLocalActivity<R>( | |||
activityType: string, | |||
args: any[], | |||
options: LocalActivityOptions | |||
options: LocalActivityOptions, | |||
summary?: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
summary should be part of options, same as for regular activities
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also from old commit, removed
uses staticSummary from options now
packages/workflow/src/workflow.ts
Outdated
overrideOptions: ActivityOptions, | ||
args: any[] | ||
): Promise<unknown> { | ||
return scheduleActivity(activityType, args, { ...options, ...overrideOptions }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not so fast. You will need to merge recursively on retry policy and priority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a deepMerge
utility in object-helpers.ts
for use here (and possibly elsewhere in the future)
packages/workflow/src/workflow.ts
Outdated
overrideOptions: ActivityOptions, | ||
args: any[] | ||
): Promise<unknown> { | ||
return scheduleLocalActivity(activityType, args, { ...options, ...overrideOptions }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as before, need to handle recursive options merging on retryOptions and priority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a deepMerge
utility in object-helpers.ts
for use here (and possibly elsewhere in the future)
* | ||
* @experimental User metadata is a new API and suspectible to change. | ||
*/ | ||
staticSummary?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that should be only summary
, no static
. What's the terminology in other SDKs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bit of a mix, but generally static
prefix for workflow start methods, otherwise no static
, i'ved opted for the same
* | ||
* @experimental User metadata is a new API and suspectible to change. | ||
*/ | ||
staticSummary?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no static
prefix for activity options, changed
I believe there should also be user metadata on:
|
…ge to merge/override nested fields in objects (used to override activity options with activities' executeWithOptions), add metadata to child workflows and condition calls
Added for conditions and child workflows. Signals/updates/queries have a also going to mention this again, because it's been awhile |
What was changed
Added functionality for users to provide
staticDetails
andstaticSummary
metadata to workflow start commands.These can be retrieved via
describe
on the workflow handler and are visibile via the UI/CLI.Users can set
currentDetails
- a mutable details string within the workflow - viaSetCurrentDetails
and retrieve it viaGetCurrentDetails
. This is visible via the UI/CLI and the__temporal_workflow_metadata
internal query.Users can add a
summary
(short string description) when setting a timer and when starting an activity.A notable part of the change is the addition of
runWithOptions
to activities. Activities can now override their activity options at call time, like so:Closes [Feature Request] Support user metadata #1544
How was this tested:
Couple integration tests
Any docs updates needed?
Maybe