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

ESM --experimental-loader=@opentelemetry/instrumentation/hook.mjs breaks typescript path aliases #5097

Closed
douglasg14b opened this issue Oct 29, 2024 · 11 comments
Assignees
Labels
bug Something isn't working needs:reproducer This bug/feature is in need of a minimal reproducer triage

Comments

@douglasg14b
Copy link

douglasg14b commented Oct 29, 2024

What happened?

Using the recommended way of instrumenting an ESM application causes path aliases to break, rendering it unusable.

Steps to Reproduce

  1. Have a TS project that you start with tsx
  2. Setup path aliases in your tsconfig & package.json
  3. Use path aliases
  4. Add --experimental-loader=@opentelemetry/instrumentation/hook.mjs to your startup command

Expected Result

It starts

Actual Result

It fails to start, erroring out on any imported files that use path aliases

Additional Details

OpenTelemetry Setup Code

/*instrumentation.ts*/
import { getNodeAutoInstrumentations } from '@opentelemetry/auto-instrumentations-node';

import { Resource } from '@opentelemetry/resources';
import { ConsoleMetricExporter, PeriodicExportingMetricReader } from '@opentelemetry/sdk-metrics';
import { NodeSDK } from '@opentelemetry/sdk-node';
import { ATTR_SERVICE_NAME, ATTR_SERVICE_VERSION } from '@opentelemetry/semantic-conventions';

// NOTE: https://github.com/open-telemetry/opentelemetry-js/blob/main/doc/esm-support.md

const sdk = new NodeSDK({
    resource: new Resource({
        [ATTR_SERVICE_NAME]: 'yourServiceName',
        [ATTR_SERVICE_VERSION]: '1.0',
    }),
    metricReader: new PeriodicExportingMetricReader({
        exporter: new ConsoleMetricExporter(),
    }),
    instrumentations: [
        getNodeAutoInstrumentations({
            '@opentelemetry/instrumentation-http': {},
            '@opentelemetry/instrumentation-express': {},
        }),
    ],
});

sdk.start();

tsconfig.json

{
    "extends": "@ivr3/tsconfig/base.json",
    "include": [ "./**/*.ts"],
    "compilerOptions": {
        "outDir": "./dist",
        "rootDir": ".",
        "baseUrl": ".",
        "paths": {
            "@/*": ["./src/*"]
        },
        "strict": true
    }
}
{
    "$schema": "https://json.schemastore.org/tsconfig",
    "display": "Default",
    "exclude": ["node_modules"],
    "compilerOptions": {
        "lib": ["ESNext"],
        "target": "ESNext",
        "module": "ESNext",
        "moduleResolution": "Node",
        "outDir": "${configDir}/dist",
        "strict": true,
        "esModuleInterop": true,
        "forceConsistentCasingInFileNames": true,
        "skipLibCheck": true,
        "noEmitOnError": false,
        "experimentalDecorators": true,
        "resolveJsonModule": true
    }
}

package.json

