Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Expose count_workflow_executions on the temporal client #272

Merged
merged 8 commits into from
Nov 9, 2023
Merged

Expose count_workflow_executions on the temporal client #272

merged 8 commits into from
Nov 9, 2023

Conversation

harsh-stripe
Copy link
Contributor

@harsh-stripe harsh-stripe commented Oct 17, 2023 •
edited
Loading

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
jazev-stripe suggested changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@jazev-stripe jazev-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lib/temporal/workflow/count_workflows_aggregation.rb Outdated
@@ -0,0 +1,11 @@
module Temporal
class Workflow
class CountWorkflowAggregation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

spec/unit/lib/temporal/client_spec.rb Outdated
end

it 'returns the count' do
resp = subject.count_workflow_executions(namespace, '')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

lib/temporal/client.rb Outdated
@@ -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)
Copy link
Contributor

@jazev-stripe jazev-stripe Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
jazev-stripe approved these changes Oct 27, 2023
View reviewed changes
Copy link
Contributor

@jazev-stripe jazev-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lib/temporal/client.rb Outdated Show resolved Hide resolved
@harsh-stripe @jazev-stripe
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
DeRauk reviewed Oct 30, 2023
View reviewed changes
lib/temporal/client.rb Outdated
# @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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@harsh-stripe
Copy link
Contributor Author

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

@harsh-stripe
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
Copy link
Contributor Author

@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 DeRauk merged commit cce6f02 into coinbase:master Nov 9, 2023
2 checks passed
@DeRauk
Copy link
Contributor

DeRauk commented Nov 9, 2023

Thank you!

jeffschoner pushed a commit to jeffschoner/temporal-ruby that referenced this pull request Nov 25, 2023
@harsh-stripe @jazev-stripe @jeffschoner
Expose count_workflow_executions on the temporal client (coinbase#272)
3393e36 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DeRauk DeRauk DeRauk approved these changes

@jazev-stripe jazev-stripe jazev-stripe approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@harsh-stripe @DeRauk @jazev-stripe