feat(proto): add @opentelemetry/proto package

[dyladan]

This is a WIP new package which may eventually be shared among the otlp protobuf/grpc exporters

• Uses protobufjs
• Uses long to support nanosecond timestamps with int precision
• Works in browser and node
• Implements hrTime and hexToBuf in order to avoid dependency on core

TODO:

• see if it is possible to remove grpc-js dependency for non-grpc exporters (optional peer dependency?)
• create transformation functions for logs
• test traces
• test in browser
• check bundle size vs current solution
• add README.md
• add ESM builds
• update package.json fields
• ensure windows compatibility

[codecov]

Codecov Report

Merging #2691 (f254fc1) into main (e1c32b2) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2691   +/-   ##
=======================================
  Coverage   93.27%   93.27%           
=======================================
  Files         158      158           
  Lines        5443     5443           
  Branches     1141     1141           
=======================================
  Hits         5077     5077           
  Misses        366      366           

[dyladan]

@vmarchaud @legendecas the protoc generator doesn't work with node 8 so i think we have a couple possible options:

1. create a small docker file/image with protoc and node 16 to generate the files - means taking a hard dependency on docker for the build process
2. drop support for node 8 - could be for just this package? not sure if the generated files cause problems with node 8 yet but if they don't at least the otlp exporters could still test for 8
3. don't use this module to generate proto files and hand-roll or similar
4. switch node versions during the CI run to build the generated ts with node 16 and test with node 8
5. maybe something else?

[Flarna]

Does protobufjs work with nodejs 8? could be an alternative.

[vmarchaud]

so i think we have a couple possible options:

I think we could use a docker image, its already used for the semantics convention codegen: https://github.com/open-telemetry/opentelemetry-js/blob/main/scripts/semconv/generate.sh

[dyladan]

Does protobufjs work with nodejs 8? could be an alternative.

not sure, i'll give it a try

so i think we have a couple possible options:

I think we could use a docker image, its already used for the semantics convention codegen: main/scripts/semconv/generate.sh

I forgot we already had that dependency. I'm going to try protobufjs first, but if it doesn't work i'll do this.

[legendecas]

