Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #773 +/- ##
=======================================
+ Coverage 75.9% 76.2% +0.2%
=======================================
Files 69 69
Lines 5510 5576 +66
=======================================
+ Hits 4187 4253 +66
Misses 1323 1323 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lquerel
left a comment
There was a problem hiding this comment.
Two minor nits, otherwise this looks good to me. Thanks!
crates/weaver_emit/src/metrics.rs
Outdated
| histogram.record(1.0, &attributes); | ||
| } | ||
| }, | ||
| _ => match instrument { |
There was a problem hiding this comment.
Why not explicitly specify the second branch?
I agree, it’s unlikely, but if we ever introduce a third variant (both in OTLP and Semconv), the compiler won’t help us do the right thing.
| fn json_to_value_type(value: &Value) -> Option<MetricValueTypeSpec> { | ||
| match value { | ||
| Value::Number(num) => { | ||
| if num.is_i64() { |
There was a problem hiding this comment.
Overall this looks good, if we agree on the direction of enforcing types in weaver.
I think in our data model for OTLP, we don't actually limit metrics to remain the same type for a stream.
For example, from https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/data-model.md, it says:
Within certain data point types (e.g.,
SumandGauge) there is variation permitted in the numeric point value; in this case, the associated variation (i.e., floating-point vs. integer) is not considered identifying.
This makes me think we should NOT be enforcing types in weaver generally.
|
@jsuereth - OK. I won't merge this then until we have a decision. Perhaps we have the |
This PR partially addresses: open-telemetry/semantic-conventions#591
value_typetometricgroup type.value_typeis not specified.registry emituses thevalue_typeto produce correct valuesregistry live-checkcompares the incoming value type from data-points and exemplars to produces avalue_type_mismatchadvice withinformationlevel.