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

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

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Mar 13, 2024

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

@@ -1,6 +1,6 @@
[submodule "experimental/packages/otlp-grpc-exporter-base/protos"]
path = experimental/packages/otlp-grpc-exporter-base/protos
url = https://github.com/open-telemetry/opentelemetry-proto.git
[submodule "experimental/packages/otlp-proto-exporter-base/protos"]
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link

codecov bot commented Mar 13, 2024

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

@pichlermarc pichlermarc marked this pull request as ready for review March 20, 2024 12:50
@pichlermarc pichlermarc requested a review from a team March 20, 2024 12:50
@pichlermarc pichlermarc added this to the OTLP Exporter GA milestone Mar 20, 2024
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…telemetry#4542)

* feat!:move serializers to otlp-transformer

* feat!: use serializeres in protobuf and json exporters

* test(otlp-transformer): add tests for trace serializer

* test(otlp-transformer): add tests for metrics serializer

* test(otlp-transformer): add tests for logs serializer

* chore: resolve more conflicts

* fix: sync package-lock

* chore: cleanup dependencies, unused code, .gitignore

* chore: fix changelog indentation

* fix(otlp-transformer): remove unused useHex from JsonMetricsSerializer

* chore: add comment about how logs data is structured

* docs: move submodule.md, adapt contents

* fixup! Merge branch 'main' into feat/transformer-serializer

* fixup! Merge branch 'main' into feat/transformer-serializer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants