Add SDK-only workflow metadata #336

antlai-temporal · 2023-12-07T04:29:45Z

What changed?
Adding a message for workflow metadata that is only available in the SDK.

Why?
The goal is to provide a standard interface for all the SDKs to read workflow metadata such as registered query, signal or update handlers.
See #temporalio/features#51

Breaking changes
no

…data-query

cretz

Looks great! Only pedantic stuff about comments. Would like to wait on rest of team to review too.

cretz · 2023-12-07T13:51:39Z

.gitignore

@@ -2,3 +2,4 @@
 /.gen
 /.vscode
 /.stamp
+*~


What commonly created files does this ignore?

I'm using emacs, best editor in the world

Is it the best editor in the world if you have to update your gitignore to avoid it's leftovers? 😁 Anyways, all good, just wanted to confirm our tooling wasn't leaving things around

cretz · 2023-12-07T13:52:59Z

temporal/api/sdk/v1/workflow_metadata.proto

+  // If missing, it represents a dynamic workflow invoked when the requested
+  // type does not match any other registered workflow type.
+  // There is at most one dynamic workflow per task queue.


Suggested change

// If missing, it represents a dynamic workflow invoked when the requested

// type does not match any other registered workflow type.

// There is at most one dynamic workflow per task queue.

// If missing, this workflow is a dynamic workflow.

Don't need the other parts of the comment, that applied to when "worker queries" were a thing.

OK. I guess it should be well known what a dynamic workflow is by now...

cretz · 2023-12-07T13:57:25Z

temporal/api/sdk/v1/workflow_metadata.proto

+  // By convention, external tools may interpret its first sentence,
+  // i.e., ending with a "." followed by a line break, as a summary of the description. 


Suggested change

// By convention, external tools may interpret its first sentence,

// i.e., ending with a "." followed by a line break, as a summary of the description.

// By convention, external tools may interpret its first part,

// i.e., ending with a line break, as a summary of the description.

My fear is that we are not going to enforce that the sentence boundary is also the line-break boundary, so how would the UI team write code to parse this? Similarly there may not be a line break at all if it's summary only. I think just "first line is summary" is acceptable, and UI can of course add an ellipsis or similar if even that is too long.

Or if you want to make sentence + line break a thing, do as https://pkg.go.dev/go/doc#Package.Synopsis does and say

Suggested change

// By convention, external tools may interpret its first sentence,

// i.e., ending with a "." followed by a line break, as a summary of the description.

// By convention, external tools may interpret its first sentence,

// i.e., ending with a "." followed by space and not preceded by capital letter or at first paragraph break, as a summary of the description.

(same goes for interaction definition)

Alternatively, it could just be markdown.

But, I think just text that the UI can decide to cut off wherever is totally fine too.

Would like to keep it plain text for now instead of markdown (that becomes a mess for UI people). But if we expect descriptions to have summaries that are cutoff at a specific point it's worth saying what that recommended cutoff is for both UI and description-writer benefit.

OK, let's keep it simple with "first line is summary". If the UI folks want other convention, it is just a comment change.

The problem here is that newlines are hard/ugly in metadata string literals in lots of programming languages. Imagine a Java annotation with newlines. I do kinda like the "line break or first non-acronym dot followed by space", but I have no strong opinion here.

cretz · 2023-12-07T13:58:05Z

temporal/api/sdk/v1/workflow_metadata.proto

+  repeated InteractionDefinition signal_definitions = 4;
+  repeated InteractionDefinition update_definitions = 5;
+}
+// (-- api-linter: core::0123::resource-annotation=disabled


Suggested change

// (-- api-linter: core::0123::resource-annotation=disabled

// (-- api-linter: core::0123::resource-annotation=disabled

cretz · 2023-12-07T13:58:34Z

temporal/api/sdk/v1/workflow_metadata.proto

