Improved .NET metrics support #129
Conversation
| Bool, | ||
| } | ||
|
|
||
| internal enum MetricIntegerKind |
There was a problem hiding this comment.
I'd just call this MetricKind. If/when we accept non-integer values, the kinds will become generic over the value types.
There was a problem hiding this comment.
This is only used for MetricIntegerOptions on a call to metric_integer_new which returns a MetricInteger. It'd make no sense to have to make metric_integer_new/MetricIntegerOptions support a non-metric-integer kind. If there's a metric_double_new/MetricDoubleOptions, it'd have MetricDoubleKind because it'd make no sense for such a method to support metric-integer kinds. The separation of metrics by their accepted type is due to the inability to expose generics in C.
There was a problem hiding this comment.
Huh? Why wouldn't metric_integer_new and metric_double_new be able to just accept MetricKind? All the kinds would be supported.
There was a problem hiding this comment.
If that is the case I can refactor then. My concern was that every kind may not be supported by every data type. For right now there is only one, so we can just wait until another comes and then I can share if all are supported, sure.
| /// </summary> | ||
| /// <param name="attributes">Attributes to append.</param> | ||
| /// <returns>New attributes with the given ones appended.</returns> | ||
| public MetricAttributes Append(IEnumerable<KeyValuePair<string, object>> attributes) |
There was a problem hiding this comment.
You'll probably want to add a method which uses the merge function exposed on attributes. This is the most efficient way (when using lang-side exporting) as it re-uses the IDs and doesn't need to allocate any new attributes - and it's fairly common to build up some attributes as a series of combined ones.
There was a problem hiding this comment.
This is internal code not user code. There is no exposed user code for an attribute class, so there is no value in a "merge" in internal code. It would never be called.
There was a problem hiding this comment.
WithTags could work in terms of merging is I suppose what I really mean, whereas just recording and passing the tags there wouldn't. WithTags implying you're going to keep around that new meter for a while.
There was a problem hiding this comment.
Given the .NET API currently, what would merging look like from a user POV? We don't expose MetricAttributes to users, we only expose WithTags that only ever needs to append.
There was a problem hiding this comment.
It wouldn't be any different - I mean WithTags can work by calling merge.
There was a problem hiding this comment.
But there is no real difference between "clone + create MetricsAttributes + merge" and just "clone + add_new_attrs" . It's all just clone + vec extend with how I use this. I don't allow metrics attributes to be mutable at this time because I don't give users the concept of a standalone set of pre-created attributes because it's usually not worth it (and the .NET API at https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.metrics works this way as well though they go one further and make you provide the full set every record call).
| internal MetricMeter(Bridge.MetricMeter meter, Bridge.MetricAttributes attributes) | ||
| { | ||
| Meter = meter; | ||
| this.attributes = attributes; |
There was a problem hiding this comment.
This is sort of more like defaultAttributes?
There was a problem hiding this comment.
Not really. You can call WithTags which makes a new meter with new tags. This is more like "base attributes", but it's pretty clear that "attributes" means the attributes on all metrics created from this meter just like "attributes" on the metric is for all values recorded on the metric.
| /// <inheritdoc /> | ||
| public IMetricMeter MetricMeter => metricMeter.Value; |
There was a problem hiding this comment.
Do we really want to expose a way to record metrics directly from the workflow? I can certainly see how it's useful but I definitely don't love adding things that do side effects in the middle of workflow code.
There was a problem hiding this comment.
Yes, but it is replay safe just like loggers and tracing. Our other SDKs have this and is a primary parity point we want to achieve with this project IMO.
What was changed
TemporalRuntime.MetricMeterfor users to add/record metricsActivityExecutionContext.MetricMeterfor users to add/record metrics from activitiesWorkflow.MetricMeterfor users to add/record metrics from workflows (replay safe)MetricsOptions,PrometheusOptions, andOpenTelemetryOptionsto support all current metric options from coreTemporalio.Runtime.TelemetryOptions.Tracingproperty and associated classThis is only part of the things needed for #126. This notably doesn't include a user-defined implementation for metrics handling which will come later.
Also, this PR will wait on temporalio/sdk-core#598 which will then change a unit test here.