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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ddf617f
chore(deps): update dependency require-in-the-middle to v7, plus types
trentm Apr 13, 2023
63996d3
add a changelog entry
trentm Apr 13, 2023
760dbd2
I missed these refs in my test updates
trentm Apr 13, 2023
01dab4b
fix prettier formatting
trentm Apr 13, 2023
d679caa
Merge branch 'main' into ritm-update-with-types
trentm Apr 18, 2023
051f975
Merge branch 'main' into ritm-update-with-types
trentm Apr 19, 2023
fa6f726
Merge branch 'main' into ritm-update-with-types
dyladan Apr 24, 2023
9dc87c2
Merge branch 'main' into ritm-update-with-types
trentm Apr 26, 2023
4123476
Merge branch 'main' into ritm-update-with-types
trentm May 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions experimental/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ All notable changes to experimental packages in this project will be documented
### :bug: (Bug Fix)

* fix(sdk-node): only set DiagConsoleLogger when OTEL_LOG_LEVEL is set [#3693](https://github.com/open-telemetry/opentelemetry-js/pull/3672) @pichlermarc
* fix(instrumentation): update `require-in-the-middle` to v7 [#3727](https://github.com/open-telemetry/opentelemetry-js/pull/3727) @trentm

### :books: (Refine Doc)

Expand Down
2 changes: 1 addition & 1 deletion experimental/packages/opentelemetry-instrumentation/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?

Copy link

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?

Copy link

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

opentelemetry-js/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts

Line 27 in 61aae10

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

"semver": "^7.3.2",
"shimmer": "^1.2.1"
},
Expand Down
7 changes: 4 additions & 3 deletions ...l/packages/opentelemetry-instrumentation/src/platform/node/RequireInTheMiddleSingleton.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,14 @@
* limitations under the License.
*/

import * as RequireInTheMiddle from 'require-in-the-middle';
import type { OnRequireFn } from 'require-in-the-middle';
import { Hook } from 'require-in-the-middle';
import * as path from 'path';
import { ModuleNameTrie, ModuleNameSeparator } from './ModuleNameTrie';

export type Hooked = {
moduleName: string;
onRequire: RequireInTheMiddle.OnRequireFn;
onRequire: OnRequireFn;
};

/**
Expand Down Expand Up @@ -59,7 +60,7 @@ export class RequireInTheMiddleSingleton {
}

private _initialize() {
RequireInTheMiddle(
new Hook(
dyladan marked this conversation as resolved.
Show resolved Hide resolved
// Intercept all `require` calls; we will filter the matching ones below
null,
{ internals: true },
Expand Down
12 changes: 7 additions & 5 deletions experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ import {
} from './RequireInTheMiddleSingleton';
import { InstrumentationModuleDefinition } from './types';
import { diag } from '@opentelemetry/api';
import * as RequireInTheMiddle from 'require-in-the-middle';
import type { OnRequireFn } from 'require-in-the-middle';
import { Hook } from 'require-in-the-middle';

/**
* Base abstract class for instrumenting node plugins
Expand All @@ -34,7 +35,7 @@ export abstract class InstrumentationBase<T = any>
implements types.Instrumentation
{
private _modules: InstrumentationModuleDefinition<T>[];
private _hooks: (Hooked | RequireInTheMiddle.Hooked)[] = [];
private _hooks: (Hooked | Hook)[] = [];
private _requireInTheMiddleSingleton: RequireInTheMiddleSingleton =
RequireInTheMiddleSingleton.getInstance();
private _enabled = false;
Expand Down Expand Up @@ -167,7 +168,7 @@ export abstract class InstrumentationBase<T = any>

this._warnOnPreloadedModules();
for (const module of this._modules) {
const onRequire: RequireInTheMiddle.OnRequireFn = (
const onRequire: OnRequireFn = (
exports,
name,
baseDir
Expand All @@ -181,9 +182,10 @@ export abstract class InstrumentationBase<T = any>
};

// `RequireInTheMiddleSingleton` does not support absolute paths.
// For an absolute paths, we must create a separate instance of `RequireInTheMiddle`.
// For an absolute paths, we must create a separate instance of the
// require-in-the-middle `Hook`.
const hook = path.isAbsolute(module.name)
? RequireInTheMiddle([module.name], { internals: true }, onRequire)
? new Hook([module.name], { internals: true }, onRequire)
dyladan marked this conversation as resolved.
Show resolved Hide resolved
: this._requireInTheMiddleSingleton.register(module.name, onRequire);

this._hooks.push(hook);
Expand Down
36 changes: 0 additions & 36 deletions ...ental/packages/opentelemetry-instrumentation/src/platform/node/require-in-the-middle.d.ts
View file
Open in desktop

This file was deleted.