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(otlp-proto): pre-compile proto files #3098

Merged
merged 5 commits into from
Jul 20, 2022

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jul 18, 2022

Which problem is this PR solving?

Precompile proto files to JS source code and get rid of runtime code generation. Also, this allows exporting traces and metrics in one process simultaneously by not caching the ExportRequestProto type.

This doesn't change how otlp-grpc-exporter loads proto files as https://github.com/grpc/grpc-node/tree/master/packages/proto-loader requires raw proto files to load service definitions.

Fixes #515

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

The existing tests should cover this change.

Checklist:

  • Followed the style guidelines of this project

@legendecas legendecas force-pushed the proto-eval branch 4 times, most recently from a46e7b6 to ee885a4 Compare July 18, 2022 03:26
@codecov
Copy link

codecov bot commented Jul 18, 2022

Codecov Report

Merging #3098 (6620fd8) into main (9ac9976) will decrease coverage by 0.96%.
The diff coverage is n/a.

❗ Current head 6620fd8 differs from pull request most recent head 8b98760. Consider uploading reports for the commit 8b98760 to get more accurate results

@@            Coverage Diff             @@
##             main    #3098      +/-   ##
==========================================
- Coverage   93.06%   92.09%   -0.97%     
==========================================
  Files         188       82     -106     
  Lines        6248     2405    -3843     
  Branches     1313      520     -793     
==========================================
- Hits         5815     2215    -3600     
+ Misses        433      190     -243     
Impacted Files Coverage Δ
...s/opentelemetry-core/src/platform/node/sdk-info.ts 0.00% <0.00%> (-100.00%) ⬇️
...opentelemetry-core/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...pentelemetry-core/src/platform/node/performance.ts 0.00% <0.00%> (-100.00%) ⬇️
...emetry-api-metrics/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...lemetry-resources/src/detectors/ProcessDetector.ts 31.81% <0.00%> (-68.19%) ⬇️
...perimental/packages/otlp-exporter-base/src/util.ts 79.41% <0.00%> (-17.65%) ⬇️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 84.48% <0.00%> (-15.52%) ⬇️
...lemetry-resources/src/detectors/BrowserDetector.ts 93.33% <0.00%> (-6.67%) ⬇️
...emetry-sdk-metrics-base/src/export/MetricReader.ts
...sync-hooks/src/AbstractAsyncHooksContextManager.ts
... and 104 more

@legendecas legendecas marked this pull request as ready for review July 18, 2022 05:55
@legendecas legendecas requested a review from a team July 18, 2022 05:55
@dyladan
Copy link
Member

dyladan commented Jul 18, 2022

Can you document how to regenerate the js when we need to update? A script would be even better.

@vmarchaud
Copy link
Member

Can you document how to regenerate the js when we need to update? A script would be even better.

I believe the script is there: https://github.com/open-telemetry/opentelemetry-js/pull/3098/files#diff-90a1f9aebed9b5b6edbb022c2bb5afb203e3317af67429a27ac9f63fb75bcb13R1 and its run automatically when we run lerna run compile

@dyladan
Copy link
Member

dyladan commented Jul 18, 2022

Can you document how to regenerate the js when we need to update? A script would be even better.

I believe the script is there: #3098 (files) and its run automatically when we run lerna run compile

Ahh i just missed it

@legendecas
Copy link
Member Author

Yeah, the generation is automatic as registered in the package.json scripts and there is no need to manually run the script to compile it.

@dyladan
Copy link
Member

dyladan commented Jul 19, 2022

Do we know how this affects final bundle size in browser?

@legendecas
Copy link
Member Author

Do we know how this affects final bundle size in browser?

With a naïve comparison, code generated with this PR reduces the bundle size to ~300kb (bundled and minified with @vercel/ncc) from ~310kb (~220kb JS source code + ~90kb proto files).

@dyladan
Copy link
Member

dyladan commented Jul 19, 2022

Wow both quite large. Ok thanks for the stats

@dyladan
Copy link
Member

dyladan commented Jul 19, 2022

This seems ok to merge when you're happy with it.

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.

javascript: uses unsafe-eval
4 participants