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-grpc-exporter-base): use statically generated protobuf code #3705

Merged

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Mar 29, 2023

Which problem is this PR solving?

  • gRPC exporter is currently not working with bundling tools as it uses the proto files from the build directory to generate a client on runtime.
  • grpc_tools_node_protoc can generate service and protobuf definitions, however, the protobuf types generated by that tool are not compatible with types from our existing @opentelemetry/otlp-transformer package.

This PR adds support for using the gRPC exporter with bundling tools by mimicking what the code generated by gRPC's static code generator is doing, while still keeping the @opentelemetry/otlp-transformer package as-is. This is accomplished by hand-rolling a service definition and using code that is statically generated using protobufjs-cli instead.

As we already use protobuf-js-cli, in the http/protobuf exporter package, I re-used the existing script and moved it to the scripts directory in the repository root.

Fixes #2786, #3632, #3656

Type of change

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

How Has This Been Tested?

  • Existing tests
  • Manual testing with webpack

Checklist:

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

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #3705 (3a62d02) into main (abfb1bb) will decrease coverage by 0.31%.
The diff coverage is 43.33%.

❗ Current head 3a62d02 differs from pull request most recent head eb3bb5b. Consider uploading reports for the commit eb3bb5b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3705      +/-   ##
==========================================
- Coverage   93.50%   93.19%   -0.31%     
==========================================
  Files         293      296       +3     
  Lines        8961     8999      +38     
  Branches     1853     1853              
==========================================
+ Hits         8379     8387       +8     
- Misses        582      612      +30     
Impacted Files Coverage Δ
...ntal/packages/otlp-grpc-exporter-base/src/types.ts 100.00% <ø> (ø)
...-grpc-exporter-base/src/LogsExportServiceClient.ts 42.85% <42.85%> (ø)
...pc-exporter-base/src/MetricsExportServiceClient.ts 42.85% <42.85%> (ø)
...grpc-exporter-base/src/TraceExportServiceClient.ts 42.85% <42.85%> (ø)
...ental/packages/otlp-grpc-exporter-base/src/util.ts 76.00% <44.44%> (+0.03%) ⬆️

... and 4 files with indirect coverage changes

@pichlermarc pichlermarc changed the title feat(otlp-grpc-exporter-base): use generated protos feat(otlp-grpc-exporter-base): use statically generated protobuf code Mar 29, 2023
package.json Outdated Show resolved Hide resolved
Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Looks good. I tried this myself once before and got quite frustrated

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Looks good to me. It looks like the precompile is running npm protos which itself is running npm submodule, then after the compile is finished lerna run protos is run again. i think this isn't required anymore? Seems to only affect grpc exporter base and proto exporter base which both handle it in the precompile step. I think also the submodules are updated once per package instead of globally. Instead of --concurrency 1 it might make sense to just run once? I think the script is not doing anything package-specific.

@pichlermarc
Copy link
Member Author

Looks good to me. It looks like the precompile is running npm protos which itself is running npm submodule, then after the compile is finished lerna run protos is run again. i think this isn't required anymore? Seems to only affect grpc exporter base and proto exporter base which both handle it in the precompile step. I think also the submodules are updated once per package instead of globally. Instead of --concurrency 1 it might make sense to just run once? I think the script is not doing anything package-specific.

I'm not sure I understood correctly: did you mean that I should split the protos script into submodule update and file generation script and then only trigger the submodule update once in the top-level package.json instead? 🤔

I pushed a change that does that but keeps the protos step in each package's package.json so that the compile script in the exporter packages can still be used independently from the top level package.json(073b01a). Hope that addresses this concern 🙂

@dyladan
Copy link
Member

dyladan commented Apr 25, 2023

Looks good to me. It looks like the precompile is running npm protos which itself is running npm submodule, then after the compile is finished lerna run protos is run again. i think this isn't required anymore? Seems to only affect grpc exporter base and proto exporter base which both handle it in the precompile step. I think also the submodules are updated once per package instead of globally. Instead of --concurrency 1 it might make sense to just run once? I think the script is not doing anything package-specific.

