feat(sdk): implement exporter metrics#6480
feat(sdk): implement exporter metrics#6480anuraaga wants to merge 14 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6480 +/- ##
==========================================
- Coverage 95.73% 95.64% -0.10%
==========================================
Files 364 378 +14
Lines 11826 11979 +153
Branches 2765 2788 +23
==========================================
+ Hits 11322 11457 +135
- Misses 504 522 +18
🚀 New features to boost your workflow:
|
| signal, | ||
| url: options.url, | ||
| meterProvider, | ||
| errorAttributes: (error: unknown) => { |
There was a problem hiding this comment.
Java implementation had an easier time with it since it instruments the transports themselves. I couldn't see how to do it here though because of RetryingTransport. So I went with this hacky, but non-invasive approach.
trentm
left a comment
trentm
left a comment
There was a problem hiding this comment.
@anuraaga Thanks. This is just some early notes. I'm learning the exporter packages. My main thought so far is whether limiting to just the OTLP-exporters could simplify a little bit. Or at least avoid adding to the @opentelemetry/core API to limit the blast radius a bit.
| ); | ||
| } | ||
|
|
||
| startExport(request: Request): (error: unknown) => void { |
There was a problem hiding this comment.
Instead of Request (which is pretty overloaded), perhaps Internal or InternalRepresentation, closer to what is used for the same thing in the otlp-exporter-base package? I'm still learning this code, so this is a fairly naive suggestion. I.e. take with a grain of salt.
experimental/packages/opentelemetry-exporter-metrics-otlp-proto/src/semconv.ts
Show resolved
Hide resolved
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| export interface ISignal<Request> { |
There was a problem hiding this comment.
This interface ISignal and another one in "opentelemetry-core/src/internal/ExporterMetrics.ts" I think could have a better name. These are specific to exporter SDK Metrics.
|
@anuraaga We discussed on the JS SIG call today: both the Jaeger and Zipkin exporters are deprecated. We don't need to add new features to them. It was mentioned that the Prometheus exporter is a non-OTLP exporter, however. Should that exporter be getting an implementation of any of the |
…y-js into exporter-metrics
|
Thanks @trentm - this is probably as good a time to mark as ready for review though not surprised if there are still cleanups needed. Notably, allowing metric exporter to be wired up was much trickier than I wished, though not sure of any alternative. Also, unlike the other PRs this one is pretty huge so I am planning on wiring up in opentelemetry-sdk-node in a separate PR after this one.
Prometheus is a |
2 participants
Which problem is this PR solving?
I am helping implement SDK metrics.
https://opentelemetry.io/docs/specs/semconv/otel/sdk-metrics/#metric-otelsdkexporterspaninflight
This PR adds metrics for exporters. otel-js has a looot of exporters so this PR is pretty big and somewhat complex. I tried to design things as sensibly as possible but happy to hear any suggestions.
I have added important tests but there are still more tests to add for sanity checking the actual exporter factories, and also need to add zipkin. But as this PR is quite complex, it will be great to get an initial review while working on it.
/cc @trentm
Short description of the changes
Add metrics for export operations
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: