Add OpenTelemetry Sample #78
Conversation
src/OpenTelemetry/Program.cs
Outdated
|
|
||
| using var meter = new Meter(assemblyName.Name!, assemblyName.Version!.ToString()); | ||
|
|
||
| string instanceId = args.ElementAtOrDefault(0) ?? throw new ArgumentException("Must pass 'worker' or 'workflow' as the single argument"); |
There was a problem hiding this comment.
A little bit of repetition here, but it matters greatly for the OpenTelemetry sample in terms of correlating telemetry coming from the worker or client.
| aspire-dashboard: | ||
| environment: | ||
| Dashboard__Frontend__AuthMode: Unsecured | ||
| image: mcr.microsoft.com/dotnet/aspire-dashboard:8.0 |
There was a problem hiding this comment.
I did see that the python samples are using the Jaeger image and not the Aspire Dashboard, which is developed by Microsoft: https://github.com/temporalio/samples-python/tree/main/open_telemetry
I can change to the Jaeger image if desired.
The main benefit of using the Aspire dashboard over Jaeger is that it provides visualizations for metrics, which iirc Jaeger does not. I think using a Microsoft image is probably okay specifically for .NET
There was a problem hiding this comment.
I don't mind this, I like the metrics, so I think this image is great. Alternatively, you can get rid of docker compose and just tell people how to start the this docker container directly in the README, but this is fine too.
There was a problem hiding this comment.
Imo, docker compose is easier with a single command, since environment variables and port forwarding are involved
src/OpenTelemetry/Program.cs
Outdated
| { | ||
| Metrics = new MetricsOptions() | ||
| { | ||
| CustomMetricMeter = new CustomMetricMeter(meter), |
There was a problem hiding this comment.
Hrmm, so there are two ways to configure OTel telemetry metrics. One way is this, but the other way is simply to set the OTel options in this metric options so our internal code forwards. We often encourage the latter because it's less setup and it matches other SDKs.
I wonder if there is some way we can show both? Like maybe switched from one to the other via CLI flag?
There was a problem hiding this comment.
I split this into two samples, OpenTelemetry/CoreSdkFowarding and OpenTelemetry/DotNetMetrics.
I thought this was cleanest, and follows a similar pattern that exists with the polling samples
There was a problem hiding this comment.
Ok, will look. Alternatively we probably could have just had a different parameter accepted in the Program.cs to choose one approach or the other instead of completely separate projects.
src/OpenTelemetry/Activities.cs
Outdated
| public static class Activities | ||
| { | ||
| [Activity] | ||
| public static void MyActivity(string input) |
There was a problem hiding this comment.
I think it'd be valuable here and in workflow to show a custom metric. E.g. ActivityExecutionContext.Current.MetricMeter.CreateCounter<int>("my-counter").Add(123) and Workflow.MetricMeter.CreateCounter<int>("my-counter").Add(123). Technically you could instantiate the activity class with the .NET mertic meter, but this way respects the existing abstraction (so you don't have to have custom meter), and for workflows you can't inject anything and it's a replay-safe metric meter (i.e. it doesn't record on replay).
There was a problem hiding this comment.
Is this file needed? We don't usually add to the rest of our samples
There was a problem hiding this comment.
Not needed, removed.
launchSettings are a convenience that popular .NET IDEs such as Rider, VS, and VSCode support that allow developers to pre-configure executables launched directly in the IDE with command line arguments, environment variables, etc
| aspire-dashboard: | ||
| environment: | ||
| Dashboard__Frontend__AuthMode: Unsecured | ||
| image: mcr.microsoft.com/dotnet/aspire-dashboard:8.0 |
There was a problem hiding this comment.
I don't mind this, I like the metrics, so I think this image is great. Alternatively, you can get rid of docker compose and just tell people how to start the this docker container directly in the README, but this is fine too.
There was a problem hiding this comment.
Sorry it took so long to get back, I was OOO last week. Overall LGTM. Would like to have OTel tracing in both and I really, I think it's clearer to have just one project w/ metric config switched based on command line (if you can't choose which to default, make the CLI arg required to pick one, e.g. --dotnet-metrics and --core-metrics or something).
src/OpenTelemetry/CoreSdkForwarding/TemporalioSamples.OpenTelemetry.CoreSdkForwarding.csproj
Outdated
Show resolved
Hide resolved
|
@robcao - assuming you don't mind, I am going to work in your branch to get this over the finish line. I think it may just be a matter of merging main (fixing conflicts) and a few extra things. |
# Conflicts: # Directory.Build.props # README.md # TemporalioSamples.sln
Yeah, I don't mind. Sorry about the unintentional abandonment of the pull request, I had meant to get back to it, but life just got in the way. I don't mind collapsing the samples into a single project file. My original thought was that the two different techniques used to gather metrics have different dependencies, so that putting them together in the same project file makes it harder to understand the minimal set of dependencies, but I am ok either way. |
All good, I think we're ready for merge, thanks for the contribution!
I kept them separate for now, it works fine I think |
What was changed
Added OpenTelemetry sample.
Why?
It was missing and requested.
Checklist
Closes
Sample request: OpenTelemetry #67
How was this tested:
Added screenshots similar to the python sample