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

fix(exporter-*-otlp-grpc)!: lazy load gRPC

Open pichlermarc opened this issue 1 year ago • 1 comments

Which problem is this PR solving?

We've had problems in the past with the gRPC exporters, as they do not lazy-require the @grpc/grpc-js library. This causes problems with @opentelemetry/instrumentation-grpc and @opentelemetry/instrumentation-http. This PR addresses this by re-structuring internals in a way that @grpc/grpc-js is only required on the first export.

Fixes #4151 Fixes #4266 (now loads gRPC on export, instead of the next tick, which means this should not happen anymore IF the SDK/exporter is shut down properly before the test environment is torn down).

Type of change

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Breaking changes

Removes getServiceClientType()

This affects

  • OTLPTraceExporter from @opentelemetry/exporter-trace-otlp-grpc
  • OTLPLogsExporter from @opentelemetry/exporter-logs-otlp-grpc
  • OTLPGRPCExporterNodeBase @opentelemetry/otlp-grpc-exporter-base (intended for internal use only)

It was previously possible to get the service client type (Traces, Metrics, Logs) from the exporter - this is now not possible anymore. This was used internally to construct a service client. Due to the restructuring of the internals in this PR, this is now not needed anymore.

Possible impact: low this returned a static string that did not change per-exporter.

Removes getServiceProtoPath()

This affects

  • OTLPTraceExporter from @opentelemetry/exporter-trace-otlp-grpc
  • OTLPLogsExporter from @opentelemetry/exporter-logs-otlp-grpc
  • OTLPGRPCExporterNodeBase @opentelemetry/otlp-grpc-exporter-base (intended for internal use only)

It was previously possible to get the service proto path used by exporter - this is now not possible anymore. This was used internally to construct a service client. Due to the restructuring of the internals in this PR, this is now not needed anymore.

Possible impact: low this returned a static string that did not change per-exporter.

Removes metadata property, this affects

  • OTLPTraceExporter from @opentelemetry/exporter-trace-otlp-grpc
  • OTLPLogsExporter from @opentelemetry/exporter-logs-otlp-grpc
  • OTLPGRPCExporterNodeBase @opentelemetry/otlp-grpc-exporter-base (intended for internal use only)

It was previously possible to edit metadata while the exporter is running. This is likely not an intended feature as the property was only read in the internal export code and tests, but only written by the exporters themselves.

Possible impact: medium users could have used the metadata property to dynamically modify metadata while the export is running.

Removes serviceClient property, this affects

  • OTLPTraceExporter from @opentelemetry/exporter-trace-otlp-grpc
  • OTLPLogsExporter from @opentelemetry/exporter-logs-otlp-grpc
  • OTLPGRPCExporterNodeBase @opentelemetry/otlp-grpc-exporter-base (intended for internal use only)

This was likely also intended for internal use but unintentionally exposed to users. It allowed full access to the internally used grpc service client.

Possible impact: unknown users could have used the service client directly to modify export behavior.

Removes compression property, this affects

  • OTLPTraceExporter from @opentelemetry/exporter-trace-otlp-grpc
  • OTLPLogsExporter from @opentelemetry/exporter-logs-otlp-grpc
  • OTLPGRPCExporterNodeBase @opentelemetry/otlp-grpc-exporter-base (intended for internal use only)

This was likely also intended for internal use but unintentionally exposed to users. It allowed reading the compression property. It was read when a new service client was created, but changes to it would not affect the export.

Possible impact: low, if needed it can be read from the config that's passed to the constructor

How Has This Been Tested?

  • [x] Adapted existing unit tests

pichlermarc avatar Jan 19 '24 10:01 pichlermarc

Codecov Report

Merging #4432 (50515cc) into main (75bd723) will decrease coverage by 0.07%. The diff coverage is 84.16%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4432      +/-   ##
==========================================
- Coverage   92.39%   92.32%   -0.07%     
==========================================
  Files         330      327       -3     
  Lines        9531     9462      -69     
  Branches     2036     2024      -12     
==========================================
- Hits         8806     8736      -70     
- Misses        725      726       +1     
Files Coverage Δ
...ges/exporter-logs-otlp-grpc/src/OTLPLogExporter.ts 100.00% <100.00%> (+7.14%) :arrow_up:
.../exporter-trace-otlp-grpc/src/OTLPTraceExporter.ts 100.00% <100.00%> (+7.14%) :arrow_up:
...porter-metrics-otlp-grpc/src/OTLPMetricExporter.ts 100.00% <100.00%> (+6.25%) :arrow_up:
...-grpc-exporter-base/src/grpc-exporter-transport.ts 100.00% <100.00%> (ø)
...ental/packages/otlp-grpc-exporter-base/src/util.ts 95.23% <100.00%> (+18.86%) :arrow_up:
...rter-base/src/create-service-client-constructor.ts 69.23% <69.23%> (ø)
...grpc-exporter-base/src/OTLPGRPCExporterNodeBase.ts 66.66% <65.11%> (-7.15%) :arrow_down:

... and 4 files with indirect coverage changes

codecov[bot] avatar Jan 19 '24 10:01 codecov[bot]