I'm not sure I understood correctly: did you mean that I should split the protos script into submodule update and file generation script and then only trigger the submodule update once in the top-level package.json instead? 🤔

Yes. This just prevents duplicate submodule updates when the whole project is compiled

I pushed a change that does that but keeps the protos step in each package's package.json so that the compile script in the exporter packages can still be used independently from the top level package.json(073b01a). Hope that addresses this concern 🙂

Yeah it looks right to me

@pichlermarc
Copy link
Member Author

I'll wait for #3712 to merge first and then add the logs client as well.
It will be easier to do it this way around as the diff is smaller for this PR.

@pichlermarc
Copy link
Member Author

Added logs too, should be good to go. 🙂
@dyladan could you have another look? 🙂

@pichlermarc pichlermarc merged commit 2f1e316 into open-telemetry:main Apr 28, 2023
14 checks passed
@pichlermarc pichlermarc deleted the use-generated-grpc branch April 28, 2023 10:58
kodiakhq bot pushed a commit to sullivanpj/open-system that referenced this pull request Jun 3, 2023
…324)

<h3>Snyk has created this PR to upgrade @opentelemetry/instrumentation from 0.38.0 to 0.39.1.</h3>

:information_source: Keep your dependencies up-to-date. This makes it easier to fix existing vulnerabilities and to more quickly identify and fix newly disclosed vulnerabilities when they affect your project.
<hr/>

- The recommended version is **2 versions** ahead of your current version.
- The recommended version was released **22 days ago**, on 2023-05-12.


<details>
<summary><b>Release notes</b></summary>
<br/>
  <details>
    <summary>Package name: <b>@opentelemetry/instrumentation</b></summary>
    <ul>
      <li>
        <b>0.39.1</b> - <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/releases/tag/experimental%2Fv0.39.1">2023-05-12</a></br><h3><g-emoji class="g-emoji" alias="bug" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f41b.png">🐛</g-emoji> (Bug Fix)</h3>
<ul>
<li>fix(otlp-transformer): move api-logs to dependencies <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/pull/3798" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/3798/hovercard">#3798</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/pichlermarc/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://snyk.io/redirect/github/pichlermarc">@ pichlermarc</a></li>
</ul>
      </li>
      <li>
        <b>0.39.0</b> - <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/releases/tag/experimental%2Fv0.39.0">2023-05-11</a></br><h3><g-emoji class="g-emoji" alias="rocket" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f680.png">🚀</g-emoji> (Enhancement)</h3>
<ul>
<li>feat(otlp-transformer): support log records. <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/pull/3712/" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/3712/hovercard">#3712</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/llc1123/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://snyk.io/redirect/github/llc1123">@ llc1123</a></li>
<li>feat(otlp-grpc-exporter-base): support log records. <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/pull/3712/" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/3712/hovercard">#3712</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/llc1123/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://snyk.io/redirect/github/llc1123">@ llc1123</a></li>
<li>feat(exporter-logs-otlp-grpc): otlp-grpc exporter for logs. <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/pull/3712/" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/3712/hovercard">#3712</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/llc1123/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://snyk.io/redirect/github/llc1123">@ llc1123</a></li>
<li>feat(otlp-grpc-exporter-base): use statically generated protobuf code <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/pull/3705" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/3705/hovercard">#3705</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/pichlermarc/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://snyk.io/redirect/github/pichlermarc">@ pichlermarc</a></li>
<li>refactor(otlp-transformer): refine metric transformers. <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/pull/3770/" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/3770/hovercard">#3770</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/llc1123/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://snyk.io/redirect/github/llc1123">@ llc1123</a></li>
</ul>
<h3><g-emoji class="g-emoji" alias="bug" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f41b.png">🐛</g-emoji> (Bug Fix)</h3>
<ul>
<li>fix(instrumentation): update <code>require-in-the-middle</code> to v7.1.0 <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/pull/3727" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/3727/hovercard">#3727</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/trentm/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://snyk.io/redirect/github/trentm">@ trentm</a></li>
<li>fix(instrumentation): update <code>require-in-the-middle</code> to v7.0.1 <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/pull/3743" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/3743/hovercard">#3743</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/trentm/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://snyk.io/redirect/github/trentm">@ trentm</a></li>
</ul>
<h3><g-emoji class="g-emoji" alias="books" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f4da.png">📚</g-emoji> (Refine Doc)</h3>
<ul>
<li>doc(instrumentation): add limitiations section to readme <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/pull/3786" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/3786/hovercard">#3786</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/Flarna/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://snyk.io/redirect/github/Flarna">@ Flarna</a></li>
</ul>
      </li>
      <li>
        <b>0.38.0</b> - <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/releases/tag/experimental%2Fv0.38.0">2023-04-13</a></br><h3><g-emoji class="g-emoji" alias="boom" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f4a5.png">💥</g-emoji> Breaking Change</h3>
<ul>
<li>fix: remove HTTP/HTTPS prefix from span name in instrumentation-xml-http-request <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/pull/3672" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/3672/hovercard">#3672</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/jufab/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://snyk.io/redirect/github/jufab">@ jufab</a></li>
<li>fix(sdk-node)!: remove unused defaultAttributes option <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/pull/3724" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/3724/hovercard">#3724</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/pichlermarc/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://snyk.io/redirect/github/pichlermarc">@ pichlermarc</a>
<ul>
<li>Please use <code>NodeSDKConfiguration.resource</code> instead</li>
</ul>
</li>
</ul>
<h3><g-emoji class="g-emoji" alias="rocket" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f680.png">🚀</g-emoji> (Enhancement)</h3>
<ul>
<li>feat(sdk-logs): use logs API 0.38</li>
</ul>
<h3><g-emoji class="g-emoji" alias="bug" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f41b.png">🐛</g-emoji> (Bug Fix)</h3>
<ul>
<li>fix(sdk-node): only set DiagConsoleLogger when OTEL_LOG_LEVEL is set <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/pull/3693" data-hovercard-type="pull_request" data-hovercard-url="/open-telemetry/opentelemetry-js/pull/3693/hovercard">#3693</a> <a class="user-mention notranslate" data-hovercard-type="user" data-hovercard-url="/users/pichlermarc/hovercard" data-octo-click="hovercard-link-click" data-octo-dimensions="link_type:self" href="https://snyk.io/redirect/github/pichlermarc">@ pichlermarc</a></li>
</ul>
      </li>
    </ul>
    from <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/releases">@opentelemetry/instrumentation GitHub release notes</a>
  </details>
</details>


<details>
  <summary><b>Commit messages</b></summary>
  </br>
  <details>
    <summary>Package name: <b>@opentelemetry/instrumentation</b></summary>
    <ul>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/bba09c0e5a6d92fc71ed08028966ae9d56da53db">bba09c0</a> chore: release 0.39.1 (#3800)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/29c46ef5cefe2cc3bf88a64c9ba810aa557f7a06">29c46ef</a> fix(otlp-transformer): move api-logs to dependencies (#3798)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/8fc76896595aac912bf9e15d4f19c167317844c8">8fc7689</a> chore: release 1.13 / 0.39 (#3776)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/32632bd30309621908da8166c6595a23af892f06">32632bd</a> doc(instrumentation): add limitiations section to readme (#3786)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/b8b63083869fb5850fd4830ded0fc509a2cd0f0a">b8b6308</a> chore: remove &quot;opentelemetry&quot; prefix from opencensus-shim package directory (#3784)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/7255da994fe79f115ceb9b1260fb171cf9313706">7255da9</a> feat(opencensus-shim) add mapping logic and propagation shim (#3751)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/32ae641ad13be8786cce9ffc3942efa385c0ab33">32ae641</a> fix(deps): update dependency require-in-the-middle to v7.1.0 for types and named export (#3727)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/2f1e31648402b310448d226ae9632b115e6ad5c4">2f1e316</a> feat(otlp-grpc-exporter-base): use statically generated protobuf code (#3705)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/abfb1bb68e1ec8a183c01644ab9673cd6ac74841">abfb1bb</a> refactor(otlp-transformer): refine metrics transformer (#3770)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/a96116db88c7c0a0d1185601942b379c6118bd6d">a96116d</a> deps: remove &#x60;rimraf&#x60; (#3769)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/98e4e821bcd5b3d74f95db26439a93c3abb9f467">98e4e82</a> feat(exporter-logs-otlp-grpc): implements otlp-grpc exporters for logs (#3712)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/9347ca6a3ea5f40b9fff33d1a9a3ae2bea7e6ba8">9347ca6</a> fix(http): stop listening to &#x60;request&#x60;&#x27;s &#x60;close&#x60; event once it has emitted &#x60;response&#x60; (#3625)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/6676414505f6ffc75e151fa162e351fa1de7204e">6676414</a> feat(opencensus-shim): add OpenCensus shim package boilerplate (#3750)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/cd0d8806e0ed4c9dc68be9773770349c712e5e76">cd0d880</a> fix(sdk-node): lazy require @ opentelemetry/exporter-jaeger (#3739)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/de354dbbdd24bb9f93edef6d098fe8484c7bc3ff">de354db</a> fix: VisibilityState type for typescript &gt;4.6 (#3741)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/61aae103f39ffa20f4e01f23f4bfd6b1cc6f7e9e">61aae10</a> Fix changelog link (#3744)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/3e99e13a4ec55a94926fdb1c1c6f5955783b3ab3">3e99e13</a> fix(instrumentation): update dep require-in-the-middle to 7.0.1 (#3743)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/1b50f91850d0d503feb3a2aeb5c83d63598e8180">1b50f91</a> deps(sdk-logs): remove unused rimraf dev dependency (#3738)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/5b9534bd47deb2a15f7414ba7df05057f6ab9b1e">5b9534b</a> deps: remove unused mkdirp and rimraf dependencies (#3737)</li>
      <li><a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/commit/053acb604c9d2b735bb267ff47724bb3af1ffd62">053acb6</a> Logs version fixup (#3729)</li>
    </ul>

   <a href="https://snyk.io/redirect/github/open-telemetry/opentelemetry-js/compare/a04090010ee18e17487b449984807cc2b7b6e3e6...bba09c0e5a6d92fc71ed08028966ae9d56da53db">Compare</a>
  </details>
</details>
<hr/>

**Note:** *You are seeing this because you or someone else with access to this repository has authorized Snyk to open upgrade PRs.*

For more information:  <img src="https://api.segment.io/v1/pixel/track?data=eyJ3cml0ZUtleSI6InJyWmxZcEdHY2RyTHZsb0lYd0dUcVg4WkFRTnNCOUEwIiwiYW5vbnltb3VzSWQiOiI2Y2I5ODY1ZC00NzEyLTQ2NTEtYmQ5Mi0zZThlODVlMGU2MWYiLCJldmVudCI6IlBSIHZpZXdlZCIsInByb3BlcnRpZXMiOnsicHJJZCI6IjZjYjk4NjVkLTQ3MTItNDY1MS1iZDkyLTNlOGU4NWUwZTYxZiJ9fQ==" width="0" height="0"/>

🧐 [View latest project report](https://app.snyk.io/org/sullivanpj/project/61dfb01d-4c48-4d8b-a40c-a3939907a630?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🛠 [Adjust upgrade PR settings](https://app.snyk.io/org/sullivanpj/project/61dfb01d-4c48-4d8b-a40c-a3939907a630/settings/integration?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🔕 [Ignore this dependency or unsubscribe from future upgrade PRs](https://app.snyk.io/org/sullivanpj/project/61dfb01d-4c48-4d8b-a40c-a3939907a630/settings/integration?pkg&#x3D;@opentelemetry/instrumentation&amp;utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr#auto-dep-upgrades)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants