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

fix(deps): update dependency require-in-the-middle to v7.1.0 for types and named export #3727

Merged
merged 9 commits into from
May 2, 2023

Conversation

trentm
Copy link
Contributor

@trentm trentm commented Apr 13, 2023

Which problem is this PR solving?

This updates the 'require-in-the-middle' dep to one that includes typescript types and that
has a named export for the Hook function. It switches to using that named export.

Fixes: #3701

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • cd experimental/packages/opentelemetry-instrumentation; npm install; npm test

Checklist:

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

@trentm trentm changed the title chore(deps): update dependency require-in-the-middle to v7, plus types fix(deps): update dependency require-in-the-middle to v7, plus types Apr 13, 2023
@@ -68,7 +68,7 @@
"url": "https://github.com/open-telemetry/opentelemetry-js/issues"
},
"dependencies": {
"require-in-the-middle": "^6.0.0",
"require-in-the-middle": "elastic/require-in-the-middle#trentm/types",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for reviewers: This points to the proposed elastic/require-in-the-middle#67 to move TypeScript types to the require-in-the-middle module. Do those look reasonable?

Choose a reason for hiding this comment

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

Yes makes sense. It might be better though to create a new pull request for the types replacement.

Copy link
Member

Choose a reason for hiding this comment

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

Using git to pull dependencies can result in significant slowdowns in many cases. Is there some reason you don't want to include them with the main package?

Choose a reason for hiding this comment

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

Using git to pull dependencies can result

I think the intention was not to merge this. Only to see if it works and to see if we agree with this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dyladan The intention was not the merge this, but to allow for feedback on the index.d.ts file I was adding to require-in-the-middle at elastic/require-in-the-middle#67 before merging that, doing an RITM release, and then updating this PR to an actual RITM release version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be better though to create a new pull request for the types replacement.

I can do that tomorrow. It'll then be one update to fix #3655 and a separate one for #3701

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've moved the fix for the first issue to #3743. Once that is in, I'll get a RITM 7.1.0 release with types and finish preparing this PR.

Copy link
Member

Choose a reason for hiding this comment

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

In the other PR you use ^7.0.1 therefore 7.1.0 will be automatically used once released.
Will this result in problems/type conflicts and potentially broken builds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Flarna I don't think it will cause a problem. I've now released require-in-the-middle@7.1.0 -- which includes types. It still supports the default Hook export for usage. I guess what I don't know is whether TypeScript will prefer the local require-in-the-middle.d.ts over the types in the require-in-the-middle dep when resolving

import * as RequireInTheMiddle from 'require-in-the-middle';
and similar.

I re-ran npm install in "experimental/packages/opentelemetry-instrumentation" in a git clone of the main branch to get the new release:

% npm ls require-in-the-middle
@opentelemetry/instrumentation@0.38.0 /Users/trentm/src/opentelemetry-js/experimental/packages/opentelemetry-instrumentation
└── require-in-the-middle@7.1.0

% git branch
* main
  v1.10

and npm run compile, npm test, and npm run lint are passing for me.

Copy link
Member

Choose a reason for hiding this comment

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

I believe the types are actually merged "intelligently" when a module is declared twice

@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #3727 (91fdca2) into main (2f1e316) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 91fdca2 differs from pull request most recent head 4123476. Consider uploading reports for the commit 4123476 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3727      +/-   ##
==========================================
- Coverage   93.21%   93.20%   -0.01%     
==========================================
  Files         296      296              
  Lines        8999     8998       -1     
  Branches     1853     1853              
==========================================
- Hits         8388     8387       -1     
  Misses        611      611              
Impacted Files Coverage Δ
...n/src/platform/node/RequireInTheMiddleSingleton.ts 90.00% <100.00%> (ø)
...strumentation/src/platform/node/instrumentation.ts 89.47% <100.00%> (-0.11%) ⬇️

@trentm trentm changed the title fix(deps): update dependency require-in-the-middle to v7, plus types fix(deps): update dependency require-in-the-middle to v7.1.0 for types and named export Apr 19, 2023
@trentm trentm marked this pull request as ready for review April 19, 2023 17:52
@trentm trentm requested review from a team and kirrg001 April 19, 2023 17:52
@trentm
Copy link
Contributor Author

trentm commented May 2, 2023

@kirrg001 Are you appropriate to review this? Or perhaps @dyladan's review is sufficient to merge this. Another user recently asked on the motivating issue when this might get merged.

@pichlermarc pichlermarc merged commit 32ae641 into open-telemetry:main May 2, 2023
@trentm
Copy link
Contributor Author

trentm commented May 2, 2023

Thanks, all!

@trentm trentm deleted the ritm-update-with-types branch May 2, 2023 14:54
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
Development

Successfully merging this pull request may close these issues.

@opentelemetry/instrumentation crashes when used with typescript esModuleInterop: true
5 participants