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

Allow aliases when requiring instrumented libraries #2110

Closed
1 of 2 tasks
seemk opened this issue Apr 14, 2021 · 5 comments
Closed
1 of 2 tasks

Allow aliases when requiring instrumented libraries #2110

seemk opened this issue Apr 14, 2021 · 5 comments
Labels
feature-request stale up-for-grabs Good for taking. Extra help will be provided by maintainers

Comments

@seemk
Copy link
Contributor

seemk commented Apr 14, 2021

Instrumentations don't apply hooks when the library name given to require doesn't match instrumentation module definition's name exactly.

Suppose I'm using a package called foobar and I alias it in package.json like this and there exists a FoobarInstrumentation which matches against foobar:

dependencies: {
  "foobar2": "npm:[email protected]"
}

Now when I call require("foobar2") the instrumentation module definition's name, foobar does not match it exactly and the instrumentation is not loaded:
https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts#L146-L147

Besides aliases it will be useful when testing instrumentations, e.g. it will be possible to have multiple versions of the same library:

devDependencies: {
  "foobar1": "npm:[email protected]",
  "foobar2": "npm:[email protected]",
}

Now the tests will be able to do both require("foobar1") and require("foobar2"), both of them will be instrumented.

  • This only affects the JavaScript OpenTelemetry library
  • This may affect other libraries, but I would like to get opinions here first
@rauno56
Copy link
Member

rauno56 commented Apr 14, 2021

Playing with the idea a bit further, for that to be implemented, we need to change how we use require-in-the-middle or change require-in-the-middle itself.

  1. Change how we use require-in-the-middle. Currently we wrap require every once for every patch fn any instrumentation has. That buries the real require potentially very deep into those calls, makes it difficult to unwrap that later(in fact ritm has a workaround for this, a "soft-unhook") and makes the complexity to match an instrumentation to a require inefficient since all those functions have to be gone through every time in turn.

    What we could do instead is add one _onRequire, let ritm resolve the package for us and then have our own internal logic to find out if the package needs to be patched or not by the version(something we already do) and the actual name in the package.json(something we don't yet do, but since we check the version anyways, adding it would be cheap).

  2. Change require-in-the-middle itself. The same idea as above could be pushed into ritm. The root ritm package does not currently inspect package.json(but a dependency might), so it would potentially be an added overhead.

@weyert
Copy link
Contributor

weyert commented Apr 18, 2021

Yeah, great idea, I think this would allow people that forked libraries (e.g. typeorm) and published under a different name to still use the existing instrumentation libraries. E.g. instead of typeorm using @company/typeorm

@mhennoch
Copy link
Contributor

I think this will fix #1391 also

@vmarchaud vmarchaud added the up-for-grabs Good for taking. Extra help will be provided by maintainers label May 18, 2021
@github-actions
Copy link

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Jul 25, 2022
@github-actions
Copy link

github-actions bot commented Aug 8, 2022

This issue was closed because it has been stale for 14 days with no activity.

@github-actions github-actions bot closed this as completed Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request stale up-for-grabs Good for taking. Extra help will be provided by maintainers
Projects
None yet
Development

No branches or pull requests

5 participants