opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

OTLP metric exporters implementation

Open legendecas opened this issue 3 years ago • 3 comments
trafficstars

Currently, we have OTLP trace exporters to be implemented in an inherited class pattern:

OTLPTraceExporter (grpc) -> OTLPGRPCExporterNodeBase -> OTLPExporterBase
OTLPTraceExporter (HTTP)-> OTLPExporterNodeBase -> OTLPExporterBase
OTLPTraceExporter (proto) -> OTLPProtoExporterNodeBase -> OTLPExporterNodeBase -> OTLPExporterBase

On the other hand, the OTLP metric exporters are not in this pattern. The base OTLP metric exporter, a MetricExporter, has an internal metric exporter, which also implements the MetricExporter interface. This pattern looks abstruse to me.

Can the OTLP metric exporter be implemented without the intermediate exporter?

/cc @pichlermarc

legendecas avatar Sep 26 '22 15:09 legendecas

Yes implementing this way should be possible. I originally introduced this as to not bloat up the PRs (#2871, #2893), as they were already way too large at that time.

The reason for that was that the OTLP Exporter expects a list of objects passed to the export() function. This worked for the Trace exporters as they are expecting a list of spans, but not for the Metrics Exporters, that pass a single ResourceMetrics instance. My solution for that was to introduce this Proxy, which kept the diff manageable, but looks, as you already said, abstruse and is far from the optimal solution. :slightly_frowning_face:

My original intention was always to follow up with more refactoring PRs to simplify the exporters (and their tests), but I unfortunately never came around to doing it. If needed, I can prioritize this. :slightly_smiling_face:

pichlermarc avatar Sep 27 '22 06:09 pichlermarc

My original intention was always to follow up with more refactoring PRs to simplify the exporters (and their tests), but I unfortunately never came around to doing it. If needed, I can prioritize this. 🙂

No, not in a hurry as it is working now. I was opening this issue to see if this is intentional and if not we can track the progress here. Thank you for the clarification.

legendecas avatar Sep 27 '22 07:09 legendecas

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] avatar Nov 28 '22 06:11 github-actions[bot]

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] avatar Jan 30 '23 06:01 github-actions[bot]