+//     aip.dev/not-precedent: The `name` field is optional. --)
+// (-- api-linter: core::0203::optional=disabled --)
+message InteractionDefinition {
+  // An optional name to select the handler. If missing, it represents


Suggested change

// An optional name to select the handler. If missing, it represents

// An optional name for the handler. If missing, it represents

There is no real selecting here

cretz · 2023-12-07T13:59:08Z

temporal/api/sdk/v1/workflow_metadata.proto

+// (-- api-linter: core::0203::optional=disabled --)
+message InteractionDefinition {
+  // An optional name to select the handler. If missing, it represents
+  // a dynamic handler that process any interactions not handled by others.


Suggested change

// a dynamic handler that process any interactions not handled by others.

// a dynamic handler that processes any interactions not handled by others.

cretz · 2023-12-07T14:00:18Z

temporal/api/sdk/v1/workflow_metadata.proto

+option ruby_package = "Temporalio::Api::Sdk::V1";
+option csharp_namespace = "Temporalio.Api.Sdk.V1";
+
+message WorkflowMetadata {


Can you add a comment saying the name of the query that retrieves this information? We need that somewhere, we might as well put it here.

Makes sense, using __temporal_getWorkflowMetadata

👍 Added benefit of that query is that it will fail on older SDKs thereby returning the same thing as the UI's little intentional-failed-query-check would. We do need to discuss the cloud pricing that may be associated with this query however (previously the intentional-failed-query-check would not charge because the query did not succeed).

cretz · 2023-12-07T14:03:01Z

temporal/api/sdk/v1/workflow_metadata.proto

+// (-- api-linter: core::0123::resource-annotation=disabled
+//     aip.dev/not-precedent: The `name` field is optional. --)
+// (-- api-linter: core::0203::optional=disabled --)
+message InteractionDefinition {


Suggested change

message InteractionDefinition {

message WorkflowInteractionDefinition {

I should have qualified this when I first made this message, sorry. I would also support just Interaction as a sub-message of WorkflowDefinition.

Using WorkflowInteractionDefinition

Quinn-With-Two-Ns · 2023-12-07T17:20:29Z

Will the query for this metadata have any parameters or are users only able to query for the full thing?

Sushisource

Cool! Agree with the doc suggestions

antlai-temporal · 2023-12-07T21:37:23Z

Will the query for this metadata have any parameters or are users only able to query for the full thing?
I think the whole thing to begin with (it should not be that much data). As @cretz mentioned the issue is also pricing,
if we decide to charge for the query, many partial queries make no sense... But my preference is that queries with _temporal prefix be free, as it is today for the failed query...
BTW, that means we should not allow apps to register queries with the __temporal prefix...

antlai-temporal added 2 commits December 6, 2023 20:09

Adding SDK workflow metadata message

bac1693

Merge remote-tracking branch 'upstream/master' into add-workflow-meta…

7a5b0e5

…data-query

antlai-temporal requested review from a team as code owners December 7, 2023 04:29

antlai-temporal requested review from bergundy, cretz and Alex-Tideman December 7, 2023 04:30

cretz approved these changes Dec 7, 2023

View reviewed changes

cretz reviewed Dec 7, 2023

View reviewed changes

Sushisource approved these changes Dec 7, 2023

View reviewed changes

Edit comments

6d17e8c

antlai-temporal merged commit 798c32f into temporalio:master Dec 8, 2023
4 checks passed

cretz mentioned this pull request Dec 11, 2023

Update upstream protos temporalio/sdk-core#655

Merged

cretz mentioned this pull request Jan 10, 2024

feat(worker): Add a workflow metadata query temporalio/sdk-typescript#1319

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SDK-only workflow metadata #336

Add SDK-only workflow metadata #336

antlai-temporal commented Dec 7, 2023

cretz left a comment

cretz Dec 7, 2023

antlai-temporal Dec 7, 2023

cretz Dec 7, 2023

cretz Dec 7, 2023

antlai-temporal Dec 7, 2023

cretz Dec 7, 2023

Sushisource Dec 7, 2023 •

edited

Loading

cretz Dec 7, 2023

antlai-temporal Dec 7, 2023

cretz Dec 7, 2023

cretz Dec 7, 2023

antlai-temporal Dec 7, 2023

cretz Dec 7, 2023

antlai-temporal Dec 7, 2023

cretz Dec 7, 2023

antlai-temporal Dec 7, 2023

cretz Dec 7, 2023

antlai-temporal Dec 7, 2023

cretz Dec 7, 2023

cretz Dec 7, 2023

antlai-temporal Dec 7, 2023

Quinn-With-Two-Ns commented Dec 7, 2023

Sushisource left a comment

antlai-temporal commented Dec 7, 2023 •

edited

Loading

		// By convention, external tools may interpret its first sentence,
		// i.e., ending with a "." followed by a line break, as a summary of the description.

	// (-- api-linter: core::0123::resource-annotation=disabled

	// (-- api-linter: core::0123::resource-annotation=disabled

	// An optional name to select the handler. If missing, it represents
	// An optional name for the handler. If missing, it represents

	// a dynamic handler that process any interactions not handled by others.
	// a dynamic handler that processes any interactions not handled by others.

	message InteractionDefinition {
	message WorkflowInteractionDefinition {

Add SDK-only workflow metadata #336

Add SDK-only workflow metadata #336

Conversation

antlai-temporal commented Dec 7, 2023

cretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushisource Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quinn-With-Two-Ns commented Dec 7, 2023

Sushisource left a comment

Choose a reason for hiding this comment

antlai-temporal commented Dec 7, 2023 • edited Loading

Sushisource Dec 7, 2023 •

edited

Loading

antlai-temporal commented Dec 7, 2023 •

edited

Loading