Implement SDK metrics for trace #7895
Conversation
| return this; | ||
| } | ||
|
|
||
| /** Sets the {@link InternalTelemetryVersion} defining which metrics this processor records. */ |
There was a problem hiding this comment.
I copied the pattern from #7265 (comment)
I'm a bit confused though since it sounded like the intent was to "After some grace period remove support for the legacy metrics", but with this in the public API it seems not possible (I was expecting to use a system property for the switch). Guessing the ship has sailed but want to doublecheck
There was a problem hiding this comment.
Yes, looks like we have to support the legacy format forever.
I don't understand why we only have LEGACY and LATEST though - and not the 1_31 as proposed by @jack-berg initially.
Latest means you opt in to a breaking change if the semconv changes is the future, so only legacy is really stable.
| * href="https://opentelemetry.io/docs/specs/semconv/otel/sdk-metrics/#span-metrics">semantic | ||
| * conventions</a>. | ||
| */ | ||
| final class SemConvSpanProcessorMetrics implements SpanProcessorMetrics { |
There was a problem hiding this comment.
While the logs version will be almost identical, I think it's not worth trying to abstract something since there is a goal to have less internal shared code, and it's still not that much, with the word span showing up in descriptions in a way that it would get pretty awkward anyways
There was a problem hiding this comment.
I would like less code duplication - but I agree that this repo tolerates a little duplication to make the code easier to read.
| int totalRecordedLinks, | ||
| long startEpochNanos) { | ||
| long startEpochNanos, | ||
| Runnable onEnd) { |
There was a problem hiding this comment.
The name did remind me of span processors, but it seemed we still need this in addition to them since the span metrics aren't tied to any specific processor
There was a problem hiding this comment.
agree - and it wouldn't be easy to add it to the active processor, as the runnable captures the sampling decision from start span.
Maybe it would be easier to follow when the variable would be named recordMetrics.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7895 +/- ##
============================================
+ Coverage 90.08% 90.15% +0.06%
- Complexity 7325 7360 +35
============================================
Files 825 829 +4
Lines 22051 22181 +130
Branches 2179 2182 +3
============================================
+ Hits 19865 19997 +132
+ Misses 1506 1505 -1
+ Partials 680 679 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
I am helping implement the SDK metrics spec. Exporter metrics for all signals have already been implemented, this starts on the tracer side. A very similar PR will come later for logs, and a less similar one for metrics
https://opentelemetry.io/docs/specs/semconv/otel/sdk-metrics/