Expose count_workflow_executions on the temporal client by harsh-stripe · Pull Request #272 · coinbase/temporal-ruby

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

      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

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
DeRauk merged 8 commits intocoinbase:masterfrom
harsh-stripe:count-workflow-executions

harsh-stripe commented Oct 17, 2023 •

edited

Loading

Uh oh!

jazev-stripe left a comment

Uh oh!

jazev-stripe Oct 18, 2023

Uh oh!

jazev-stripe Oct 18, 2023

Uh oh!

harsh-stripe Oct 18, 2023

Uh oh!

jazev-stripe Oct 18, 2023 •

edited

Loading

Uh oh!

jazev-stripe left a comment

Uh oh!

Uh oh!

DeRauk Oct 30, 2023

Uh oh!

harsh-stripe Nov 2, 2023

Uh oh!

harsh-stripe commented Nov 2, 2023

Uh oh!

harsh-stripe commented Nov 9, 2023

Uh oh!

DeRauk commented Nov 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Conversation

harsh-stripe commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

jazev-stripe left a comment

Choose a reason for hiding this comment

Uh oh!

jazev-stripe Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

jazev-stripe Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

harsh-stripe Oct 18, 2023

Choose a reason for hiding this comment

Uh oh!

jazev-stripe Oct 18, 2023 • 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!

jazev-stripe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DeRauk Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

harsh-stripe Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

harsh-stripe commented Nov 2, 2023

Uh oh!

harsh-stripe commented Nov 9, 2023

Uh oh!

DeRauk commented Nov 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

harsh-stripe commented Oct 17, 2023 •

edited

Loading

jazev-stripe Oct 18, 2023 •

edited

Loading