-
Notifications
You must be signed in to change notification settings - Fork 33
Added Counter Interceptor Sample #77
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.
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
Changes from 14 commits
d678366
f428229
26fdbee
c68e75d
ec9da55
31c0743
ca90698
7ac67a9
34c83af
7682937
37692a7
d548089
d113bd3
c0131fd
4557ffe
c7b6e31
f3a82cc
078bbe6
fc4fada
c6f5e55
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,54 @@ | ||||||||||||||||||||||||
| public record Counts | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| public Counts() | ||||||||||||||||||||||||
| { | ||||||||||||||||||||||||
| clientExecutions = 0; | ||||||||||||||||||||||||
| clientQueries = 0; | ||||||||||||||||||||||||
| clientSignals = 0; | ||||||||||||||||||||||||
| workflowExecutions = 0; | ||||||||||||||||||||||||
| workflowSignals = 0; | ||||||||||||||||||||||||
| workflowQueries = 0; | ||||||||||||||||||||||||
| workflowChildExecutions = 0; | ||||||||||||||||||||||||
| workflowActivityExecutions = 0; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
||||||||||||||||||||||||
| 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,17 @@ | ||||||
| namespace TemporalioSamples.CounterInterceptor; | ||||||
|
|
||||||
| using System.Diagnostics; | ||||||
| using Temporalio.Activities; | ||||||
|
|
||||||
| public class MyActivities | ||||||
| { | ||||||
| [Activity] | ||||||
| public string SayHello(string name, string title) => | ||||||
| "Hello " + title + " " + name; | ||||||
|
||||||
| "Hello " + title + " " + name; | |
| $"Hello {title} {name}"; |
I think interpolation is a bit clearer here and in next activity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are inconsistent with your single-line methods, this should be => like the one just above it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| namespace TemporalioSamples.CounterInterceptor; | ||
|
|
||
| using Temporalio.Workflows; | ||
|
|
||
| [Workflow] | ||
| public class MyChildWorkflow | ||
| { | ||
| private readonly ActivityOptions activityOptions = new() | ||
| { | ||
| StartToCloseTimeout = TimeSpan.FromSeconds(10), | ||
| }; | ||
|
|
||
| [WorkflowRun] | ||
| public async Task<string> RunAsync(string name, string title) => | ||
| await Workflow.ExecuteActivityAsync((MyActivities act) => act.SayHello(name, title), activityOptions) + | ||
| await Workflow.ExecuteActivityAsync((MyActivities act) => act.SayGoodBye(name, title), activityOptions); | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,137 @@ | ||||||||||
| namespace TemporalioSamples.CounterInterceptor; | ||||||||||
|
|
||||||||||
| using System.Collections.Concurrent; | ||||||||||
| using Temporalio.Activities; | ||||||||||
| using Temporalio.Client; | ||||||||||
| using Temporalio.Client.Interceptors; | ||||||||||
| using Temporalio.Worker.Interceptors; | ||||||||||
| using Temporalio.Workflows; | ||||||||||
|
|
||||||||||
| public class MyCounterInterceptor : IClientInterceptor, IWorkerInterceptor | ||||||||||
| { | ||||||||||
| private ConcurrentDictionary<string, Counts> counts = new(); | ||||||||||
|
|
||||||||||
| public ConcurrentDictionary<string, Counts> Counts => counts; | ||||||||||
|
||||||||||
| private ConcurrentDictionary<string, Counts> counts = new(); | |
| public ConcurrentDictionary<string, Counts> Counts => counts; | |
| public ConcurrentDictionary<string, Counts> Counts { get; } = new(); |
Use auto properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should happen on every call, not just start/execute workflow. The root.counts[id].ClientSignals call below will throw an exception when someone uses this to signal a workflow that wasn't started here. Same for other situations. Consider just having a helper on the root like:
private void Increment(string id, Action<Counts> increment) =>
increment(counts.GetOrAdd(id, _ => new()));Or something like that (untested). Then you can just change this to root.Increment(id ?? "None", c => Interlocked.Increment(ref c.ClientExecutions)); I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debated on whether it needed to be done on every call or not.
I was able to use your example as you had it. Worked the first time.
Thanks!
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
input.Id can never be null on this call (and some others), so ?? should be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pedantic, but can also be single-method syntax using =>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| namespace TemporalioSamples.CounterInterceptor; | ||
|
|
||
| using Temporalio.Workflows; | ||
|
|
||
| [Workflow] | ||
| public class MyWorkflow | ||
| { | ||
| private bool exit; // Automatically defaults to false | ||
|
|
||
| [WorkflowRun] | ||
| public async Task<string> RunAsync() | ||
| { | ||
| // Wait for greeting info | ||
| await Workflow.WaitConditionAsync(() => Name != null && Title != null); | ||
|
||
|
|
||
| // Execute Child Workflow | ||
| var result = await Workflow.ExecuteChildWorkflowAsync( | ||
| (MyChildWorkflow wf) => wf.RunAsync(Name, Title), | ||
| new() { Id = "counter-interceptor-child" }); | ||
|
|
||
| // Wait for exit signal | ||
| await Workflow.WaitConditionAsync(() => exit); | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| [WorkflowSignal] | ||
| public async Task SignalNameAndTitleAsync(string name, string title) | ||
| { | ||
| Name = name; | ||
| Title = title; | ||
| } | ||
|
|
||
| [WorkflowQuery] | ||
| public string Name { get; private set; } = string.Empty; | ||
|
|
||
| [WorkflowQuery] | ||
| public string Title { get; private set; } = string.Empty; | ||
|
|
||
| [WorkflowSignal] | ||
| public async Task ExitAsync() => exit = true; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| namespace TemporalioSamples.CounterInterceptor; | ||
|
|
||
| using Temporalio.Client; | ||
| using Temporalio.Worker; | ||
|
|
||
| internal class Program | ||
| { | ||
| private static async Task Main(string[] args) | ||
| { | ||
| var counterInterceptor = new MyCounterInterceptor(); | ||
| var client = await TemporalClient.ConnectAsync( | ||
| options: new("localhost:7233") | ||
| { | ||
| Interceptors = new[] | ||
| { | ||
| counterInterceptor, | ||
| }, | ||
| }); | ||
|
|
||
| using var tokenSource = new CancellationTokenSource(); | ||
| Console.CancelKeyPress += (_, eventArgs) => | ||
|
||
| { | ||
| tokenSource.Cancel(); | ||
| eventArgs.Cancel = true; | ||
| }; | ||
|
|
||
| var activities = new MyActivities(); | ||
|
|
||
| var taskQueue = "CounterInterceptorTaskQueue"; | ||
|
|
||
| var workerOptions = new TemporalWorkerOptions(taskQueue). | ||
| AddAllActivities(activities). | ||
| AddWorkflow<MyWorkflow>(). | ||
| AddWorkflow<MyChildWorkflow>(); | ||
|
|
||
| // workerOptions.Interceptors = new[] { counterInterceptor }; | ||
| using var worker = new TemporalWorker( | ||
| client, | ||
| workerOptions); | ||
|
|
||
| // Run worker until cancelled | ||
| Console.WriteLine("Running worker..."); | ||
| try | ||
| { | ||
| // Start the workers | ||
| var workerResult = worker.ExecuteAsync(tokenSource.Token); | ||
|
|
||
| // Start the workflow | ||
| var handle = await client.StartWorkflowAsync( | ||
| (MyWorkflow wf) => wf.RunAsync(), | ||
| new(id: Guid.NewGuid().ToString(), taskQueue: taskQueue)); | ||
|
|
||
| Console.WriteLine("Sending name and title to workflow"); | ||
| await handle.SignalAsync(wf => wf.SignalNameAndTitleAsync("John", "Customer")); | ||
|
|
||
| var name = await handle.QueryAsync(wf => wf.Name); | ||
| var title = await handle.QueryAsync(wf => wf.Title); | ||
|
|
||
| // Send exit signal to workflow | ||
| await handle.SignalAsync(wf => wf.ExitAsync()); | ||
|
|
||
| var result = await handle.GetResultAsync(); | ||
|
|
||
| Console.WriteLine($"Workflow result is {result}"); | ||
|
|
||
| Console.WriteLine("Query results: "); | ||
| Console.WriteLine($"\tName: {name}"); | ||
| Console.WriteLine($"\tTitle: {title}"); | ||
|
|
||
| // Print worker counter info | ||
| Console.WriteLine("\nCollected Worker Counter Info:\n"); | ||
| Console.WriteLine(counterInterceptor.WorkerInfo()); | ||
| Console.WriteLine($"Number of workers: {counterInterceptor.Counts.Count}"); | ||
|
|
||
| // Print client counter info | ||
| Console.WriteLine(); | ||
| Console.WriteLine("Collected Client Counter Info:\n"); | ||
| Console.WriteLine(counterInterceptor.ClientInfo()); | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| Console.WriteLine("Worker cancelled"); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No benefit to making a record, might as well make a class. Also, make sure you put the namespace on this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed