dd-trace-js icon indicating copy to clipboard operation
dd-trace-js copied to clipboard

Add support for ES modules

Open rochdev opened this issue 6 years ago • 21 comments

Right now we rely on require to patch modules automatically. We should also support ES modules as well as the esm module.

rochdev avatar Nov 05 '18 15:11 rochdev

Any news on this? 🙂

We're experiencing some issues instrumenting redis using a node app with only native ES modules (package.json has type "module")

app.js:

import 'dotenv/config.js';
import './ddTracer.js'; // This line must come before importing any instrumented module.
import redis from 'redis';
const app = express()
// redis stuff
....

ddTracer.js:

import trace from 'dd-trace';
const tracer = trace.init({ plugins: true });
export default tracer;

Manually enabling redis does not work either:

tracer.use('redis', { service: 'redis' });

The plugins http-client, dns, tcp and express seemingly works.

Thanks 🙏

luax avatar Dec 08 '20 13:12 luax

This is still on our roadmap but we're not actively working on it. What is your use case for using ES modules?

rochdev avatar Dec 16 '20 20:12 rochdev

Thank you @rochdev. I would not say that there is one particular use case but the app is only running modern native Node.js and JS functionality and not any transpiled code.

luax avatar Dec 18 '20 07:12 luax

We're having a similar issue when using koa and redis.

It seems I can either require and init dd-trace in the main server.js file and get redis tracing but lose endpoints being tagged automatically, OR I can import './datadog'; a file with import datadogTrace from 'dd-trace'; and get automatic endpoint tagged as expected, but redis will be disabled with the trace error TypeError: Cannot read property 'prototype' of undefined at Instrumenter.patch (/app/app/node_modules/dd-trace/packages/datadog-plugin-redis/src/index.js:57:35)

For now we're erring on the side of actually getting redis tracing, but I'd really like to not have to manually tag my endpoints, too.

provstevi avatar Dec 18 '20 22:12 provstevi

@provstevi Can you clarify what the issue is when not using import? When using require everything should work out of the box. What is not tagged automatically? (assuming here that tagged means traced)

rochdev avatar Jan 05 '21 00:01 rochdev

OK, to restate, for our services that use koa, we also use esm.

If I have the following, I get everything except redis out of the box, including desired resource names from the route, and other expected plugins, e.g. servicename-http-client, servicename-tcp, servicename-dns.

// startServer.js
require = require('esm')(module);
module.exports = require('./server.js');

// server.js
import './datadog';

// datadog.js
const datadogTrace = require('dd-trace');
const tracer = datadogTrace.init();
const opentracing = require('opentracing');
opentracing.initGlobalTracer(tracer);

Debug reports the following error:

TypeError: Cannot read property 'prototype' of undefined
    at Instrumenter.patch (/app/app/node_modules/dd-trace/packages/datadog-plugin-redis/src/index.js:57:35)
    at Instrumenter.patch (/app/app/node_modules/dd-trace/packages/dd-trace/src/instrumenter.js:219:36)
    at /app/app/node_modules/dd-trace/packages/dd-trace/src/platform/node/loader.js:101:52
    at Array.forEach (<anonymous>)
    at /app/app/node_modules/dd-trace/packages/dd-trace/src/platform/node/loader.js:97:14
    at Map.forEach (<anonymous>)
    at Loader._hookModule (/app/app/node_modules/dd-trace/packages/dd-trace/src/platform/node/loader.js:92:8)
    at Object.Module.require (/app/app/node_modules/require-in-the-middle/index.js:83:30)
    at Object.t (/app/app/node_modules/esm/esm.js:1:279775)
    at n (/app/app/node_modules/esm/esm.js:1:279589)
    at Object.<anonymous> (/app/app/node_modules/redis/lib/debug.js:3:13)
    at Object.<anonymous> (/app/app/node_modules/esm/esm.js:1:251206)
    at Object.<anonymous> (/app/app/node_modules/esm/esm.js:1:287446)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Object.Ph (/app/app/node_modules/esm/esm.js:1:286187)
    at p (/app/app/node_modules/esm/esm.js:1:287490)
