Improved workflows example program and added in statestore functionality. by RyanLettieri · Pull Request #1020 · dapr/dotnet-sdk

RyanLettieri · 2023-02-02T22:40:27Z

…ionality

Signed-off-by: Ryan Lettieri ryanLettieri@microsoft.com

Description

Please explain the changes you've made

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation

codecov · 2023-02-02T22:43:18Z

Codecov Report

Merging #1020 (1d118bb) into master (bc3ec80) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1020   +/-   ##
=======================================
  Coverage   69.45%   69.45%           
=======================================
  Files         162      162           
  Lines        5432     5432           
  Branches      585      585           
=======================================
  Hits         3773     3773           
  Misses       1519     1519           
  Partials      140      140

Flag	Coverage Δ
net6	`69.38% <ø> (ø)`
net7	`69.38% <ø> (ø)`
netcoreapp3.1	`69.42% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cgillum

Nice work on this! Here's my feedback. Mostly nit-picky stuff.

cgillum · 2023-02-02T22:44:55Z

 using WorkflowWebApp.Workflows;
+using WorkflowWebApp.Models;
 using JsonOptions = Microsoft.AspNetCore.Http.Json.JsonOptions;
+using Dapr.Client;


nit: Let's sort the using statements alphabetically.

cgillum · 2023-02-02T22:45:47Z

+    string orderId = Guid.NewGuid().ToString()[..8];
+    // All the necessary inputs (with workflow options being optional)
+    string workflowComponent = "dapr";
+    string workflowName = "OrderProcessingWorkflow";


let's do nameof(OrderProcessingWorkflow) instead of hardcoding the name. That way the program won't break if someone renames the workflow class.

cgillum · 2023-02-02T22:46:15Z

+    // All the necessary inputs (with workflow options being optional)
+    string workflowComponent = "dapr";
+    string workflowName = "OrderProcessingWorkflow";
+    object input = orderInfo;


nit: remove this and pass orderInfo as the input directly.

cgillum · 2023-02-02T22:46:43Z

+    string workflowComponent = "dapr";
+    string workflowName = "OrderProcessingWorkflow";
+    object input = orderInfo;
+    Dictionary<string, string> workflowOptions = new Dictionary<string, string>();


Is workflowOptions required or can we just pass null below since we don't add any options?

cgillum · 2023-02-02T22:46:58Z

+    string workflowName = "OrderProcessingWorkflow";
+    object input = orderInfo;
+    Dictionary<string, string> workflowOptions = new Dictionary<string, string>();
+    CancellationToken cts = new CancellationToken();


Let's remove this and use CancellationToken.None below.

cgillum · 2023-02-02T22:59:50Z

+            this.logger.LogInformation($"There are now: {original.Quantity} {original.Name} left in stock");
+
+            this.logger.LogInformation(
+                "OrderID '{requestId}' processed successfully",


I'd remove this log statement. This activity is only concerned with managing inventory and shouldn't make any assumptions about the status of the order.

cgillum · 2023-02-02T23:01:55Z

    {
        readonly ILogger logger;
+        readonly DaprClient client;
+        private static readonly string storeName = "statestore";


Looks like this is defined in two places. Let's put it somewhere more global to reduce the duplication.

cgillum · 2023-02-02T23:03:57Z

+            if (original == null)
+            {
+                // Not enough paperclips.
+                return new InventoryResult(false, original, originalETag);


Instead of original and originalETag here, just do null, null. The intent is otherwise confusing to the reader.

"original" is still needed since it is the response from the statestore. If there are no items, we need to check for it. Furthermore, we use 'original' to see the quantity of items as well as logging. I think we should just rename 'original' to 'orderResponse' or something along those lines

cgillum · 2023-02-02T23:14:32Z


 The application will listen for HTTP requests at `http://localhost:10080`.

+This workflow example utilizes a redis statestore. In order to retrieve items from this statestore, an HTTP command must first be sent down to restock the inventory:


I think you meant, "In order to populate items into the state store..."

cgillum · 2023-02-02T23:15:36Z

+On Windows (PowerShell):
+
+```powershell
+curl -i -X POST http://localhost:10080/reset `


This command actually isn't correct for PowerShell. The trailing ` means you're entering a multi-line command, which is not the case.

In any case, the command is exactly the same for both bash and powershell, so no need to distinguish them here.

mcandeia · 2023-02-03T17:21:25Z