{
    "name": "",
    "version": "1.47.3",
    "type": "module",
    "main": "src/infra/web/index.ts",
    "scripts": {
        "start:dev": "pnpm build:routes && dotenvx run --env-file=.env.development -- npx tsx --experimental-loader=@opentelemetry/instrumentation/hook.mjs --import ./telemetry.ts src/infra/web/index.ts",
        "build:routes": "tsoa -c tsoaConfig.json spec-and-routes",
    },
    "dependencies": {
        "@aws-sdk/client-connect": "catalog:aws-sdk",
        "@aws-sdk/client-dynamodb": "catalog:aws-sdk",
        "@aws-sdk/client-pinpoint": "catalog:aws-sdk",
        "@aws-sdk/client-s3": "catalog:aws-sdk",
        "@aws-sdk/client-ssm": "catalog:aws-sdk",
        "@aws-sdk/lib-dynamodb": "catalog:aws-sdk",
        "@aws-sdk/s3-request-presigner": "catalog:aws-sdk",
        "@aws-sdk/util-dynamodb": "catalog:aws-sdk",
        "@opentelemetry/sdk-node": "^0.54.0",
        "@opentelemetry/sdk-metrics": "^1.27.0",
        "@opentelemetry/instrumentation": "^0.54.0",
        "@opentelemetry/auto-instrumentations-node": "^0.52.0",
        "@opentelemetry/exporter-prometheus": "^0.54.0",
        "@opentelemetry/semantic-conventions": "^1.27.0",
        "@opentelemetry/resources": "^1.27.0",
        "@dotenvx/dotenvx": "^0.37.1",
        "@smithy/node-http-handler": "3.0.0",
        "@tsoa/runtime": "^6.2.1",
        "cors": "^2.8.5",
        "date-holidays": "^3.23.12",
        "dotenv": "*",
        "dynamodb-toolbox": "^0.9.5",
        "env-var": "^7.5.0",
        "express": "^4.19.2",
        "file-type": "^19.1.0",
        "fluent-ffmpeg": "^2.1.3",
        "jsonwebtoken": "^9.0.2",
        "ky": "^1.3.0",
        "libphonenumber-js": "^1.11.4",
        "lodash-es": "^4.17.21",
        "luxon": "^3.5.0",
        "module-alias": "^2.2.3",
        "tsoa": "^6.2.1",
        "type-fest": "^4.18.3",
        "typia": "^6.10.4",
        "uuid": "*"
    },
    "devDependencies": {
        "@types/jsonwebtoken": "^9.0.6",
        "@faker-js/faker": "^8.4.1",
        "@hyrious/esbuild-plugin-commonjs": "^0.2.4",
        "@types/chai": "^4.3.19",
        "@types/cors": "^2.8.17",
        "@types/express": "^4.17.21",
        "@types/fluent-ffmpeg": "^2.1.24",
        "@types/lodash-es": "^4.17.12",
        "@types/luxon": "^3.4.2",
        "@types/module-alias": "^2.0.4",
        "@types/supertest": "^6.0.2",
        "@types/uuid": "*",
        "aws-sdk-client-mock": "^4.0.1",
        "copyfiles": "^2.4.1",
        "deepmerge": "^4.3.1",
        "supertest": "^7.0.0",
        "ts-add-js-extension": "^1.6.4",
        "ts-patch": "^3.2.1",
        "ts-runtime-checks": "^0.6.2",
        "tsc-alias": "^1.8.10",
        "tsx": "^4.11.2",
        "typescript": "^5.6.2",
        "vitest-tsconfig-paths": "^3.4.1"
    },
    "_moduleAliases": {
        "@": "src"
    }
}

Relevant log output

No response

@douglasg14b douglasg14b added bug Something isn't working triage labels Oct 29, 2024
@dyladan dyladan added the needs:reproducer This bug/feature is in need of a minimal reproducer label Oct 30, 2024
@jpmunz
Copy link

jpmunz commented Oct 30, 2024

hey @douglasg14b would you be able to provide the tsconfig.json you're using as well? Thanks

@douglasg14b
Copy link
Author

@jpmunz tsconfig added to OP

@jpmunz
Copy link

jpmunz commented Oct 31, 2024

@douglasg14b I believe this behaviour is coming from the import-in-the-middle package used by @opentelemetry/instrumentation/hook.mjs. I setup a minimal example here to reproduce without opentelemetry: https://github.com/jpmunz/simple-node-app

I'm not actually sure if this package is intended to understand those path aliases, I'm asking about that here nodejs/import-in-the-middle#162, if not there would need to be some additional step in your build process to resolve those paths prior to running the loader

@douglasg14b
Copy link
Author

douglasg14b commented Nov 1, 2024

Hm, I'm not entirely sure how paths would be resolved before using tsx since tsc-alias is expected to run after compilation. Which happens during tsx run.

Precompiling would defeat the purpose of using tsx (For which there are many reasons)

@jpmunz
Copy link

jpmunz commented Nov 4, 2024