TypeError: Cannot read property 'prototype' of undefined
    at Instrumenter.unpatch (/app/app/node_modules/dd-trace/packages/datadog-plugin-redis/src/index.js:60:37)
    at /app/app/node_modules/dd-trace/packages/dd-trace/src/instrumenter.js:229:35
    at Set.forEach (<anonymous>)
    at Instrumenter.unpatch (/app/app/node_modules/dd-trace/packages/dd-trace/src/instrumenter.js:227:20)
    at /app/app/node_modules/dd-trace/packages/dd-trace/src/instrumenter.js:197:14
    at Array.forEach (<anonymous>)
    at Instrumenter.unload (/app/app/node_modules/dd-trace/packages/dd-trace/src/instrumenter.js:196:8)
    at /app/app/node_modules/dd-trace/packages/dd-trace/src/platform/node/loader.js:106:30
    at Map.forEach (<anonymous>)
    at Loader._hookModule (/app/app/node_modules/dd-trace/packages/dd-trace/src/platform/node/loader.js:92:8)
    at Object.Module.require (/app/app/node_modules/require-in-the-middle/index.js:83:30)
    at Object.t (/app/app/node_modules/esm/esm.js:1:279775)
    at n (/app/app/node_modules/esm/esm.js:1:279589)
    at Object.<anonymous> (/app/app/node_modules/redis/lib/debug.js:3:13)
    at Object.<anonymous> (/app/app/node_modules/esm/esm.js:1:251206)
    at Object.<anonymous> (/app/app/node_modules/esm/esm.js:1:287446)
Error while trying to patch redis. The plugin has been disabled.

and I get the following integrations loaded

"integrations_loaded":["http","https","fs","net","dns","[email protected]","http2","[email protected]","[email protected]","[email protected]","[email protected]"]

If I instead do the following, I get only redis out of the box. I added logic to manually tag the endpoints so I got that to mostly work, but no tracing of any other upstream calls, which really clouds my observations:

// server.js
const datadogTrace = require('dd-trace');
const tracer = datadogTrace.init();
const opentracing = require('opentracing');
opentracing.initGlobalTracer(tracer);

and I get the following integrations:

"integrations_loaded":["[email protected]","[email protected]","[email protected]","[email protected]","[email protected]","[email protected]"]

With this config, I had to manually setting the http.route tag to get the resources to show up in APM, and I really don't want to have to try to manually tag the other plugins, but at the moment I have half the picture of what's going on in my service.

In our services that use express, which do not use esm, we are getting redis, express, and the other plugins we expect with no issue.

provstevi avatar Jan 05 '21 04:01 provstevi

I also have a support ticket open for this - 426982

provstevi avatar Jan 05 '21 04:01 provstevi

@provstevi Gotcha, thanks for the clarification! I expected an all or nothing situation, not individual plugins working and other not working. Definitely something we'll be looking into at some point.

One thing we've been trying to understand is the use case for using ESM over CommonJS. Can you share why you're using ESM?

rochdev avatar Jan 05 '21 21:01 rochdev

@rochdev it was a technical decision made by one our teams, and is now baked pretty deeply into several supporting packages. Same reason we have some services using koa and some using express.

NB: ES Modules are stable in node as of 15.3.

provstevi avatar Jan 06 '21 02:01 provstevi