+            }
+
+            this.logger.LogInformation(
+                "There are: {requestId}, {name} available for purchase",


question/nit: is the requestId the remaining items in stock? I wasn't sure, but the sentence sounds more correct to me without punctuation.

Suggested change

"There are: {requestId}, {name} available for purchase",

"There are {requestId} {name} available for purchase",

mcandeia · 2023-02-06T15:38:50Z

I'd rather prefer to have the shell scripts commands ready to run (for lazy users whose will copy/paste before going through the text, like me [sometimes]).

I can see two options:

Generate a random ID and print after the command get executed

randomId=$(cat /dev/urandom | LC_ALL=C tr -dc 'a-zA-Z0-9' | fold -w 50 | head -n 1) curl -i -X POST http://localhost:3500/v1.0-alpha1/workflows/dapr/OrderProcessingWorkflow/$randomId/start \ -H "Content-Type: application/json" \ -d '{ "input" : {"Name": "Paperclips", "TotalCost": 99.95, "Quantity": 1}}'

This command works on Linix OSs

Then ask the user to copy out the instance_id from the curl output.

Auto-generated workflow ids?

Given the circumstances (time-wise), I guess this option is not feasible cause it might require new APIs, etc.

You could also, assign the randomId to a variable to be reusable later

I updated the example to use a hardcoded ID. That seemed like an overall simpler solution. :)

And yes, we need to support auto-generated IDs. I'm planning to open a bug for v1.11 to support this.

mcandeia · 2023-02-06T15:56:53Z

Sry, my powershell knowledge isn't sufficient to suggest any specific solution for random string than googling it: https://devblogs.microsoft.com/scripting/generate-random-letters-with-powershell/

I switched to using harcoded IDs. It's what I use for my manual testing anyways, and the IDs can be reused after the workflow completes.

mcandeia · 2023-02-06T16:01:30Z

I guess it's not returning any Location header, what I'm missing?

The other think is that my traceparent is not working as well (returning a bunch of zero) is that correct?

I guess you have zipkin configured while I don't. So you might have to disable since it was not mentioned?

I’m planning to open a bug to track the missing Location header from the StartWorkflow API. Previously, we were returning it from the custom web API. For now, we should remove this from the directions.

I removed the Location header from the response example and removed mention of it from the text. I also went ahead and removed the Traceparent headers to avoid distracting the user from the primary objective of this example.

mcandeia · 2023-02-07T15:41:50Z

issue: the other properties uses _ and this one is using a capital letter, but since this payloads comes from runtime I guess there's no alternative than creating a issue to change this later (v1.11).

this will be seen as a breaking change but since wf is in alpha we are fine.

We can change "WFInfo" in v1.11 since it requires a change to the components-contrib code.

Tracking here: dapr/dapr#5911

mcandeia · 2023-02-07T15:42:48Z

should we fulfill the custom_status field with something meaningful ?

I think we can consider this for a future update.

mcandeia · 2023-02-07T15:44:19Z

Is it valuable to supply the documentation link for individuals without a configured Zipkin?

https://docs.dapr.io/operations/monitoring/tracing/zipkin/

mcandeia · 2023-02-07T15:45:56Z

nit: remove blank line

mcandeia · 2023-02-07T15:47:22Z

Do we need to return something <object>? Can we just return a Task ?

The base class currently requires a Task<T>, but we can look into providing a Task option in a future update.

mcandeia · 2023-02-07T15:48:40Z

Is 1 second always sufficient?

It's just for vanity purposes, but so far it's been sufficient each time I tried.

mcandeia · 2023-02-07T15:49:06Z

do we need to mention this here?

I figured that folks probably should rely on this pattern moving forward, and that it serves as a heads-up.

msfussell · 2023-02-08T23:00:14Z

I would also update

https://github.com/dapr/dotnet-sdk#readme to for workflows. e.g. Update this section "This repo builds the following packages:" Also have the pre-requisites change (now need .net 6.0?)
Need to work with @hhunter-ms to add a page here https://github.com/dapr/dotnet-sdk/tree/master/daprdocs/content/en/dotnet-sdk-docs for workflows.

cgillum · 2023-02-09T17:17:09Z

Tracking here: dapr/dapr#5911

cgillum · 2023-02-09T17:17:33Z

I think we can consider this for a future update.

cgillum · 2023-02-09T17:18:26Z

The base class currently requires a Task<T>, but we can look into providing a Task option in a future update.

cgillum · 2023-02-09T17:18:52Z

It's just for vanity purposes, but so far it's been sufficient each time I tried.

cgillum · 2023-02-09T17:19:24Z

I figured that folks probably should rely on this pattern moving forward, and that it serves as a heads-up.

Initial work for workflows DotNET SDK Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>

…ionality Signed-off-by: Ryan Lettieri <ryanLettieri@microsoft.com>