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.

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

Added Counter Interceptor Sample #77

Merged
merged 20 commits into from
Aug 20, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ Prerequisites:
* [ActivityWorker](src/ActivityWorker) - Use .NET activities from a workflow in another language.
* [AspNet](src/AspNet) - Demonstration of a generic host worker and an ASP.NET workflow starter.
* [ClientMtls](src/ClientMtls) - How to use client certificate authentication, e.g. for Temporal Cloud.
* [CounterInterceptor](src/CounterInterceptor/) - Simple Workflow and Client Interceptors example.
rross marked this conversation as resolved.
Show resolved Hide resolved
* [ContextPropagation](src/ContextPropagation) - Context propagation via interceptors.
* [DependencyInjection](src/DependencyInjection) - How to inject dependencies in activities and use generic hosts for workers
* [Encryption](src/Encryption) - End-to-end encryption with Temporal payload codecs.
Expand Down
89 changes: 89 additions & 0 deletions src/CounterInterceptor/ClientCounter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
namespace TemporalioSamples.CounterInterceptor;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.


using System.Numerics;

public class ClientCounter
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored.

{
private const string NumberOfWorkflowExecutions = "numOfWorkflowExec";
private const string NumberOfSignals = "numOfSignals";
private const string NumberOfQueries = "numOfQueries";
private static Dictionary<string, Dictionary<string, BigInteger?>> perWorkflowIdDictionary =
rross marked this conversation as resolved.
Show resolved Hide resolved
rross marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be static (or the workflow counter). Static mutable state is bad. Just have the interceptor have an exposed property with its values and reference it from the interceptor.

new Dictionary<string, Dictionary<string, BigInteger?>>();
rross marked this conversation as resolved.
Show resolved Hide resolved

public static string Info()
{
string result = string.Empty;
foreach (var item in perWorkflowIdDictionary)
rross marked this conversation as resolved.
Show resolved Hide resolved
{
var info = item.Value;
result = result +
"\n** Workflow ID: " + item.Key +
"\n\tTotal Number of Workflow Exec: " + info[NumberOfWorkflowExecutions] +
"\n\tTotal Number of Signals: " + info[NumberOfSignals] +
"\n\tTotal Number of Queries: " + info[NumberOfQueries];
}

return result;
}

public static BigInteger? NumOfWorkflowExecutions(string workflowId)
Copy link
Member

Choose a reason for hiding this comment

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

Assuming the field is properly changed to private static Dictionary<string, WorkflowCounts> workflowCounts, a simple public static WorkflowCounts? GetWorkflowCounts(string workflowId) => workflowCounts.TryGetValue(workflowId, out var counts) ? counts : null will work

{
return perWorkflowIdDictionary[workflowId][NumberOfWorkflowExecutions];
}

public static BigInteger? NumOfSignals(string workflowId)
{
return perWorkflowIdDictionary[workflowId][NumberOfSignals];
}

public static BigInteger? NumOfQueries(string workflowId)
{
return perWorkflowIdDictionary[workflowId][NumberOfQueries];
}

public void AddStartInvocation(string workflowId)
{
Add(workflowId, NumberOfWorkflowExecutions);
}

public void AddSignalInvocation(string workflowId)
{
Add(workflowId, NumberOfSignals);
}

public void AddQueryInvocation(string workflowId)
{
Add(workflowId, NumberOfQueries);
}

// Creates a default counter info map for a workflowid
private static Dictionary<string, BigInteger?> GetDefaultInfoMap()
{
return new Dictionary<string, BigInteger?>()
{
{ NumberOfWorkflowExecutions, 0 },
{ NumberOfSignals, 0 },
{ NumberOfQueries, 0 },
};
}

private void Add(string workflowId, string type)
{
if (!perWorkflowIdDictionary.TryGetValue(workflowId, out Dictionary<string, BigInteger?>? value))
{
value = GetDefaultInfoMap();
perWorkflowIdDictionary.Add(workflowId, value);
}

if (value[type] == null)
{
value[type] = 1;
}
else
{
var current = value[type];
var next = current + 1;
value[type] = next;
}
rross marked this conversation as resolved.
Show resolved Hide resolved
}
}
7 changes: 7 additions & 0 deletions src/CounterInterceptor/Constants.cs
Copy link
Member

Choose a reason for hiding this comment

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

This file is likely not needed. Non-test code only uses each of these constants in one file, so put them there (test code should be using more random names to avoid clashing with other test runs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Removed

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
namespace TemporalioSamples.CounterInterceptor;

public static class Constants
{
public const string TaskQueue = "counter-interceptor-sample";
public const string ChildWorkflowId = "TestInterceptorChildWorkflow";
}
19 changes: 19 additions & 0 deletions src/CounterInterceptor/MyActivities.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
namespace TemporalioSamples.CounterInterceptor;

using System.Diagnostics;
using Temporalio.Activities;

public class MyActivities
{
[Activity]
public string SayHello(string name, string title)
{
return "Hello " + title + " " + name;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public string SayHello(string name, string title)
{
return "Hello " + title + " " + name;
}
public string SayHello(string name, string title) => $"Hello {title} {name}";

Use string interpolation and single-statement syntax (will stop commenting on this, but it applies project wide)

Copy link
Member

Choose a reason for hiding this comment

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

Bump, this was not addressed


[Activity]
public string SayGoodBye(string name, string title)
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

{
return "Goodbye " + title + " " + name;
}
}
21 changes: 21 additions & 0 deletions src/CounterInterceptor/MyChildWorkflow.workflow.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
namespace TemporalioSamples.CounterInterceptor;

using Temporalio.Workflows;

[Workflow]
public class MyChildWorkflow
{
private readonly ActivityOptions activityOptions = new()
{
StartToCloseTimeout = TimeSpan.FromSeconds(10),
};

[WorkflowRun]
public async Task<string> ExecChildAsync(string name, string title)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public async Task<string> ExecChildAsync(string name, string title)
public async Task<string> RunAsync(string name, string title)

Not required obviously, but I think calling all run calls RunAsync is clearest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree. Fixed.

{
string result = await Workflow.ExecuteActivityAsync((MyActivities act) => act.SayHello(name, title), activityOptions);
result += await Workflow.ExecuteActivityAsync((MyActivities act) => act.SayGoodBye(name, title), activityOptions);

return result;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public async Task<string> ExecChildAsync(string name, string title)
{
string result = await Workflow.ExecuteActivityAsync((MyActivities act) => act.SayHello(name, title), activityOptions);
result += await Workflow.ExecuteActivityAsync((MyActivities act) => act.SayGoodBye(name, title), activityOptions);
return result;
}
public Task<string> ExecChildAsync(string name, string title) =>
await Workflow.ExecuteActivityAsync(
(MyActivities act) => act.SayHello(name, title), activityOptions) +
await Workflow.ExecuteActivityAsync(
(MyActivities act) => act.SayGoodBye(name, title), activityOptions);

This reads clearer to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Fixed.

}
56 changes: 56 additions & 0 deletions src/CounterInterceptor/MyWorkflow.workflow.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
namespace TemporalioSamples.CounterInterceptor;

using Temporalio.Workflows;

[Workflow]
public class MyWorkflow
{
private string name = string.Empty;
private string title = string.Empty;
private bool exit; // automatically defaults to false

[WorkflowRun]
public async Task<string> ExecAsync()
{
// wait for greeting info
await Workflow.WaitConditionAsync(() => name != null && title != null);
Copy link
Member

Choose a reason for hiding this comment

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

This condition always passes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand what you mean. Can you clarify please?

Copy link
Member

Choose a reason for hiding this comment

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

Name and title are never null, so what is the purpose of this condition?


// Execute Child Workflow
string result = await Workflow.ExecuteChildWorkflowAsync(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
string result = await Workflow.ExecuteChildWorkflowAsync(
var result = await Workflow.ExecuteChildWorkflowAsync(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

(MyChildWorkflow wf) => wf.ExecChildAsync(name, title),
new()
{
Id = Constants.ChildWorkflowId,
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
new()
{
Id = Constants.ChildWorkflowId,
});
new() { Id = Constants.ChildWorkflowId });

Don't have to span lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


// Wait for exit signal
await Workflow.WaitConditionAsync(() => exit != false);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
await Workflow.WaitConditionAsync(() => exit != false);
await Workflow.WaitConditionAsync(() => exit);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


return result;
}

[WorkflowSignal]
public async Task SignalNameAndTitleAsync(string name, string title)
{
this.name = name;
this.title = title;
}

[WorkflowQuery]
public string QueryName()
{
return name;
}

[WorkflowQuery]
public string QueryTitle()
{
return title;
}
Copy link
Member

Choose a reason for hiding this comment

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

Remove the private fields and use properties. Queries are built to be property getters:

Suggested change
[WorkflowQuery]
public string QueryName()
{
return name;
}
[WorkflowQuery]
public string QueryTitle()
{
return title;
}
[WorkflowQuery]
public string Name { get; private set; } = string.Empty;
[WorkflowQuery]
public string Title { get; private set; } = string.Empty;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very cool. Fixed.


[WorkflowSignal]
public async Task ExitAsync()
{
this.exit = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public async Task ExitAsync()
{
this.exit = true;
}
public async Task ExitAsync() => exit = true;

No need for all the extra lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}
108 changes: 108 additions & 0 deletions src/CounterInterceptor/Program.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
namespace TemporalioSamples.CounterInterceptor;

using Temporalio.Client;
using Temporalio.Worker;

internal class Program
{
private static async Task Main(string[] args)
{
static string GetEnvVarWithDefault(string envName, string defaultValue)
{
string? value = Environment.GetEnvironmentVariable(envName);
if (string.IsNullOrEmpty(value))
{
return defaultValue;
}
return value;
}

var address = GetEnvVarWithDefault("TEMPORAL_ADDRESS", "127.0.0.1:7233");
var temporalNamespace = GetEnvVarWithDefault("TEMPORAL_NAMESPACE", "default");
var tlsCertPath = GetEnvVarWithDefault("TEMPORAL_TLS_CERT", string.Empty);
var tlsKeyPath = GetEnvVarWithDefault("TEMPORAL_TLS_KEY", string.Empty);
Copy link
Member

Choose a reason for hiding this comment

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

This is not something we do with our other samples. If/when we come back and make samples work across many environments, we will update them all. No need to have just this sample accept environment variables unlike others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get what you are saying and really wish we did this everywhere...

Copy link
Member

Choose a reason for hiding this comment

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

We plan to

TlsOptions? tls = null;
if (!string.IsNullOrEmpty(tlsCertPath) && !string.IsNullOrEmpty(tlsKeyPath))
rross marked this conversation as resolved.
Show resolved Hide resolved
{
tls = new()
{
ClientCert = await File.ReadAllBytesAsync(tlsCertPath),
ClientPrivateKey = await File.ReadAllBytesAsync(tlsKeyPath),
};
}

var client = await TemporalClient.ConnectAsync(
options: new(address)
{
Namespace = temporalNamespace,
Tls = tls,
Interceptors = new[]
{
new SimpleClientCallsInterceptor(),
Copy link
Member

Choose a reason for hiding this comment

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

You should store this as a field and access the counters off of it instead of making counters static

},
});

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

Choose a reason for hiding this comment

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

This program is a run then complete, you don't need ctrl+c support as if it was a long running worker. I would recommend just removing cancel token and shutting down the worker manually if you must have the entire program self-contained (i.e. combining client and worker).

Copy link
Member

Choose a reason for hiding this comment

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

Bump, this was never addressed

Copy link
Member

Choose a reason for hiding this comment

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

Bump again, this is still not addressed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

{
tokenSource.Cancel();
eventArgs.Cancel = true;
};

var activities = new MyActivities();

var workerOptions = new TemporalWorkerOptions(Constants.TaskQueue).
AddAllActivities(activities).
AddWorkflow<MyWorkflow>().
AddWorkflow<MyChildWorkflow>();

workerOptions.Interceptors = new[] { new SimpleCounterWorkerInterceptor() };
Copy link
Member

Choose a reason for hiding this comment

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

You should consider combining these interceptors. Make the one interceptor implement both IClientInterceptor and IWorkerInterceptor. The it's automatically used for worker interceptors and you don't have to set this setting. This is what we recommend here and is documented on the Interceptors field for client. See the https://github.com/temporalio/samples-dotnet/tree/main/src/ContextPropagation sample.


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
Copy link
Member

Choose a reason for hiding this comment

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

Inconsistent with whether you capitalize comments

var handle = await client.StartWorkflowAsync(
(MyWorkflow wf) => wf.ExecAsync(),
new(id: Guid.NewGuid().ToString(), taskQueue: Constants.TaskQueue));

Console.WriteLine("Sending name and title to workflow");
await handle.SignalAsync(wf => wf.SignalNameAndTitleAsync("John", "Customer"));

var name = await handle.QueryAsync(wf => wf.QueryName());
var title = await handle.QueryAsync(wf => wf.QueryTitle());

// send exit signal to workflow
await handle.SignalAsync(wf => wf.ExitAsync());

var result = await handle.GetResultAsync();

Console.WriteLine($"Workflow result is {result}", result);
Copy link
Member

Choose a reason for hiding this comment

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

Inconsistent with whether you use string concatenation or string interpolation

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Console.WriteLine($"Workflow result is {result}", result);
Console.WriteLine($"Workflow result is {result}");

or

Suggested change
Console.WriteLine($"Workflow result is {result}", result);
Console.WriteLine($"Workflow result is {0}", result);

But not both. This mistake is made in other lines below. Probably the former approach is best.

Copy link
Member

Choose a reason for hiding this comment

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

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


Console.WriteLine("Query results: ");
Console.WriteLine("Name: " + name);
Console.WriteLine("Title: " + title);

// print worker counter info
Console.WriteLine("Collected Worker Counter Info: ");
Console.WriteLine(WorkerCounter.Info());

// print client counter info
Console.WriteLine();
Console.WriteLine("Collected Client Counter Info:");
Console.WriteLine(ClientCounter.Info());
}
catch (OperationCanceledException)
{
Console.WriteLine("Worker cancelled");
}
}
}
28 changes: 28 additions & 0 deletions src/CounterInterceptor/README.md
rross marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# dotnet-counter-interceptor
Copy link
Member

Choose a reason for hiding this comment

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

Not really the best title, but that's ok

The sample demonstrates:
- the use of a simple Worker Workflow Interceptor that counts the number of Workflow Executions, Child Workflow Executions, and Activity Executions as well as the number of Signals and Queries. It is based
off of the [Java Sample](https://github.com/temporalio/samples-java/tree/main) located [here](https://github.com/temporalio/samples-java/tree/main/core/src/main/java/io/temporal/samples/countinterceptor)
- the use of a simple Client Workflow Interceptor that counts the number of Workflow Executions as well as the number of Signals and Queries.

## Start local Temporal Server
```bash
# run only once
temporal server start-dev
```
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Start local Temporal Server
```bash
# run only once
temporal server start-dev
```
To run, first see [README.md](https://github.com/temporalio/samples-dotnet/blob/main/README.md) for prerequisites

We like to keep the starting of the local server in one place since it can change and there are multiple options. This can be seen in other samples. (in fact, we will make these run easy on cloud too soon, so we don't want to have to come back and update these)

Copy link
Member

Choose a reason for hiding this comment

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

Bump, this was never addressed

Copy link
Member

Choose a reason for hiding this comment

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

Bump again, this is still not addressed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally addressed :)


## Run Worker Locally
Copy link
Member

Choose a reason for hiding this comment

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

This is more than the worker, it's the worker and the client calls mixed into one. Many samples split these, but don't have to here, but it's not just "worker" as the title suggests.

Copy link
Member

Choose a reason for hiding this comment

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

Bump, this was never addressed

Copy link
Member

Choose a reason for hiding this comment

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

Bump again, this is still not addressed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

```bash
# make sure you have temporal server running (see section above)
dotnet run
```

## Run Worker using Temporal Cloud
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this section applies anymore

Copy link
Member

Choose a reason for hiding this comment

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

Bump, this was never addressed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted.

```bash
# set up environment variables
export TEMPORAL_NAMESPACE=<namespace>.<accountId>
export TEMPORAL_ADDRESS=<namespace>.<accountId>.tmprl.cloud:7233
export TEMPORAL_TLS_CERT=/path/to/cert
export TEMPORAL_TLS_KEY=/path/to/key
# run the worker
dotnet run
```
Loading