Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(exporters)!: collapse base classes into one #5031

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Oct 2, 2024

Which problem is this PR solving?

Through recent changes I've been working on reducing code-duplication across the exporters and separating config-code, transport code and base exporter code. I'm doing this for three reasons:

  • improve how well we can test parts of the exporter
  • address some design-level bugs that we've introduced over the year
  • reduce the public API to maintainable levels

In the changes I made previously, we have seen a convergence of the OTLPExporterNodeBase, OTLPExporterBrowserBase, and OTLPGRPCExporterBase classes. This PR removes all of these classes, and replaces them with an implementation of a new interface: IOTLPExportDelegate

Collapsing these into a single implementation with a single name has the benefit of fixing #4794, which was caused by differently named platform exports for Node.js and the Browser - a design-level bug that needed resolving before we can consider an OTLP Exporter GA release. I'm fixing the problem of having different public interfaces for different transports by introducing new entrypoints: node-http and browser-http which are only imported by their respective platform-specific implementations.

This export delegate contains all the behavior that is present in the OTLP exporter specification and shared across all signals. It can be created via a factory function to avoid exposing the internal API as public. Specific exporters are expected to use this delegate to eventually compose an exporter though different transports and serializers. Composition is used over inheritance as polymorphism is not of relevance when using an OTLP exporter: It is either a PushMetricExporter, a SpanExporter, or a LogRecordExporter - having an OTLPExporterBase does not add any value to the end-user as the public interface ends up being the same. On top of that it was only used internally as it was the easy way of getting base functionality to the individual exporters. In doing so it also exposed a lot of internals as part of the public API.

OTLPExporterBase still exists but now only takes an IOTLPExportDelegate and will eventually be removed. I've decided to keep it around to have a base for the individual exporters, which can be instantiated with new. In a future revision, I plan to introduce factory functions for each exporter which returns a PushMetricExporter, a SpanExporter, or a LogRecordExporter instead of a specific exporter class. The factory functions will also do away with some duplicate ways of configuring transports.

As a way to transition to these factory functions, this PR introduces "translation" helpers for the "old" configuration to the "new" one which is already used internally (these legacy conversions can be identified by the word legacy in their names).

Fixes #4794

Breaking changes

  • feat(otlp-exporter-base)!: collapse base classes into one
    • OTLPExporterNodeBase has been removed in favor of a platform-agnostic implementation (OTLPExporterBase)
    • OTLPExporterBrowserBase has been removed in favor of a platform-agnostic implementation (OTLPExporterBase)
    • ExportServiceError was intended for internal use and has been dropped from exports
    • validateAndNormalizeHeaders was intended for internal use and has been dropped from exports
    • OTLPExporterBase all properties are now private, the constructor now takes an IOTLPExportDelegate, the type parameter for config type has been dropped.
      • This type is scheduled for removal in a future version of this package, please treat all exporters as SpanExporter, PushMetricExporter or LogRecordExporter, based on their respective type.
  • feat(otlp-grpc-exporter-base)!: collapse base classes into one
    • OTLPGRPCExporterNodeBase has been removed in favor of a platform-agnostic implementation (OTLPExporterBase from @opentelemetry/otlp-exporter-base)

TODOS

  • more tests
  • properly labeling deprecated types

Type of change

Please delete options that are not relevant.

  • Bug fix
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Added unit tests, adapted existing tests

Copy link

codecov bot commented Oct 2, 2024

Codecov Report

Attention: Patch coverage is 68.54839% with 39 lines in your changes missing coverage. Please review.

Project coverage is 94.06%. Comparing base (7293e69) to head (3021b96).

Files with missing lines Patch % Lines
...ges/otlp-exporter-base/src/export-promise-queue.ts 6.25% 15 Missing ⚠️
.../configuration/convert-legacy-node-http-options.ts 26.66% 11 Missing ⚠️
...ackages/otlp-exporter-base/src/OTLPExporterBase.ts 16.66% 5 Missing ⚠️
...er-metrics-otlp-http/src/OTLPMetricExporterBase.ts 66.66% 2 Missing ⚠️
...tlp-exporter-base/src/otlp-http-export-delegate.ts 71.42% 2 Missing ⚠️
...-exporter-base/src/otlp-network-export-delegate.ts 60.00% 2 Missing ⚠️
...-grpc-exporter-base/src/grpc-exporter-transport.ts 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5031      +/-   ##
==========================================
+ Coverage   93.26%   94.06%   +0.80%     
==========================================
  Files         317      317              
  Lines        8195     8073     -122     
  Branches     1641     1618      -23     
==========================================
- Hits         7643     7594      -49     
+ Misses        552      479      -73     
Files with missing lines Coverage Δ
...ges/exporter-logs-otlp-grpc/src/OTLPLogExporter.ts 100.00% <100.00%> (ø)
...ogs-otlp-http/src/platform/node/OTLPLogExporter.ts 100.00% <100.00%> (ø)
...gs-otlp-proto/src/platform/node/OTLPLogExporter.ts 100.00% <100.00%> (ø)
.../exporter-trace-otlp-grpc/src/OTLPTraceExporter.ts 100.00% <100.00%> (ø)
...e-otlp-http/src/platform/node/OTLPTraceExporter.ts 100.00% <100.00%> (ø)
...-otlp-proto/src/platform/node/OTLPTraceExporter.ts 100.00% <100.00%> (ø)
...porter-metrics-otlp-grpc/src/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...-otlp-http/src/platform/node/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...orter-metrics-otlp-proto/src/OTLPMetricExporter.ts 100.00% <100.00%> (ø)
...ase/src/configuration/legacy-node-configuration.ts 100.00% <ø> (ø)
... and 14 more

... and 3 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong import for "OTLPExporterNodeBase"
1 participant