We also just ran into this issue as well. A potential workaround would be to migrate to Typescript (there's some tooling for this) but support for ESM would be fantastic until we can do that.

DaedalusX avatar Apr 02 '21 08:04 DaedalusX

I've also hit this issue. We have some isomorphic code that uses ESM both in Node and in modern browsers (without transpilation), so we just use ESM throughout the project. It would be too much work to revisit that decision now just to get Datadog tracing to work.

richard-riverford avatar Jun 11 '21 09:06 richard-riverford

We're working on a plan right now to support ESM soon. Since ESM in Node only supports a single loader hook, it's a bit complicated because it means we need to figure out a way to basically hook into any existing hook. While there is work to address that in Node itself, we still need a workaround that can land beforehand, especially since we support older versions of Node. Supporting this also means a rewrite of some core components of the tracer which will be non-trivial. However, we'll be prioritizing this soon as usage of ESM has been increasing. I will update this issue as progress is made.

rochdev avatar Jun 11 '21 16:06 rochdev

Experimental support for ESM is in dd-trace@>=1.3.0. Simply add the loader hook hook using the following CLI argument to Node.js.

--experimental-loader dd-trace/loader-hook.mjs

Please contact us directly at [email protected] if any issues arise.

bengl avatar Sep 15 '21 21:09 bengl

Experimental support for ESM is in dd-trace@>=1.3.0. Simply add the loader hook hook using the following CLI argument to Node.js.

--experimental-loader dd-trace/loader-hook.mjs

Please contact us directly at [email protected] if any issues arise.

I think this information should includes somewhere in the doc.

luokuning avatar Dec 19 '21 10:12 luokuning

I think this information should includes somewhere in the doc.

We're still working on the functionality which is still in experimental stage, but we'll definitely add it to the doc once it's stable. There are a few known issues right now that can cause problems which we are actively working on. We definitely recommend testing this thoroughly before going to production with the loader, and to contact us if there are any issues.

rochdev avatar Dec 20 '21 14:12 rochdev

Great; thanks folks.


From: Roch Devost @.> Sent: 20 December 2021 14:58 To: DataDog/dd-trace-js @.> Cc: Richard Turner @.>; Comment @.> Subject: Re: [DataDog/dd-trace-js] Add support for ES modules (#349)

I think this information should includes somewhere in the doc.

We're still working on the functionality which is still in experimental stage, but we'll definitely add it to the doc once it's stable. There are a few known issues right now that can cause problems which we are actively working on. We definitely recommend testing this thoroughly before going to production with the loader, and to contact us if there are any issues.

— Reply to this email directly, view it on GitHubhttps://github.com/DataDog/dd-trace-js/issues/349#issuecomment-997997730, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AF3XTKHQRGXW72DW6UI7L6DUR4775ANCNFSM4GB2V2HQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you commented.Message ID: @.***>

[https://media.riverford.co.uk/email/email-footer-christmas-2021.jpg] Riverford Organic Farmers Ltd, Buckfastleigh, Devon, TQ11 0JU, UK Registered in England No. 3731570

richard-riverford avatar Dec 20 '21 15:12 richard-riverford

When can we expect support for Node >= 16.12.1 where the loader API changed? Trying to add the hook with dd-trace 1.7.0 on Node 16.13.1 outputs

(node:391569) DeprecationWarning: Obsolete loader hook(s) supplied and will be ignored: getFormat, getSource

...and the process exits immediately.

robaca avatar Jan 28 '22 11:01 robaca

@robaca I believe that was fixed a little while ago, maybe it was just not released. We're releasing 2.0 soon which will include everything on master, so I'd say to try that version when it lands (should be later today).

rochdev avatar Jan 28 '22 19:01 rochdev

Hi @rochdev it seems that the immediate exit of our service has nothing to do with the deprecation warning. It's also happening with the older Node 16.11.0 as soon as we add the loader argument - difficult to troubleshoot.

robaca avatar Jan 31 '22 16:01 robaca

We found the root cause of the immediate exit: we had a circular dependency between module files. After fixing that, the server started as expected. It would be great if the loader could at least render some descriptive error message in such a case.

robaca avatar Feb 22 '22 09:02 robaca

@rochdev

For example I cannot even start my project with --experimental-loader hook. It just gets stuck. Other loaders work ok. Though this may be due to TypeScript 4.7.4, top level await and usage of reflect-metadata and libraries dependent on reflect-metadata.

Most NodeJS ESM projects have packages which are compiled as CommonJS modules. I have pinpointed issues in profiler. It gets stuck on ritm.js: 51:56 when trying to process @aws-sdk v3.

Everything works ok on old CommonJS Node 12 project with aws sdk v2 written in pure JS.

It seems like aws-sdk v3 is the culprit when loader loads and tries to instrument code from libraries written and published in CommonJS format.

duki994 avatar Sep 29 '22 11:09 duki994

@rochdev

There are no dd-trace startup logs when using standard Node ESM loader. When using experimental, there are logs, but service won't start.

dd-trace: 3.3.1, Node: 16.17.0

duki994 avatar Oct 03 '22 07:10 duki994

@duki994 We're working on refactoring our tests so that eventually we can test each integration with ESM, but this is a several months long project. In the meantime, can you provide a reproduction so we can at least address the immediate issue?

rochdev avatar Oct 03 '22 19:10 rochdev

@rochdev

I can give you V8 profiling logs during startup of project.

Raw log v8-10-4-22_2-44-50-PM-.log

Parsed txt statistics v8_statistics.txt

Generally, sample project I made to try and demonstrate issue cannot demonstrate it, because it doesn't occur in sample project.

duki994 avatar Oct 04 '22 13:10 duki994

@duki994 is this still happening with the latest v4 dd-trace release?

khanayan123 avatar Aug 29 '23 17:08 khanayan123

Closing due to inactivity, if you are still having this problem please feel free to reopen

khanayan123 avatar Sep 06 '23 17:09 khanayan123