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

Compile protos statically instead of importing them dynamically #2887

Closed
markandrus opened this issue Apr 5, 2022 · 2 comments
Closed

Compile protos statically instead of importing them dynamically #2887

markandrus opened this issue Apr 5, 2022 · 2 comments

Comments

@markandrus
Copy link

markandrus commented Apr 5, 2022

@opentelemetry/exporter-trace-otlp-grpc uses @grpc/proto-loader to load protos at runtime, dynamically producing a package definition that can be passed to grpc.loadPackageDefinition. The relevant code can be found here.

The problem is that @grpc/proto-loader and these external proto files don't play nicely with bundlers like esbuild, webpack, etc. The issue can be hacked around (see #2786 (comment)), but it's quite messy.

Describe the solution you'd like

The protos @opentelemetry/exporter-trace-otlp-grpc uses are static and so, instead of dynamically producing a package definition at runtime, why not statically produce a package definition at compile time?

grpc-tools, from the same people as @grpc/proto-loader, provides a command-line tool, grpc_tools_node_protoc. According to their README.md, it

Generates code that does not require any gRPC library, and instead generates PackageDefinition objects that can be passed to the loadPackageDefinition function provided by both the grpc and @grpc/grpc-js libraries.

I don't have experience with gRPC in Node.js, but I expect that, leveraging grpc-tools as a development dependency, we could stop shipping proto files inside of @opentelemetry/exporter-trace-otlp-grpc. Instead, we would depend on pre-compiled JavaScript (the package definition). Bundlers like esbuild, webpack, etc., would automatically work.

There's one thing to note about grpc-tools: it installs arch × OS-dependent binaries, and it currently lacks arm64 builds for darwin (see grpc/grpc-node#1405). This means it wouldn't be easy to just clone the repo and compile the protos into package definitions on any machine. Instead, if the protos are changing rarely, perhaps the package definitions should be compiled and checked in by a committer with access to an amd64 machine.

Update 1: I started implementing this. As far as I can tell, it is workable generating for grpc_js, but not with generate_package_definition. It also requires a change in the way the protobuf messages are constructed. For example, right now, @opentelemetry/exporter-trace-otlp-grpc depends on @opentelemetry/exporter-trace-otlp-http in order to use these types and toOTLPExportTraceServiceRequest. Instead, I think it would make sense to remove the dependency on @opentelemtry/exporter-trace-otlp-http, since those types can be generated by ts-protoc-gen. Additionally, we'd need to reimplement toOTLPExportTraceServiceRequest in @opentelemetry/exporter-trace-otlp-grpc in terms of low-level, generated methods (setResourceSpans, etc.).

Update 2: Looks like something similar was attempted with pbjs in #2691 and then ultimately hand-rolled in #2746. The new solution will use the JSON instead of binary protobuf encoding (?), although there are some questions around stability of names in the JSON encoding. Inter-package dependencies will change as well. It seems there's active work here, and I should just wait.

Describe alternatives you've considered

  • Abandon esbuild in order to use @opentelemetry/exporter-trace-otlp-grpc. Uncompressed, my build artifacts would go from a few megabytes to hundreds of megabytes.
  • Try using @opentelemetry/exporter-trace-otlp-proto instead (it loads proto files differently and so may need fewer hacks to get working in my environment, but still not nice).

Additional context

This change would be relevant to @opentelemetry/exporter-trace-otlp-proto and any other OpenTelemetry JavaScript library that uses (static) protos.

@dyladan
Copy link
Member

dyladan commented Apr 6, 2022

Similar work is already ongoing here. We created the otlp-transformations package to handle the data manipulation and will be splitting the serialization for protobuf into its own package.

@markandrus
Copy link
Author

Thank you! I will sit tight and wait

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

No branches or pull requests

2 participants