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

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

Merged

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Jan 19, 2024

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

  • Bug fix (non-breaking change which fixes an issue)
  • 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?

  • Adapted existing unit tests

Copy link

codecov bot commented Jan 19, 2024

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%) ⬆️
.../exporter-trace-otlp-grpc/src/OTLPTraceExporter.ts 100.00% <100.00%> (+7.14%) ⬆️
...porter-metrics-otlp-grpc/src/OTLPMetricExporter.ts 100.00% <100.00%> (+6.25%) ⬆️
...-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%) ⬆️
...rter-base/src/create-service-client-constructor.ts 69.23% <69.23%> (ø)
...grpc-exporter-base/src/OTLPGRPCExporterNodeBase.ts 66.66% <65.11%> (-7.15%) ⬇️

... and 4 files with indirect coverage changes

@pichlermarc pichlermarc marked this pull request as ready for review January 26, 2024 12:01
@pichlermarc pichlermarc requested a review from a team January 26, 2024 12:01
@pichlermarc pichlermarc added priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect labels Feb 12, 2024
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just a couple places I think clarifying comments would be a good idea.

Comment on lines -66 to -72
getServiceClientType() {
return ServiceClientType.LOGS;
}

getServiceProtoPath(): string {
return 'opentelemetry/proto/collector/logs/v1/logs_service.proto';
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: breaking change

Comment on lines -66 to -72
getServiceClientType() {
return ServiceClientType.SPANS;
}

getServiceProtoPath(): string {
return 'opentelemetry/proto/collector/trace/v1/trace_service.proto';
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: breaking

Comment on lines -61 to -66
getServiceProtoPath(): string {
return 'opentelemetry/proto/collector/metrics/v1/metrics_service.proto';
}

getServiceClientType(): ServiceClientType {
return ServiceClientType.METRICS;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: breaking

@pichlermarc pichlermarc merged commit 5a033e5 into open-telemetry:main Mar 6, 2024
20 checks passed
@pichlermarc pichlermarc deleted the fix/grpc-lazy-load branch March 6, 2024 11:45
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
* fix(exporter-*-otlp-grpc)!: lazy load gRPC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:otlp-grpc-exporter-base priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Experimental grpc plugin async onInit conflicts with testing norms Lazy load grpc in OTEL grpc exporters
3 participants