Expose count_workflow_executions on the temporal client #272

harsh-stripe · 2023-10-17T23:51:27Z

Summary

This change exposes a count_workflow_executions method on the Temporal client so that users can invoke it using code such as:

client = Temporal::Client.new(config)
result = client.count_workflow_executions(namespace: 'ruby-samples', query: 'WorkflowType="TestingWorkflow"')

Test Plan

Unit test:

bundle exec rspec spec/unit/lib/temporal/client_spec.rb:1106

jazev-stripe

I only had a few small ideas; let me know if you disagree

jazev-stripe · 2023-10-18T22:11:16Z

lib/temporal/workflow/count_workflows_aggregation.rb

@@ -0,0 +1,11 @@
+module Temporal
+  class Workflow
+    class CountWorkflowAggregation


ooc, why "Aggregation" as the suffix instead of a more neutral "Result" or "Response"? Does aggregation mean it is aggregating all of the fields in the CountWorkflowExecutionsResponse, or is it a reference to the CountWorkflowExecutionsResponse.AggregationGroup type?

jazev-stripe · 2023-10-18T22:11:47Z

spec/unit/lib/temporal/client_spec.rb

+    end
+
+    it 'returns the count' do
+      resp = subject.count_workflow_executions(namespace, '')


nit: having a non-empty query makes the test slightly more realistic (at least I don't think an empty query works on the real server)

Believe it or not, an empty query does work on the real server (see slack), but I've gone ahead and modified the test to be more realistic (there's very few scenarios during which you'd do an empty query).

jazev-stripe · 2023-10-18T22:13:37Z

lib/temporal/client.rb

@@ -425,6 +426,11 @@ def query_workflow_executions(namespace, query, filter: {}, next_page_token: nil
      Temporal::Workflow::Executions.new(connection: connection, status: :all, request_options: { namespace: namespace, query: query, next_page_token: next_page_token, max_page_size: max_page_size }.merge(filter))
    end

+    def count_workflow_executions(namespace, query)


it would be nice to have a short doc-comment with the types of the parameters/return value (even if it isn't enforced without Sorbet, and in this case the types are fairly trivial). I know a lot of the other methods don't have it, but I'm putting my type evangelist hat on 🤠.

jazev-stripe

The only bit of feedback was the typo in the doc-comment. Other than that, it looks great

lib/temporal/client.rb

Fix a typo with the new return type. Co-authored-by: jazev-stripe <128553781+jazev-stripe@users.noreply.github.com>

DeRauk · 2023-10-30T14:18:41Z

lib/temporal/client.rb

+    # @return [Temporal::Workflow::CountWorkflowsResult] an integer count of workflows matching the query
+    def count_workflow_executions(namespace, query: nil)
+      response = connection.count_workflow_executions(namespace: namespace, query: query)
+      Workflow::CountWorkflowsResult.new(count: response.count)


@harsh-stripe Why create a new response object to return here? It'd be simpler to just return the count from this method

Made a commit to return the count instead of this wrapped object. I had initially created it thinking it might be useful for a future contributor if they wanted to include aggregation groups based on https://github.com/temporalio/api/blob/master/temporal/api/workflowservice/v1/request_response.proto#L795-L798 in the future, but it isn't required now since it doesn't quite work yet in Temporal.

…ng it

harsh-stripe · 2023-11-02T22:10:46Z

@DeRauk - Addressed your feedback to return the count value directly instead of as a wrapped object.

…up in github actions and while this test is nice to have, it isn't critical to have an integration spec for it because it relies on timing due to the async visibility store

harsh-stripe · 2023-11-09T17:54:07Z

@DeRauk - it looks like the GitHub actions for this project sets up a temporal-server instance which does not use advanced visibility.

Ultimately, I think it might just be worth removing the example spec for count_workflow_executions since the spec relies on a visibility backend store, and it is also reliant on timing since the data push to the visibility store happens async. The spec would've been nice to have, but I don't believe it's critical, so I've removed it.

DeRauk · 2023-11-09T19:03:10Z

Thank you!

* Expose count_workflow_executions on the temporal client * Return a wrapped type for count_workflows response * Add integration tests * Remove debug log lines * Update lib/temporal/client.rb Fix a typo with the new return type. Co-authored-by: jazev-stripe <128553781+jazev-stripe@users.noreply.github.com> * Return the count value of count_workflow_executions instead of wrapping it * Remove the example integration spec for count_workflows. ES isn't setup in github actions and while this test is nice to have, it isn't critical to have an integration spec for it because it relies on timing due to the async visibility store --------- Co-authored-by: jazev-stripe <128553781+jazev-stripe@users.noreply.github.com>

harsh-stripe added 2 commits October 17, 2023 16:26

Expose count_workflow_executions on the temporal client

09eca23

Return a wrapped type for count_workflows response

f8dd685

jazev-stripe suggested changes Oct 18, 2023

View reviewed changes

harsh-stripe added 2 commits October 18, 2023 18:37

Add integration tests

17660a3

Remove debug log lines

be52fa7

harsh-stripe requested a review from jazev-stripe October 19, 2023 01:44

jazev-stripe approved these changes Oct 27, 2023

View reviewed changes

lib/temporal/client.rb Outdated Show resolved Hide resolved

Update lib/temporal/client.rb

35bad5d

Fix a typo with the new return type. Co-authored-by: jazev-stripe <128553781+jazev-stripe@users.noreply.github.com>

DeRauk reviewed Oct 30, 2023

View reviewed changes

harsh-stripe added 2 commits November 2, 2023 15:05

Return the count value of count_workflow_executions instead of wrappi…

29e7b78

…ng it

Resolve merge conflicts

cbf1008

harsh-stripe requested a review from DeRauk November 2, 2023 22:10

DeRauk approved these changes Nov 8, 2023

View reviewed changes

Remove the example integration spec for count_workflows. ES isn't set…

3239f9a

…up in github actions and while this test is nice to have, it isn't critical to have an integration spec for it because it relies on timing due to the async visibility store

harsh-stripe requested a review from DeRauk November 9, 2023 17:54

DeRauk approved these changes Nov 9, 2023

View reviewed changes

DeRauk merged commit cce6f02 into coinbase:master Nov 9, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose count_workflow_executions on the temporal client #272

Expose count_workflow_executions on the temporal client #272

harsh-stripe commented Oct 17, 2023 •

edited

Loading

jazev-stripe left a comment

jazev-stripe Oct 18, 2023

jazev-stripe Oct 18, 2023

harsh-stripe Oct 18, 2023

jazev-stripe Oct 18, 2023 •

edited

Loading

jazev-stripe left a comment

DeRauk Oct 30, 2023

harsh-stripe Nov 2, 2023

harsh-stripe commented Nov 2, 2023

harsh-stripe commented Nov 9, 2023

DeRauk commented Nov 9, 2023

Expose count_workflow_executions on the temporal client #272

Expose count_workflow_executions on the temporal client #272

Conversation

harsh-stripe commented Oct 17, 2023 • edited Loading

Summary

Test Plan

jazev-stripe left a comment

Choose a reason for hiding this comment

jazev-stripe Oct 18, 2023

Choose a reason for hiding this comment

jazev-stripe Oct 18, 2023

Choose a reason for hiding this comment

harsh-stripe Oct 18, 2023

Choose a reason for hiding this comment

jazev-stripe Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

jazev-stripe left a comment

Choose a reason for hiding this comment

DeRauk Oct 30, 2023

Choose a reason for hiding this comment

harsh-stripe Nov 2, 2023

Choose a reason for hiding this comment

harsh-stripe commented Nov 2, 2023

harsh-stripe commented Nov 9, 2023

DeRauk commented Nov 9, 2023

harsh-stripe commented Oct 17, 2023 •

edited

Loading

jazev-stripe Oct 18, 2023 •

edited

Loading