feat(worker): Add a workflow metadata query #1319

antlai-temporal · 2023-12-15T04:55:21Z

What was changed

Typescript SDK implementation of temporalio/features#51 providing a built-in query to return
metadata of registered query, signal, and update handlers.

Why?

See temporalio/features#51

Checklist

Closes
[Proposal] Built-in registered handlers query #1081
How was this tested:

Added two integration tests.

bergundy

LGTM, had one clarifying question.

bergundy · 2023-12-15T16:25:17Z

packages/workflow/src/internals.ts

+      {
+        handler: (): temporal.api.sdk.v1.IWorkflowMetadata => {
+          const workflowType = this.info.workflowType;
+          const description = this.info?.memo?.__temporal_workflow_description;


Hmm... I wasn't part of the design but why put this in the memo? And if we're putting this in the memo, why also return it here?

I started adding a new field, but I saw that memo is sometimes used to add a description to the workflow, and it was confusing why a new field was needed if we had memo...

The reason to also return it with the query is just convenience. There is nothing else returned by the query that associates with the queried workflow, so to make the result standalone they will have to patch it...

I think we should question if we want to put this in memo since we now have the standard query (although I see some advantages to it such as not needing a worker up to get the description).

If we want to use both memo and query, it might be better to have a dedicated method to set the description for a workflow.

I'm a bit reluctant with configuring this through a memo. At present, given that we don't support upsertMemo, that means the description would have to be provided by the code scheduling the workflow (i.e. client or parent workflow), which isn't desirable. Or, if we have upsertMemo, description would have to be set after starting the workflow, through a command, making this a billable feature.

I don't really want to change WorkflowExecutionInfo protos for something that is going to be rarely used, and its functionality is redundant to memo. And it has to work across continueAsNew, or multiple workers, so I cannot see how to do that without a proto change.

I'm inclined to just not set it at all, and if someone needs extra workflow info they can use memo + lookup with the workflow id. Alternatively, we could do an explicit setter as @bergundy mentioned, after upsertMemo is implemented in TS.

About the cost of upsertMemo, I don't think the WF description is needed to use the query method, today the UI just uses the handlers names, and, in any case, the query costs are going to dominate anyway...

Any thoughts @bergundy @mjameswh ?

Memo is not a good place to put this IMO. Memo takes up visibility store room and is only for information that needs to be returned on list/describe.

I don't really want to change WorkflowExecutionInfo protos for something that is going to be rarely used, and its functionality is redundant to memo.

I don't think this information is related to existing protos and such. This is just a query return value. If there is a TS-SDK-specific problem of being able to set metadata for a workflow, ok we can discuss this TS-SDK-specific problem with a TS-SDK-specific solution (e.g. mutable value on TS info class/iface), but this issue should be just about returning this information from query and nothing more IMO. We shouldn't reuse general purpose Temporal constructs like memo.

@cretz I was following your proto schema that included a workflow description. What kind of workflow description you don't want to appear in list/describe? Do we need this at all?
I though that in all SDKs you need to modify protos if you want some workflow info to propagate across continueAsNew and worker's multiple execution resume. Is there a standard way across SDKs to do that without modifying protos?

I was following your proto schema that included a workflow description

Which proto schema? If you mean temporalio/api#336, that was meant purely as a query result and those messages aren't used otherwise (yet).

What kind of workflow description you don't want to appear in list/describe? Do we need this at all?

I wasn't expecting list/describe to be affected by this at all. Those are server-side calls. Maybe one day, but for now this is just querying a running workflow for information about itself at this time (sorry if there was confusion on the feature repo issue, it got a bit scatted).

I though that in all SDKs you need to modify protos if you want some workflow info to propagate across continueAsNew and worker's multiple execution resume. Is there a standard way across SDKs to do that without modifying protos?

This is fixed code-based metadata, it is not runtime data necessarily. You should just answer the query with the information the workflow code knows about itself and nothing more.

This is fixed code-based metadata, it is not runtime data necessarily. You should just answer the query with the >information the workflow code knows about itself and nothing more.

OK, that changes things, I thought it could change per workflow instance and not only per workflow type.

I don't know a clean way to decorate a function in a way that works both for TS and JS. We could patch the function object to add a static method/getter, or define a static map mapping function names to descriptions, but they both look like a hack...

We could also just return the function name in the description for now...

I thought it could change per workflow instance and not only per workflow type.

It can change per workflow instance. People manually register signal handlers. But for the vast majority it won't change per instance. In Go you can't preregister anything, so it's always per instance (and there is no such thing as a signal handler). For other langs, yes, they have metadata facilities (decorators, attributes, annotations, etc). As for workflow description, I don't know of a good way for TS SDK to provide this kind of metadata and I'd be ok leaving it unset until decided.

I don't know a clean way to decorate a function in a way that works both for TS and JS. We could patch the function object to add a static method/getter, or define a static map mapping function names to descriptions, but they both look like a hack...

Same. Users could call setDescription inside the workflow at runtime or somehow at registration time, but in the meantime I'm ok leaving this unset for TS SDK while we think about it.

We could also just return the function name in the description for now...

I vote leave it unset

packages/workflow/src/internals.ts

packages/workflow/src/workflow.ts

mjameswh · 2023-12-30T21:45:04Z

packages/test/src/integration-tests-old.ts

@@ -398,7 +398,7 @@ export function runIntegrationTests(codec?: PayloadCodec): void {
    await t.throwsAsync(workflow.query('not found'), {
      instanceOf: QueryNotRegisteredError,
      message:
-        'Workflow did not register a handler for not found. Registered queries: [__stack_trace __enhanced_stack_trace isBlocked]',
+        'Workflow did not register a handler for not found. Registered queries: [__stack_trace __enhanced_stack_trace __temporal_workflow_metadata isBlocked]',


That's getting messy. I think we should consider not listing "internals" queries.

It could be useful for knowing what built-in queries, like __temporal_workflow_metadata, are supported by the WF.
Also the UI today may expect some handlers present when parsing the error message...

UI doesn't and shouldn't expect specific handlers to be present in the list returned on query not found error message. Using that error message to report existing queries is hack, not normalized across SDKs, and going forward, UI should call __temporal_workflow_metadata directly anyway.

I agree with you on the UI, but unfortunately, depending on how the UI parses the query today, we may break it if we return an empty set. It is also unfortunate that isBlocked does not have the __ internal prefix. Once they switch, we can do the cleanup with them...

👍 Fixing the query-failed string to not include all queries anymore should be a separate task once we feel comfortable enough modern UIs are no longer using it (i.e. fixed version has been out there a while). There are other approaches too such as having this error return the workflow metadata as extra error details. Hrmm, I wonder if we should do that...can discuss separately.

My point was not about query-failed no longer returning any queries, but only about not returning SDK built-in queries. For example, if using an old UI, they should not see __stack_trace, __enhanced_stack_trace and __temporal_workflow_metadata in the query drop down. Returning user queries is still fine, for now.

If needed that's easy to filter by the UI using the __ prefix. I like that we can quickly know what internal queries are supported for a given workflow...

…tadata

packages/test/src/integration-tests-old.ts

…tadata

packages/workflow/src/interfaces.ts

packages/workflow/src/internals.ts

packages/workflow/src/workflow.ts

mjameswh · 2024-01-16T20:50:30Z

I think we should prevent overriding built-in query handlers...

…tadata

antlai-temporal · 2024-01-17T23:09:38Z

I think we should prevent overriding built-in query handlers...

Yes, I have opened an issue for that #1343 I don't want to delay this one anymore...

…tadata

Add a workflow metadata query

6b21407

antlai-temporal requested a review from a team as a code owner December 15, 2023 04:55

bergundy approved these changes Dec 15, 2023

View reviewed changes

Merge branch 'main' into add-get-workflow-metadata

d016991

mjameswh reviewed Dec 28, 2023

View reviewed changes

packages/workflow/src/internals.ts Outdated Show resolved Hide resolved

mjameswh reviewed Dec 30, 2023

View reviewed changes

packages/workflow/src/workflow.ts Outdated Show resolved Hide resolved

mjameswh reviewed Dec 30, 2023

View reviewed changes

antlai-temporal added 4 commits January 9, 2024 08:50

Merge remote-tracking branch 'upstream/main' into add-get-workflow-me…

f447b04

…tadata

Track current sdk-core version

9047d65

Update sdk-core to removed dbg()

58e71f3

Remove ad-hoc types

bfeab6a

cretz reviewed Jan 10, 2024

View reviewed changes

packages/test/src/integration-tests-old.ts Show resolved Hide resolved

antlai-temporal added 3 commits January 10, 2024 09:19

Merge remote-tracking branch 'upstream/main' into add-get-workflow-me…

d79dd8d

…tadata

Fix format

eabbe0b

Remove workflow description for memo

0c3a6b5

Sushisource reviewed Jan 12, 2024

View reviewed changes

packages/workflow/src/interfaces.ts Outdated Show resolved Hide resolved

mjameswh reviewed Jan 16, 2024

View reviewed changes

packages/workflow/src/internals.ts Outdated Show resolved Hide resolved

mjameswh reviewed Jan 16, 2024

View reviewed changes

packages/workflow/src/workflow.ts Outdated Show resolved Hide resolved

antlai-temporal force-pushed the add-get-workflow-metadata branch from 5a050fb to 0c3a6b5 Compare January 17, 2024 00:40

antlai-temporal added 2 commits January 16, 2024 16:42

Merge remote-tracking branch 'upstream/main' into add-get-workflow-me…

8fcc7fb

…tadata

Remove AnyhandlerOptions

4f99a9c

Merge remote-tracking branch 'upstream/main' into add-get-workflow-me…

964d46b

…tadata

antlai-temporal merged commit 29e8037 into temporalio:main Jan 18, 2024

cretz mentioned this pull request Feb 2, 2024

[Feature Request] Standardize method for listing workflow queries/signals (and maybe other things like registered activities/workflows and other metadata) temporalio/features#51

Open

6 tasks

mjameswh mentioned this pull request Sep 27, 2024

[Proposal] Built-in registered handlers query #1081

Closed

THardy98 mentioned this pull request Jun 16, 2025

User metadata #1657

Open

feat(worker): Add a workflow metadata query #1319

feat(worker): Add a workflow metadata query #1319

Uh oh!

Conversation

antlai-temporal commented Dec 15, 2023

What was changed

Why?

Checklist

Uh oh!

bergundy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mjameswh commented Jan 16, 2024

Uh oh!

antlai-temporal commented Jan 17, 2024

Uh oh!

Uh oh!

cretz Jan 10, 2024 •

edited

Loading

cretz Jan 10, 2024 •

edited

Loading

cretz Jan 10, 2024 •

edited

Loading

cretz Jan 10, 2024 •

edited

Loading