+1 to protobufjs. It is web-compatible and it can pre-compile proto files to JSON (or even JS) to boost startup times. If I recall correctly @grpc/proto-loader is already using protobufjs (https://www.npmjs.com/package/@grpc/proto-loader).

[dyladan]

Updated to use protobufjs which removes the dependency on google-protobuf and on @grpc/grpc-js and adds a dependency on protobufjs

Added tests

[morigs]

Looking forward to this! @dyladan pls give us know if we can help with anything

[dyladan]

@morigs you already did by reminding me to get off my lazy butt and work on it 😆

Seriously though, reviews would be helpful. I've already done a quick PoC to prove out the concept of using this package in the otlp exporters with good results.

Open questions:

• Is the use of long acceptable from a bundle size perspective (/cc @MSNev)? Without long, the timestamps lose precision (worth noting that this is already happening in other exporters because JS numbers can't hold timestamps in the nanosecond precision we collect them in).
• Would appreciate a browser/esm expert to verify my esm packaging. I'm not really an expert here and just stole the implementation from another package.

[MSNev]

Is the use of long acceptable from a bundle size perspective (/cc @MSNev Nev Wylie FTE)?

Based on the size of the Long package and how it's used here, I'd recommend not using it and instead extract the couple of functions that are required. This is primarily because everything is namespaced from the "Long" class which means any of the referenced function (depending on your packager) won't be able to tree-shake the unused code.

It's also not clear how the "Long" value would get serialized in the protobuf references.

And isn't a JavaScript Number already a 64-bit value, what am I missing for the need to use this wrapper class?

Answered my own question: Number is an ES6 object and therefore, it's not supported on all browsers -- comes back to the missing browser support matrix and I guess versions of Node as well.

Would still prefer not to include all of the Long class.

[dyladan]

Is the use of long acceptable from a bundle size perspective (/cc @MSNev Nev Wylie FTE)?

Based on the size of the Long package and how it's used here, I'd recommend not using it and instead extract the couple of functions that are required. This is primarily because everything is namespaced from the "Long" class which means any of the referenced function (depending on your packager) won't be able to tree-shake the unused code.

Would still prefer not to include all of the Long class.

Unfortunately it isn't that simple. If you don't include the long package, protobufjs will automatically pass all input to the parseInt function, losing precision.

We could potentially try to look at how protobufjs uses the given Long class (maybe it only uses a couple methods) and come up with our own class that implements them, but if protobufjs ever updates we run the risk of them changing their use of long.js, not to mention the risk that we might introduce bugs.

We could potentially export a function which takes the Long constructor as a parameter and registers it for use by protobufjs. This would leave it to the exporter to enable long precision mode manually (presumably by exposing a configuration option to the end-user).

It's also not clear how the "Long" value would get serialized in the protobuf references.

If long is included, protobufjs takes any input to a field with the s-/u-/int64 or s-/fixed64 types (proto types not js types) and passes it to Long.fromValue(). If long.js is included, it is passed to parseInt(input, 10). protobufjs knows how to serialize both of these objects for the wire.

And isn't a JavaScript Number already a 64-bit value, what am I missing for the need to use this wrapper class?

Answered my own question: Number is an ES6 object and therefore, it's not supported on all browsers -- comes back to the missing browser support matrix and I guess versions of Node as well.

Yes, but even Number is not a 64-bit integer and cannot safely store a timestamp of nanosecond precision without losing precision. Only 53 bits can be safely stored as an integer without losing precision. The current nanosecond timestamp as I write this is 1643147336000000000, which is greater than Number.MAX_SAFE_INTEGER value of 9007199254740991

[MSNev]

Ok, re-looking at this an "assuming" that the web packages are "unlikely" to include the protobuf implementations (in preference for http/json), then as long as this is limited to the proto implementations (as it appears to be here) then it should be ok...

As I'm less concerned about the bundle size for node apps as these generally don't have PLT concerns, they will still have startup concerns (for things like Azure function apps that start/process/stop on-demand) but the overall package size is not quite as critical.

[dyladan]

Ok, re-looking at this an "assuming" that the web packages are "unlikely" to include the protobuf implementations (in preference for http/json), then as long as this is limited to the proto implementations (as it appears to be here) then it should be ok...

As I'm less concerned about the bundle size for node apps as these generally don't have PLT concerns, they will still have startup concerns (for things like Azure function apps that start/process/stop on-demand) but the overall package size is not quite as critical.

I think that isn't necessarily a correct assumption. This package does all transformations required to do the JSON representations also, which wouldn't make sense to duplicate elsewhere.

[morigs]

What if value is an object? Shouldn't it be converted into kvlistValue?

[dyladan]

Done

[morigs]

@dyladan as I understand, Long.js requires Browserify to work on frontend

[legendecas]

Both @grpc/grpc-js and grpc need a special proto loader https://github.com/grpc/grpc-node/tree/master/packages/proto-loader and I don't find the service definition generated by protobufjs is compatible with the one of @grpc/grpc-js (without cumbersome setups).

Ideally I'd find the most compatible way is to convert proto files to json, so that for web we can populate protobuf object classes with protobuf.Root.fromJSON, and for grpc with fromJSON.

[dyladan]

The proto loader is only needed if you are loading proto definitions from json or from .proto files. The static code generated is actually capable of serializing without it and sending with gRPC. Yes, with some cumbersome setup, but this prevents us from having a dependency on grpc-js directly in this package which is not desireable for the http/json exporters.

[legendecas]

IIUC, we don't need to convert the data to concrete protobuf objects if they are transferred by JSON. These conversion helpers only create data objects with valid shapes according to the proto definitions, like https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/exporter-trace-otlp-http/src/transform.ts#L171.

We can still use the types generated by pbts as opentelemetry.proto.collector.metrics.v1.IExportMetricsServiceRequest (by adding leading I).

[dyladan]

You're right we don't, but it doesn't make sense to duplicate all the transformations. We can use the interfaces and generate valid JSON objects then pass those to fromObject in order to get protobuf or we can use fromObject and call toJSON at the end to get JSON back out. Either way is fine with me.

[dyladan]

Actually it looks like even the interfaces can't be used directly for the JSON representation of the protobuf. The interfaces generated use enums, which when serialized directly with JSON.stringify are converted to numbers and not the string representation required by JSON mapping. So in order to map correctly, we would need to either manually fix that (and create our own types i guess?) or use fromObject then toJSON anyway.

[dyladan]

I'm looking into ways to reduce the payload size for the browser case. right now it uses the generated protobuf code to create an in-memory object, then converts that to JSON. the statically generated code is huge though, and we shouldn't need it for the browser at all. I'm looking into a way to have the conversions be able to live without the protobuf code.