Support duration and float metrics #223
Conversation
| { | ||
| // Have to convert TimeSpan to something .NET meter can work with. For this to even | ||
| // happen, a user would have had to set custom options to report as time span. | ||
| if (typeof(T) == typeof(TimeSpan)) |
There was a problem hiding this comment.
This whole non-erased generic type check thing feels deeply illegal to me 😆
There was a problem hiding this comment.
Reified generics are so nice in .NET compared to Java's type erasure. This is why you can call AddWorkflow<MyWorkflowClass>() and similar. But there are some downsides.
| @@ -51,18 +51,44 @@ internal class MetricInteger : SafeHandle | |||
| /// </summary> | |||
| /// <param name="value">Value to record.</param> | |||
| /// <param name="attributes">Attributes to set.</param> | |||
| public void Record(ulong value, MetricAttributes attributes) | |||
| public void RecordInteger(ulong value, MetricAttributes attributes) | |||
There was a problem hiding this comment.
Rather than allowing all record kinds on one Metric type it seems like it'd be pretty easy to hand out specialized versions with only one record method each corresponding to the kind passed in when created
There was a problem hiding this comment.
This is not public user-facing code. We do it in a .NET idiomatic way when it comes to the actual user-facing code. But it is nice for the C FFI if we do combine these into fewer calls and expect the caller be smart.
| let meter = &*(self.meter_impl.0); | ||
| (meter.metric_record_duration)( | ||
| self.metric, | ||
| value.as_millis().try_into().unwrap_or(u64::MAX), |
There was a problem hiding this comment.
I can't decide in the rare case that this happens if it's better to record this max or simply not record anything at all
There was a problem hiding this comment.
I was stuck a bit here too and just settled on this. Hopefully the issue never arises. But if we think it's enough of an issue, we can look info logging instead.
This gets even worse when we convert back to .NET types because. E.g. for .NET a ulong (i.e. u64) is "not CLS-compliant", so it's best to use long (i.e. i64) instead, so I limit to max there too (default unchecked cast wraps into negative). This is much more likely of a problem, but I can't currently think of a case where Core side will go over max-i64.
|
@Sushisource - merged based on approval (waited a day after my most recent comment responses), but we can revisit any of the pieces if you think they are important. |
What was changed
MetricMeter.CreateHistogramto provide floating point types orTimeSpanin addition to already-supported integer typesMetricMeter.CreateGaugeto provide floating point types in addition to already-supported integer typesOpenTelemetryOptions.UseSecondsForDurationoption to have OTel metrics use float seconds instead of msPrometheusOptions.UseSecondsForDurationoption to have Prom metrics use float seconds instead of msICustomMetricMeter.CreateHistogramnow supportsdoubleandTimeSpan(for those creating custom meter impls)ICustomMetricMeter.CreateGaugenow supportsdouble(for those creating custom meter impls)MetricsOptions.CustomMetricMeterOptionswhich accepts an option type whose only field isHistogramDurationFormatto specify whether meter impls will get duration metrics as integer milliseconds, float seconds, or time spansChecklist