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

perf: remove unnecessary base64 encode+decode from OTLP export #4343

Merged
merged 10 commits into from
Dec 19, 2023

Conversation

seemk
Copy link
Contributor

@seemk seemk commented Dec 2, 2023

Which problem is this PR solving?

Fixes #4338

Short description of the changes

Converting trace and span IDs to base64 is not necessary for protobuf serialization, where the base64 strings would be decoded back to their binary forms when serializing the payload.

hexToBase64 is no longer necessary (except for tests), but since it's exported by @opentelemetry/core I didn't delete it. Perhaps up for removal in 2.0?

How Has This Been Tested?

Modified unit tests and tested http, proto and grpc exporters against collector.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added

@seemk seemk requested a review from a team December 2, 2023 10:31
Copy link

codecov bot commented Dec 2, 2023

Codecov Report

Merging #4343 (688c148) into main (42aaae0) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4343      +/-   ##
==========================================
- Coverage   92.23%   92.22%   -0.01%     
==========================================
  Files         332      333       +1     
  Lines        9467     9459       -8     
  Branches     2011     2009       -2     
==========================================
- Hits         8732     8724       -8     
  Misses        735      735              
Files Coverage Δ
...ntal/packages/otlp-transformer/src/common/index.ts 100.00% <100.00%> (ø)
...mental/packages/otlp-transformer/src/logs/types.ts 100.00% <ø> (ø)
...tal/packages/otlp-transformer/src/metrics/types.ts 100.00% <ø> (ø)
...ental/packages/otlp-transformer/src/trace/types.ts 100.00% <ø> (ø)
...ges/opentelemetry-core/src/common/hex-to-binary.ts 100.00% <100.00%> (ø)
...lemetry-core/src/platform/browser/hex-to-base64.ts 100.00% <100.00%> (ø)
...ntelemetry-core/src/platform/node/hex-to-base64.ts 100.00% <100.00%> (ø)

@dyladan dyladan changed the title fix: remove unnecessary base64 encode+decode from OTLP export perf: remove unnecessary base64 encode+decode from OTLP export Dec 19, 2023
@dyladan dyladan merged commit a512494 into open-telemetry:main Dec 19, 2023
20 checks passed
Zirak pushed a commit to Zirak/opentelemetry-js that referenced this pull request Sep 14, 2024
…telemetry#4343)

* fix: remove unnecessary base64 encoding for span contexts

* chore: add changelog

---------

Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
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.

Unnecessary conversion of trace context to base64 in OTLP exporters
2 participants