Added Counter Interceptor Sample #77

rross · 2024-07-23T19:41:46Z

What was changed

Added a CounterInterceptor example

Why?

Demonstrate how to use interceptors with the .NET SDK

Checklist

Closes n/a
How was this tested:
Added a test that validates the counts.
Any docs updates needed?
Updated the main README to include this sample

CLAassistant · 2024-07-23T19:41:52Z

All committers have signed the CLA.

CLAassistant · 2024-07-23T19:41:52Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

src/CounterInterceptor/ClientCounter.cs

src/CounterInterceptor/Program.cs

src/CounterInterceptor/README.md

src/CounterInterceptor/SimpleClientCallsInterceptor.cs

src/CounterInterceptor/WorkerCounter.cs

tests/CounterInterceptor/MyWorkflowTests.cs

README.md

src/CounterInterceptor/ClientCounter.cs

src/CounterInterceptor/SimpleCounterWorkerInterceptor.cs

src/CounterInterceptor/TemporalioSamples.CounterInterceptor.csproj

tests/CounterInterceptor/MyWorkflowTests.cs

tests/TemporalioSamples.Tests.csproj

cretz · 2024-08-05T15:20:34Z

src/CounterInterceptor/ClientCounter.cs

@@ -0,0 +1,87 @@
+namespace TemporalioSamples.CounterInterceptor;


A bit surprised this passes CI, maybe we don't have a dotnet format check. Usually we want there to be a blank line between namespace declaration and type declarataion

cretz · 2024-08-05T15:21:23Z

src/CounterInterceptor/ClientCounter.cs

@@ -0,0 +1,87 @@
+namespace TemporalioSamples.CounterInterceptor;
+public record ClientCounts


Should not have a top-level type declared in any non-Program.cs file that doesn't match its name. Either make it a nested class or give it its own file.

Good catch. Fixed.

cretz · 2024-08-05T15:22:56Z

src/CounterInterceptor/ClientCounter.cs

+    public override string ToString()
+    {
+        return
+                "\n\tTotal Number of Workflow Exec: " + Executions +
+                "\n\tTotal Number of Signals: " + Signals +
+                "\n\tTotal Number of Queries: " + Queries;
+    }


Suggested change

public override string ToString()

{

return

"\n\tTotal Number of Workflow Exec: " + Executions +

"\n\tTotal Number of Signals: " + Signals +

"\n\tTotal Number of Queries: " + Queries;

}

public override string ToString() =>

$"\n\tTotal Number of Workflow Exec: {Executions}\n\t" +

$"Total Number of Signals: {Signals}\n\tTotal Number of Queries: {Queries}";

}

Can break up long line, but might as well use interpolation, and might as well use => for single-line methods. Granted, despite what I said before, using ToString() for multiline strings only for a specific scenario is a bit much, but fine.

cretz · 2024-08-05T15:24:36Z

src/CounterInterceptor/ClientCounter.cs

+            perWorkflowIdDictionary.Select(kvp => $"** Workflow ID: {kvp.Key} {kvp.Value}"));
+    }
+
+    public static uint NumOfWorkflowExecutions(string workflowId)


Use => for single-line methods

cretz · 2024-08-05T15:25:19Z

src/CounterInterceptor/ClientCounter.cs

+    }
+}
+
+public class ClientCounter


Why have this separate class? Why not just use Dictionary<string, ClientCounts> where it's needed? Do not use static state, store the dictionary on the interceptor and make it available to the creator of the interceptor.

Refactored.

cretz · 2024-08-05T15:36:19Z

src/CounterInterceptor/SimpleClientCallsInterceptor.cs

+
+public class SimpleClientCallsInterceptor : IClientInterceptor
+{
+    private ClientCounter clientCounter;


cretz · 2024-08-05T15:37:16Z

src/CounterInterceptor/SimpleClientCallsInterceptor.cs

+            return base.StartWorkflowUpdateAsync<TResult>(input);
+        }
+
+        private static string CheckId(string? id)


Bump, only three places have this, no need to make a whole new method for a simple id ?? "None" in those three places

cretz · 2024-08-05T15:37:29Z

src/CounterInterceptor/SimpleClientCallsInterceptor.cs