so to get that working with tsx I think the hook would have to be wrapped with something that could resolve the path aliases at run time. You might be able to give this workaround (based on https://stackoverflow.com/a/75318315) a try, it uses tsconfig-paths to resolve the aliases before passing them to the opentelemetry hook:

// wrapper_hook.mjs
import {
  load,
  resolve as origResolve,
  getFormat,
  getSource,
} from '@opentelemetry/instrumentation/hook.mjs';

// See https://stackoverflow.com/a/75318315
import {loadConfig, createMatchPath} from 'tsconfig-paths';
import {pathToFileURL} from 'url'
const {absoluteBaseUrl, paths} = loadConfig()
const matchPath = createMatchPath(absoluteBaseUrl, paths)
const wrappedResolve = (specifier, ctx, defaultResolve) => {
  const match = matchPath(specifier, undefined, undefined, [".ts", ".tsx"]);
  const updatedSpecifier = match
    ? pathToFileURL(match).href : specifier;

  return origResolve(updatedSpecifier, ctx, defaultResolve);
}

export { load, wrappedResolve as resolve, getFormat, getSource };

and then use that wrapper as the loader instead of '@opentelemetry/instrumentation/hook.mjs':

npx tsx --experimental-loader=./wrapper_hook.mjs --import ./telemetry.mjs <your-script>.ts

I put that together in an example here where I use path aliases and was able to see spans for http requests from the auto instrumentation working: https://github.com/jpmunz/simple-node-app/tree/otel-loading-example
(Note: I saw in your example that the setup file was telemetry.ts but I was getting import-in-the-middle complaining about that so had to rename to telemetry.mjs)

@douglasg14b
Copy link
Author

douglasg14b commented Nov 4, 2024

Thank you for this!

I do want to note that I get http metrics regardless of using the experimental-loader script.

This appears to avoid a crashing error, however, I don't get anything more than http metrics, which I receive the same (And in the same detail) without this. Express does not appear to be instrumented this way. 🤔

@douglasg14b
Copy link
Author

douglasg14b commented Nov 7, 2024

Is this expected behavior? That http is instrumented (regardless of the in-the-middle usage). But express is not either way? If not, is there configuration changes I should be trying?

@douglasg14b
Copy link
Author

douglasg14b commented Nov 7, 2024

@jpmunz Using the method described in the linked issue resolve the problem for me.

  1. I no longer need to use --experimental-loader=./optWrapperHook.mjs
  2. I can use telemetry.ts now without issue
  3. http is now instrumented correctly and includes paths & status codes
/*instrumentation.ts*/
import { getNodeAutoInstrumentations } from '@opentelemetry/auto-instrumentations-node';
import { PrometheusExporter } from '@opentelemetry/exporter-prometheus';
import { Resource } from '@opentelemetry/resources';
import { NodeSDK } from '@opentelemetry/sdk-node';
import { ATTR_SERVICE_NAME, ATTR_SERVICE_VERSION } from '@opentelemetry/semantic-conventions';
import { createAddHookMessageChannel, Hook } from 'import-in-the-middle';
import { register } from 'module';

// Yes, createAddHookMessageChannel is new. See below.
const { registerOptions, waitForAllMessagesAcknowledged } = createAddHookMessageChannel();
register('import-in-the-middle/hook.mjs', import.meta.url, registerOptions);

const sdk = new NodeSDK({
    resource: new Resource({
        [ATTR_SERVICE_NAME]: 'ivr3-api',
        [ATTR_SERVICE_VERSION]: '1.0',
    }),
    metricReader: new PrometheusExporter({
        host: '0.0.0.0',
        port: 9464,
    }),
    instrumentations: [
        getNodeAutoInstrumentations({
            '@opentelemetry/instrumentation-http': {},
            '@opentelemetry/instrumentation-express': {},
        }),
    ],
});

sdk.start();

await waitForAllMessagesAcknowledged();

@jpmunz
Copy link

jpmunz commented Nov 11, 2024

nice! If that worked it seems like the way to make this easier going forward, as that other issues suggests, is with module.register as opposed to the experiment loader stuff that is currently recommended

@jpmunz
Copy link

jpmunz commented Nov 11, 2024

Is this expected behavior? That http is instrumented (regardless of the in-the-middle usage). But express is not either way? If not, is there configuration changes I should be trying?

@douglasg14b for this I'd recommended filing another issue, if you aren't getting telemetry from express either way that seems separate from the original problem outlined here around path aliases

@dyladan
Copy link
Member

dyladan commented Nov 20, 2024

This seems to be resolved. I'm closing this, but anyone who finds this should refer to #4933. Please create a new issue for the http/express/config issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs:reproducer This bug/feature is in need of a minimal reproducer triage
Projects
None yet
Development

No branches or pull requests

3 participants