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

Cannot plug dd-trace to NextJS instrumentation module #3457

Open
thollander opened this issue Jul 25, 2023 · 26 comments
Open

Cannot plug dd-trace to NextJS instrumentation module #3457

thollander opened this issue Jul 25, 2023 · 26 comments
Labels
bug Something isn't working integration-nextjs issues relating to the Next.js framework from Vercel

Comments

@thollander
Copy link

Hello,

I'd like to use the predefined configuration of NextJS instrumentation (experimental feature) to plug dd-trace.

This way of working permits to have the dependency really used inside the application and to benefit from NextJS Standalone feature (because if we pass through the NODE_OPTIONS="-r dd-trace/init", dd-trace gets removed on production Docker build as NextJS considers it's not "unused").

Not really sure on if this issue should be open on dd-trace side or NextJS, but as the error seems to be coming from dd-trace file, I begin to open it here.

Expected behaviour
I would like to be able to plug instrumentation without error.

Actual behaviour
We currently have issues on requiring some "nodejs" module inside dd-trace.

Steps to reproduce
Repro : https://github.com/thollander/repro-datadog-nextjs

yarn install 
yarn build

Here is the stack trace :

❯ yarn build

> @ build /Users/TERENCE/Dev/workspace-perso/repro-datadog-nextjs
> next build

- warn You have enabled experimental feature (instrumentationHook) in next.config.js.
- warn Experimental features are not covered by semver, and may cause unexpected or broken application behavior. Use at your own risk.

Failed to compile.

./node_modules/@datadog/native-iast-rewriter/js/source-map/index.js:2:0
Module not found: Can't resolve 'path'

https://nextjs.org/docs/messages/module-not-found

Import trace for requested module:
./node_modules/@datadog/native-iast-rewriter/main.js
./node_modules/dd-trace/packages/dd-trace/src/appsec/iast/taint-tracking/rewriter.js
./node_modules/dd-trace/packages/dd-trace/src/appsec/iast/taint-tracking/index.js
./node_modules/dd-trace/packages/dd-trace/src/appsec/iast/index.js
./node_modules/dd-trace/packages/dd-trace/src/proxy.js
./node_modules/dd-trace/packages/dd-trace/src/index.js
./node_modules/dd-trace/packages/dd-trace/index.js
./node_modules/dd-trace/index.js
./instrumentation.ts

./node_modules/@datadog/native-iast-rewriter/js/source-map/index.js:3:0
Module not found: Can't resolve 'fs'

https://nextjs.org/docs/messages/module-not-found

Import trace for requested module:
./node_modules/@datadog/native-iast-rewriter/main.js
./node_modules/dd-trace/packages/dd-trace/src/appsec/iast/taint-tracking/rewriter.js
./node_modules/dd-trace/packages/dd-trace/src/appsec/iast/taint-tracking/index.js
./node_modules/dd-trace/packages/dd-trace/src/appsec/iast/index.js
./node_modules/dd-trace/packages/dd-trace/src/proxy.js
./node_modules/dd-trace/packages/dd-trace/src/index.js
./node_modules/dd-trace/packages/dd-trace/index.js
./node_modules/dd-trace/index.js
./instrumentation.ts

./node_modules/@datadog/native-iast-rewriter/wasm/wasm_iast_rewriter.js:506:0
Module not found: Can't resolve 'path'

https://nextjs.org/docs/messages/module-not-found

Import trace for requested module:
./node_modules/@datadog/native-iast-rewriter/main.js
./node_modules/dd-trace/packages/dd-trace/src/appsec/iast/taint-tracking/rewriter.js
./node_modules/dd-trace/packages/dd-trace/src/appsec/iast/taint-tracking/index.js
./node_modules/dd-trace/packages/dd-trace/src/appsec/iast/index.js
./node_modules/dd-trace/packages/dd-trace/src/proxy.js
./node_modules/dd-trace/packages/dd-trace/src/index.js
./node_modules/dd-trace/packages/dd-trace/index.js
./node_modules/dd-trace/index.js
./instrumentation.ts

./node_modules/@datadog/native-iast-rewriter/wasm/wasm_iast_rewriter.js:507:0
Module not found: Can't resolve 'fs'

https://nextjs.org/docs/messages/module-not-found

Import trace for requested module:
./node_modules/@datadog/native-iast-rewriter/main.js
./node_modules/dd-trace/packages/dd-trace/src/appsec/iast/taint-tracking/rewriter.js
./node_modules/dd-trace/packages/dd-trace/src/appsec/iast/taint-tracking/index.js
./node_modules/dd-trace/packages/dd-trace/src/appsec/iast/index.js
./node_modules/dd-trace/packages/dd-trace/src/proxy.js
./node_modules/dd-trace/packages/dd-trace/src/index.js
./node_modules/dd-trace/packages/dd-trace/index.js
./node_modules/dd-trace/index.js
./instrumentation.ts

./node_modules/@datadog/pprof/out/src/heap-profiler-bindings.js:42:0
Module not found: Can't resolve 'path'

https://nextjs.org/docs/messages/module-not-found

Import trace for requested module:
./node_modules/@datadog/pprof/out/src/heap-profiler.js
./node_modules/@datadog/pprof/out/src/index.js
./node_modules/dd-trace/packages/dd-trace/src/profiling/profiler.js
./node_modules/dd-trace/packages/dd-trace/src/profiling/index.js
./node_modules/dd-trace/packages/dd-trace/src/profiler.js
./node_modules/dd-trace/packages/dd-trace/src/proxy.js
./node_modules/dd-trace/packages/dd-trace/src/index.js
./node_modules/dd-trace/packages/dd-trace/index.js
./node_modules/dd-trace/index.js
./instrumentation.ts


> Build failed because of webpack errors
- info Creating an optimized production build . ELIFECYCLE  Command failed with exit code 1.

Environment

  • Operation system: MAC/Linux
  • Node.js version: 18
  • Tracer version: 4.8.1
  • Agent version: ?
  • Relevant library versions:nextJS@13.4.12
@thollander thollander added the bug Something isn't working label Jul 25, 2023
@Meemaw
Copy link

Meemaw commented Jul 25, 2023

You should init dd-trace conditionally like this:

if (process.env.NEXT_RUNTIME === "nodejs") {
  // init dd-trace
}

That being said, I've noticed that when you instrument dd-trace like this, next.request spans are lost. I believe this is because instrumentation hook is called too late, after the server has been started.

@thollander
Copy link
Author

Thanks for advising, but sadly it still doesn't work (updated the repro repo).
I have the same error as previously.

@tlhunter tlhunter added the integration-nextjs issues relating to the Next.js framework from Vercel label Jul 26, 2023
@zomgbre
Copy link

zomgbre commented Aug 8, 2023

I was able to have success with this:
File is in src/instrumentation.ts

export async function register() {
  if (process.env.NEXT_RUNTIME === "nodejs") {
    const tracerLib = await import("dd-trace");
    const tracer = tracerLib.default;

    tracer.init({ logInjection: true });
    tracer.use("next");
  }
}

There are still weird things about it which I will open a separate ticket for. (Log injection for pino not quite working right. Not sure if I need the tracer.use piece. Also resource name in APM showing as GET/POST and not the path.)

It is also important to have ENV variables set appropriately for your configuration.

@thollander
Copy link
Author

Thanks @zomgbre, it works indeed. 👍🏻
And I have the same issue as you do with resource being the HTTP method instead of the path.
image

However, the spans traces are fine with correct URL.

@Quramy
Copy link

Quramy commented Oct 2, 2023

That being said, I've noticed that when you instrument dd-trace like this, next.request spans are lost.

I have the same issue.

Are there some workarounds for this?

@psimk
Copy link

psimk commented Oct 3, 2023

hey, we're also having the same issue after upgrading to Next 13.5, are there any workarounds for this?

@dariuskul
Copy link

Any updates on this?

@Kevin-McGonigle
Copy link

Kevin-McGonigle commented Jan 15, 2024

As suggested by @Qard here, one possible solution might be using the loader hook provided by dd-trace to support ESM: node --loader dd-trace/loader-hook.mjs your-app.js.

I have, however, found that the loader-hook.mjs file is being tree-shaken during the standalone build (or something similar), and have yet to find a way to get it to stick around 😅 If anyone does get this working, please let us know!

@Kevin-McGonigle
Copy link

A workaround I've just found to have the resource names set correctly is to set it via the http plugin:

export async function register(): Promise<void> {
  if (process.env.NEXT_RUNTIME === 'nodejs') {
    const tracerLib = await import('dd-trace');
    const tracer = tracerLib.default;

    tracer.init({
      appsec: true,
      logInjection: true,
      runtimeMetrics: true,
    });

    tracer.use('http', {
      hooks: {
        request(span, req) {
          if (span && req) {
            const url = 'path' in req ? req.path : req.url;
            if (url) {
              const method = req.method;
              span.setTag('resource.name', method ? `${method} ${url}` : url);
            }
          }
        },
      },
    });
  }
}

@jamesbrooks94
Copy link

After MONTHS of not being able to get log injection with winston working, adding it to this next.config.js configuration worked! https://nextjs.org/docs/app/api-reference/next-config-js/serverComponentsExternalPackages

@seanyboy49
Copy link

hey @jamesbrooks94 I saw yesterday you had a public repo with your next/datadog/winston implementation that is now private. Do you mind sharing how you got it working? I also am using a standalone build and saw that you were doing a couple things I think.

  • initializing the tracer in a separate file so you can both init it in the instrumentation file and also import it into other files to run increment with it
  • passing node options to your build command in docker

Was this right?

@mogulist
Copy link

After MONTHS of not being able to get log injection with winston working, adding it to this next.config.js configuration worked! https://nextjs.org/docs/app/api-reference/next-config-js/serverComponentsExternalPackages
Hi, @jamesbrooks94, did you do something like that? I did as below, but Error: Cannot find module 'dd-trace/init' error happens.

const nextConfig = {
  experimental: {
    serverComponentsExternalPackages: ['dd-trace/init'],
  },
}

@radum
Copy link

radum commented Apr 23, 2024

Hello everyone, I managed to hit the same dead end like most of you here. I am running Next.js 14 with app router.

The only way I managed to get it working (although not sure if it is fully working yet) is to create a JS file server-preload.js

const packageJSON = require('../package.json');

function setUpDatadogTracing() {
	const tracer = require('dd-trace');

	tracer.init({
		runtimeMetrics: true,
		logInjection: true,
		env: 'dev',
		service: `myapp`,
		version: packageJSON?.version ?? 'unknown'
	});
}

setUpDatadogTracing();

And load it within package.json node -r server-preload.js ./node_modules/.bin/next start. Doing this I don't get only GET and POST in Resources and I have GET /_not-found for 404 pages and GET /about etc etc based on the pages I have.

I am also getting the versioning coming through for each new release I make and also the dev envs are set properly.

Logs are ingested also but only the ones that I am logging via an internal logger I made via Pino. The other ones are not coming in as they are not in JSON format.

There is a way in the file above to patch the console log and make it spit out JSON but that is a can of worms because there is lots of cleaning up that needs to be done to make it work and also it could break at any Next update.

Using the instrumentation hook I never managed to get it working, and using the telemetry from Vercel plus DD I always got undefined errors looking for the _traceID in an object.

Even with this setup I am not sure if I can see any spans and I need to check more.

For sourcemaps I am thinking to generate them and load them via the CI before I remove them from the deployed app.

Has anyone found a better way that works with most DD features and can share their setup?

@aralroca
Copy link

aralroca commented May 6, 2024

Very curios that:

if (process.env.NEXT_RUNTIME === 'nodejs') { /* WORKS */ }

And:

if (process.env.NEXT_RUNTIME !== 'nodejs') return
// NOT WORK

This happens with Next.js 13.5.4. How is it possible that with early return it doesn't work? Are you parsing the AST instead of letting JS do its job? I don't understand the error, something is missing.

@Quramy
Copy link

Quramy commented Aug 26, 2024

instrumentation.ts of the latest version Next.js works correctly for me.

/* next.config.js */

/** @type {import('next').NextConfig} */
const nextConfig = {
  experimental: {
    instrumentationHook: true,
    serverComponentsExternalPackages: ['dd-trace'],
  },
}

module.exports = nextConfig
/* src/instrumentation.ts */

export async function register() {
  if (process.env.NEXT_RUNTIME === 'nodejs') {
    await import('./tracer')
  }
}
/* src/tracer.ts */

import ddtrace from 'dd-trace'

ddtrace
  .init({
    service: 'my-nextjs-app',
    // other settings,,,
  })
  .use('next')
  .use('fetch', {
    hooks: {
      // The following hook is for confirming `fetch` instruments
      request: (span, req: any, res) => {
        console.log(`[trace][fetch] ${req?.method} ${req?.url}`)
      },
    },
  })

When I checked several months ago, there was a problem with missing spans for the fetch plugin. However, this problem has been resolved by vercel/next.js#60796.

@florianmutter
Copy link

@Quramy do you get correct resource names? When I try the same with NextJS 14.2.7 and dd-trace 5.12.0 I only get resources names GET for web.request spans.

@Quramy
Copy link

Quramy commented Sep 2, 2024

@Quramy do you get correct resource names? When I try the same with NextJS 14.2.7 and dd-trace 5.12.0 I only get resources names GET for web.request spans.

I didn't check the resource names. To change resource.name tag, we can customize like [this comment] (#3457 (comment)) .

@danielleHerszfang
Copy link

Hey @Quramy, When you initialize the tacer withlogInjection: true did it work for you? (I see it currently commented out)

@Quramy
Copy link

Quramy commented Sep 10, 2024

@danielleHerszfang

I have added to my comment about logInjection.

I tried logInjection with pino and winston, but dd.trace_id anddd.span_id were not injected. 🤔

Instead of insrumentation.ts, Using NODE_OPEION='--import tracer.js', logInjection: true injects trace_id and span_id correctly.

It may be still tough to use instrumentation.ts

@scottbartell
Copy link

After hours of debugging our integration after we upgraded Next.js due to a vulnerability in Next... it turns out that in the latest version of dd-trace-js (v5.22.0 and v4.46.0) the Next.js Plugin currently does NOT support Next.js >= 14.2.7.

It sounds like tests for the plugin were failing when Next 14.2.7 came out so they just disabled the plugin for that and all later Next versions and there have been no updates since.

Here's the PR that introduced this: #4636

Here are the changes in the release for v5.22.0 and v4.46.0 that show that the PR above was included in the release.

And finally, here's the open PR that reverts that change that was opened when the PR was first introduced - it appears the hasn't been any progress since then: #4637

@EmilioAiolfi
Copy link

Hey @tlhunter, or anyone else, do you know when the issue with the Next.js versions (>= 14.2.7) and the dd-trace-js plugin will be resolved? It's currently blocking our ability to update to the latest Next.js versions due to the vulnerability patches. Any updates would be greatly appreciated!

@Jokinen
Copy link

Jokinen commented Oct 31, 2024

We do see errors in tracing with a Next.js version > 14.2.6. As far as we can tell, there's a real incompatibility issue.

I've contacted DD support about this issue. In some previous threads, DD reps have advised to contact their support as they are much more active in that channel and might not always be able to track the discussion here. To get more attention for this issue, consider opening a support ticket.

@tlhunter
Copy link
Member

@scottbartell @EmilioAiolfi @Jokinen the compatibility with Next.js >= 14.2.7 - 14.x should be ready once this PR is merged and released: #4916

@Jokinen
Copy link

Jokinen commented Nov 22, 2024

@tlhunter Thank you! I gave it a test. The plugin is now initialised as expected and traces looked to be in better order 👍

We've have to setup some customisations that didn't appear to be compatible with the new version so I can't yet confirm whether the fix works for us in full. To be clear, at this point I am not suspecting any issue in the library, but in our custom integration code instead.

Thank you for the fix 👍

@Jokinen
Copy link

Jokinen commented Nov 26, 2024

@tlhunter I can now confirm that the fix resolves the problem our team had.

Thank you again 👍

@tlhunter
Copy link
Member

@thollander are you still having these issues?

Note that you can throw require('dd-trace') in your app code as a way to sneak around the tree shaking that next.js uses.

Also, my previous comment about internal node modules missing and the tracer accidentally being loaded in a browser context also applies to your original post.

Finally, since this issue was opened we also created this document about complex framework usage and that applies here:
https://docs.datadoghq.com/tracing/trace_collection/compatibility/nodejs/#complex-framework-usage

Basically, you'll need the --require flag when starting the backend process to load the tracer early enough, you can throw a require('dd-trace') in code that only loads in the backend to prevent tree shaking, and you'll need to be careful about preventing that code from loading in browser code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working integration-nextjs issues relating to the Next.js framework from Vercel
Projects
None yet
Development

No branches or pull requests