+        public override Task<WorkflowUpdateHandle<TResult>> StartWorkflowUpdateAsync<TResult>(
+            StartWorkflowUpdateInput input)
+        {
+            // Not tracking this


Then can remove this method entirely

cretz · 2024-08-05T15:37:47Z

src/CounterInterceptor/SimpleCounterWorkerInterceptor.cs

+        public override Task<TResult> ScheduleActivityAsync<TResult>(
+            ScheduleActivityInput input)
+        {
+            return base.ScheduleActivityAsync<TResult>(input);
+        }
+
+        public override Task SignalChildWorkflowAsync(SignalChildWorkflowInput input)
+        {
+            return base.SignalChildWorkflowAsync(input);
+        }
+
+        public override Task SignalExternalWorkflowAsync(SignalExternalWorkflowInput input)
+        {
+            return base.SignalExternalWorkflowAsync(input);
+        }


cretz · 2024-08-05T15:38:10Z

src/CounterInterceptor/WorkerCounter.cs

All the same things on the ClientCounter class apply here

cretz · 2024-08-13T12:25:13Z

src/CounterInterceptor/MyActivities.cs

+    public string SayHello(string name, string title)
+    {
+        return "Hello " + title + " " + name;
+    }


Bump, this was not addressed

cretz · 2024-08-13T12:26:00Z

src/CounterInterceptor/MyWorkflow.workflow.cs

+    public async Task<string> RunAsync()
+    {
+        // Wait for greeting info
+        await Workflow.WaitConditionAsync(() => Name != null && Title != null);


Bump, this was never addressed

cretz · 2024-08-13T12:26:35Z

src/CounterInterceptor/Program.cs

+            });
+
+        using var tokenSource = new CancellationTokenSource();
+        Console.CancelKeyPress += (_, eventArgs) =>


Bump, this was never addressed

cretz · 2024-08-13T12:27:11Z

src/CounterInterceptor/Program.cs

+
+            var result = await handle.GetResultAsync();
+
+            Console.WriteLine($"Workflow result is {result}", result);


Bump this was never addressed. If you are using interpolation, you don't also pass as a param.

cretz · 2024-08-13T12:27:20Z

src/CounterInterceptor/README.md

+## Start local Temporal Server
+```bash
+# run only once
+temporal server start-dev
+```


Bump, this was never addressed

cretz · 2024-08-13T12:35:28Z

src/CounterInterceptor/SimpleClientCallsInterceptor.cs

+    private const string NumberOfWorkflowExecutions = "numOfWorkflowExec";
+    private const string NumberOfSignals = "numOfSignals";
+    private const string NumberOfQueries = "numOfQueries";
+    private static Dictionary<string, ClientCounts> clientDictionary = new();


This should not be static and it should be readonly and you do not have to suffix "Dictionary", just call it counts or something

cretz · 2024-08-13T12:36:02Z

src/CounterInterceptor/SimpleClientCallsInterceptor.cs

+    public ClientOutboundInterceptor InterceptClient(ClientOutboundInterceptor nextInterceptor) =>
+        new ClientOutbound(this, nextInterceptor);
+
+    public string Info() =>


May be easier if you just let the caller access the dictionary and do what they want with it instead of all of these separate methods to read/write it

cretz · 2024-08-13T12:37:14Z

src/CounterInterceptor/SimpleCounterWorkerInterceptor.cs

Most things in client interceptor class apply here

cretz · 2024-08-13T12:37:48Z

src/CounterInterceptor/SimpleClientCallsInterceptor.cs

+        $"Total Number of Queries: {Queries}";
+}
+
+public class SimpleClientCallsInterceptor : IClientInterceptor


IMO for simplicity you should make one interceptor that implements both IClientInterceptor and IWorkerInterceptor (and you only then need to configure the client not the worker interceptor separately). You could also make just one counts object.

cretz · 2024-08-13T12:38:36Z

src/CounterInterceptor/SimpleCounterWorkerInterceptor.cs

+        public override Task<TResult> ScheduleActivityAsync<TResult>(
+            ScheduleActivityInput input)
+        {
+            return base.ScheduleActivityAsync<TResult>(input);
+        }
+
+        public override Task SignalChildWorkflowAsync(SignalChildWorkflowInput input)
+        {
+            return base.SignalChildWorkflowAsync(input);
+        }
+
+        public override Task SignalExternalWorkflowAsync(SignalExternalWorkflowInput input)
+        {
+            return base.SignalExternalWorkflowAsync(input);
+        }


Bump again, these weren't addressed

cretz · 2024-08-16T13:04:10Z

src/CounterInterceptor/Counts.cs

@@ -0,0 +1,54 @@
+public record Counts


No benefit to making a record, might as well make a class. Also, make sure you put the namespace on this file.

cretz · 2024-08-16T13:04:30Z

