feat!: move serialization to @opentelemetry/otlp-transformer

[pichlermarc]

Which problem is this PR solving?

This is the follow-up I talked about in #4432 (comment).

This PR pulls out the protobuf and JSON serialization code into the @opentelemetry/otlp-transformer package. This PR only touches the Node.js specific exporters, but I plan to do the same with the browser exporters in a follow up, as it'll allow us to get rid of some duplication and simplify the exporters structure. Right now the diff in this PR already very large, so splitting it in two PRs is likely the better option.

Once done for the browser exporters, we can change the public interface @opentelemetry/otlp-transformer to only be the serializers, and the responses. This minimal interface, and eliminating the convert() step in the exporter then also opens up the possibility to implement a custom protobuf serializer as proposed in #4385 without large changes to the public API.

This is also a preparation to #4116. This is currently very hard to do as the transport also handled some serialization (see diff), pulling that out is the first step to make a PR to gradually introduce the structure that I've prepared at #4415.

Also prepares for #3183 as we currently don't have a way to deserialize responses due to the way we structure the exporters. Implementing this is currently also very difficult as it always incurs a very large diff that is near impossible to review. Using the structure from this PR implementing the feature will become a lot easier. See #4415 for a proposal on how to handle partial success once refactored.

Breaking

• OTLPExporterNodeBase now has additional constructor parameters that are required
• OTLPExporterNodeBase now has an additional type parameter
• all Node.js OTLP exporters: convert() now returns an empty object. In a follow-up (feat!: use serializers in browser exporters #4581) the method will be removed as it's not necessary anymore.

Updates #4116
Updates #4583

Type of change

• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

• Unit tests

[pichlermarc]

Note: we'll be able to remove this submodule here too in the follow-up to do the same to the browser exporters.

[codecov]

Codecov Report

Attention: Patch coverage is 91.89189% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 92.72%. Comparing base (2610122) to head (0ff7bf6).
Report is 7 commits behind head on main.

❗ Current head 0ff7bf6 differs from pull request most recent head 1324071. Consider uploading reports for the commit 1324071 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4542      +/-   ##
==========================================
+ Coverage   90.77%   92.72%   +1.94%     
==========================================
  Files          90      330     +240     
  Lines        1930     9555    +7625     
  Branches      417     2048    +1631     
==========================================
+ Hits         1752     8860    +7108     
- Misses        178      695     +517     

Files	Coverage Δ
...ges/exporter-logs-otlp-grpc/src/OTLPLogExporter.ts	100.00% <ø> (ø)
...ogs-otlp-http/src/platform/node/OTLPLogExporter.ts	100.00% <100.00%> (ø)
...gs-otlp-proto/src/platform/node/OTLPLogExporter.ts	91.30% <100.00%> (ø)
.../exporter-trace-otlp-grpc/src/OTLPTraceExporter.ts	100.00% <ø> (ø)
...e-otlp-http/src/platform/node/OTLPTraceExporter.ts	100.00% <100.00%> (ø)
...-otlp-proto/src/platform/node/OTLPTraceExporter.ts	91.30% <100.00%> (ø)
...porter-metrics-otlp-grpc/src/OTLPMetricExporter.ts	100.00% <ø> (ø)
...-otlp-http/src/platform/node/OTLPMetricExporter.ts	100.00% <100.00%> (ø)
...orter-metrics-otlp-proto/src/OTLPMetricExporter.ts	92.30% <100.00%> (ø)
...kages/otlp-exporter-base/src/platform/node/util.ts	64.15% <100.00%> (ø)
... and 4 more

... and 232 files with indirect coverage changes

[trentm]

I think LGTM. I admit to not looking at the otlp-transformer/test/* files very carefully.

[trentm]

I've seen this old one kicking around in other PR's package-lock files as well.

Unrelated to this PR, but I think it is due to this challenging dep in the 'resources' package:

  "devDependencies": {
    "@opentelemetry/api": ">=1.0.0 <1.9.0",
    "@opentelemetry/resources_1.9.0": "npm:@opentelemetry/resources@1.9.0",

[pichlermarc]

Yep I've seen that around too - I also think it's likely from that dependency. From what I've seen it does not cause any harm.

[trentm]

This is replacing:

-  convert(metrics: ResourceMetrics[]): IExportMetricsServiceRequest {
-    return createExportMetricsServiceRequest(metrics, { useLongBits: false });
-  }

I think this means that the value of useHex in getOtlpEncoder() will have changed from the default of false, to this given true value. Do you know if that matters for metrics serialization?

From a quick (very naive) read of getOtlpEncoder, it looks like useHex is only used for span-context-related encoding, so perhaps the useHex value doesn't matter at all for metrics.

[pichlermarc]

useHex does not matter for metrics today. I removed it for now to align it with how it was used before - functionality-wise I'd like to change as little as possible in this PR.

9cb2593

[trentm]

Oh, there is also a otlp-grpc-exporter-base/submodule.md file that should perhaps move to otlp-transformer ?

[david-luna]

I'm not familiar with these tests. Is it necessary to move this call at the bottom of the test?

[pichlermarc]

This is a good question - I should've put a note here as it's quite odd 🙂

I removed the OTLPProtoExporterNodeBase class and the proto exporters are now using the same export logic as the JSON exporter (OTLPExporterNodeBase). That means is that it does not use the setImmediate() anymore to delay the export (see here for code that's used now). As this setImmedidate() was there before, it was possible call export, then mock and wait for the test to finish. As it's gone, that does not work anymore (export() will actually export when it's called, not in the next tick).

From what I can tell the setImmediate() was added to OTLPProtoExporterNodeBase to avoid loading http before it can be instrumented (the comment mentioned protobufjs, but AFAIK there's no such instrumentation). While this was a good effort to try and delay the loading of the http module, OTLPProtoExporterNodeBase extended OTLPExporterNodeBase which made no effort to delay the loading of http. This made the proto-base's efforts to avoid loading ineffective, as it meant that http was imported when the base was required regardless of the setImmediate() in the OTLPProtoExporterNodeBase.

In follow-ups, I'll re-structure the transport code in the exporters to use the code from #4415. This will then actually implement the lazy-loading that was intended to be there in the first place.

[david-luna]

if I understood right the variable naming log_X_Y_Z has a logic behind and:

• X is the resource
• Y is the scope
• Z is the log fragment

so log_2_1_1 combines resource_2, scope_1 and log_fragment_1.

this should be in a comment for future readers

[pichlermarc]

Yep that's true. Working with them also was not very ergonomic. These lines were just moved up one describe() block since I added more tests here; they only appear to be new in the diff due to the new indentation.

Added a comment how these are structured. 🙂
4f80ada

[pichlermarc]

Oh, there is also a otlp-grpc-exporter-base/submodule.md file that should perhaps move to otlp-transformer ?

Yes, good call. Done in e8adc4d 🙂

[pichlermarc]

Thanks for the reviews 🙂

The follow-up (doing the same for the browser exporter) is now also open for review (#4581) 🙂