src/CounterInterceptor/Counts.cs

+    public Counts()
+    {
+        clientExecutions = 0;
+        clientQueries = 0;
+        clientSignals = 0;
+        workflowExecutions = 0;
+        workflowSignals = 0;
+        workflowQueries = 0;
+        workflowChildExecutions = 0;
+        workflowActivityExecutions = 0;
+    }
+


Suggested change

public Counts()

{

clientExecutions = 0;

clientQueries = 0;

clientSignals = 0;

workflowExecutions = 0;

workflowSignals = 0;

workflowQueries = 0;

workflowChildExecutions = 0;

workflowActivityExecutions = 0;

}

Not needed, this is the default value already of every field here.

cretz · 2024-08-16T13:07:13Z

src/CounterInterceptor/MyActivities.cs

+{
+    [Activity]
+    public string SayHello(string name, string title) =>
+        "Hello " + title + " " + name;


Suggested change

"Hello " + title + " " + name;

$"Hello {title} {name}";

I think interpolation is a bit clearer here and in next activity

cretz · 2024-08-16T13:07:31Z

src/CounterInterceptor/MyActivities.cs

+        "Hello " + title + " " + name;
+
+    [Activity]
+    public string SayGoodBye(string name, string title)


You are inconsistent with your single-line methods, this should be => like the one just above it

cretz · 2024-08-16T13:08:11Z

src/CounterInterceptor/MyCounterInterceptor.cs

+    private ConcurrentDictionary<string, Counts> counts = new();
+
+    public ConcurrentDictionary<string, Counts> Counts => counts;


Suggested change

private ConcurrentDictionary<string, Counts> counts = new();

public ConcurrentDictionary<string, Counts> Counts => counts;

public ConcurrentDictionary<string, Counts> Counts { get; } = new();

Use auto properties

cretz · 2024-08-16T13:20:10Z

src/CounterInterceptor/MyCounterInterceptor.cs

+            // we don't care if it doesn't add it as we will
+            // still increment the current value.
+            root.counts.TryAdd(id, new Counts());
+            Interlocked.Increment(ref root.counts[id].WorkflowExecutions);


Note, this is not workflow executions, but workflow replays. You could get 1000 of these for the same workflow. Same for other counts in the workflow inbound/outbound. These will be double counted on every replay. You can consider not doing this on replay, or you can clarify in docs of the fields that they include replay attempts (you probably want the former).

Oh wow. That's important to call out.

I see that Workflow.Info has an Attempt field, but not sure it makes sense to use that to determine replay, as I think that is used for the number of retry attempts.

There is Workflow.Unsafe.IsReplaying you can make sure is false before doing the increment (this is what we do in tracing interceptor for instance). It should be noted though that on task failure, this will run over and over in non-replaying situation, but besides task failure it will work.

cretz · 2024-08-16T13:21:11Z

src/CounterInterceptor/MyWorkflow.workflow.cs

+    public async Task<string> RunAsync()
+    {
+        // Wait for greeting info
+        await Workflow.WaitConditionAsync(() => Name != null && Title != null);


Bump again, this is still not addressed

cretz · 2024-08-16T13:22:09Z

src/CounterInterceptor/Program.cs

+            });
+
+        using var tokenSource = new CancellationTokenSource();
+        Console.CancelKeyPress += (_, eventArgs) =>


Bump again, this is still not addressed

cretz · 2024-08-16T13:22:39Z

src/CounterInterceptor/README.md

+## Start local Temporal Server
+```bash
+# run only once
+temporal server start-dev
+```


Bump again, this is still not addressed

cretz · 2024-08-16T13:22:48Z

src/CounterInterceptor/README.md

+temporal server start-dev
+```
+
+## Run Worker Locally


Bump again, this is still not addressed

cretz

Looks good, just want to take a look at the outstanding issues on the README

cretz · 2024-08-19T18:07:48Z

src/CounterInterceptor/Counts.cs

+namespace TemporalioSamples.CounterInterceptor;
+public class Counts


You are inconsistent with whether you add spacing between namespace and type declaration

cretz

Looks good, thanks!

cretz · 2024-08-20T12:24:23Z

src/CounterInterceptor/README.md

@@ -0,0 +1,13 @@
+# dotnet-counter-interceptor


Not really the best title, but that's ok

rross added 2 commits July 23, 2024 14:17

Added CounterInterceptor Sample

d678366

Fixed spelling error. Added tests

f428229

Fixed whitespace format issue

26fdbee