open-telemetry / opentelemetry-js-contrib

OpenTelemetry instrumentation for JavaScript modules
https://opentelemetry.io
Apache License 2.0
637 stars 476 forks source link

[undici] No modules instrumentation has been defined for '@opentelemetry/instrumentation-undici@0.2.0' #2237

Open mattcollier opened 1 month ago

mattcollier commented 1 month ago

What version of OpenTelemetry are you using?

"@opentelemetry/instrumentation-undici": "^0.2.0", "@opentelemetry/sdk-node": "^0.51.1",

What version of Node are you using?

20.12.1

What did you do?

Running this minimal example: https://github.com/digitalbazaar/otel-example/blob/main/index.js

Which produces the following output with the message that: No modules instrumentation has been defined for '@opentelemetry/instrumentation-undici@0.2.0', nothing will be patched

I am not seeing any spans related to the use of undici(fetch) being produced which I assume is related to the above message.

What does one need to do to get this instrumentation package working properly?

Thank you,

Matt

matt@matt-X570-AORUS-ELITE:~/dev/otel-example$ npm start

> otel-example@0.1.0 start
> node index.js

@opentelemetry/api: Registered a global for diag v1.8.0.
No modules instrumentation has been defined for '@opentelemetry/instrumentation-undici@0.2.0', nothing will be patched
@opentelemetry/api: Registered a global for trace v1.8.0.
@opentelemetry/api: Registered a global for context v1.8.0.
@opentelemetry/api: Registered a global for propagation v1.8.0.
Accessing resource attributes before async attributes settled
EnvDetector found resource. Resource {
  _attributes: {},
  asyncAttributesPending: false,
  _syncAttributes: {},
  _asyncAttributesPromise: Promise { {} }
}
ProcessDetector found resource. Resource {
  _attributes: {
    'process.pid': 165495,
    'process.executable.name': 'node',
    'process.executable.path': '/home/matt/.nvm/versions/node/v20.12.1/bin/node',
    'process.command_args': [
      '/home/matt/.nvm/versions/node/v20.12.1/bin/node',
      '/home/matt/dev/otel-example/index.js'
    ],
    'process.runtime.version': '20.12.1',
    'process.runtime.name': 'nodejs',
    'process.runtime.description': 'Node.js',
    'process.command': '/home/matt/dev/otel-example/index.js',
    'process.owner': 'matt'
  },
  asyncAttributesPending: false,
  _syncAttributes: {},
  _asyncAttributesPromise: Promise {
    {
      'process.pid': 165495,
      'process.executable.name': 'node',
      'process.executable.path': '/home/matt/.nvm/versions/node/v20.12.1/bin/node',
      'process.command_args': [Array],
      'process.runtime.version': '20.12.1',
      'process.runtime.name': 'nodejs',
      'process.runtime.description': 'Node.js',
      'process.command': '/home/matt/dev/otel-example/index.js',
      'process.owner': 'matt'
    }
  }
}
HostDetector found resource. Resource {
  _attributes: { 'host.name': 'matt-X570-AORUS-ELITE', 'host.arch': 'amd64' },
  asyncAttributesPending: false,
  _syncAttributes: {},
  _asyncAttributesPromise: Promise {
    { 'host.name': 'matt-X570-AORUS-ELITE', 'host.arch': 'amd64' }
  }
}
mattcollier commented 1 month ago

I've been reading that instrumentation problems can sometimes be caused by ESM issues, so I re-implemented as CJS on a branch here with no benefit: https://github.com/digitalbazaar/otel-example/tree/commonJs

mattcollier commented 1 month ago

Here's the place where the "No modules..." is logged: https://github.com/open-telemetry/opentelemetry-js/blob/860e5d57466dffba402fc6be8239f923e7569b97/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts#L71

Seems like maybe this code path is looking for imported modules to instrument which I believe is not applicable to the undici instrumentation because it hooks into diagnostic channels instead.

So if that message/warning is "normal", then the question is simply "why is it not working?"

JamieDanielson commented 1 month ago

Thank you for providing the example code!

It looks like you are using the node-fetch package, not native fetch in Node.js, As far as I'm aware, the node-fetch package does not use undici, and instead is intended to be an http API for Node.js runtime. So you should be able to use the regular HttpInstrumentation here and have it work.

Native fetch in Node.js as of v18/20 uses undici under the hood, which is what this instrumentation is intended to work for. Can you confirm if the HttpInstrumentation works for you, and/or swap out node-fetch for fetch?

nhz-io commented 1 month ago

The diag message is normal. instrumentation-undici does not patch any imported module: https://github.com/open-telemetry/opentelemetry-js-contrib/blob/7272ca85625248718c6276559b36853ea93ae97e/plugins/node/instrumentation-undici/src/undici.ts#L74

You are indeed using node-fetch: https://github.com/digitalbazaar/otel-example/blob/06f50a190d6b2b2d5bdf415938757d4b72f10894/index.js#L5 which is not undici

mattcollier commented 1 month ago

Hello, All, thanks for looking at this with me. With commit 0638c7e8f241542059dd5fbcd1eca96a3e058439 I have removed the use of node-fetch and use the fetch global instead. I'm testing with Node.js v20.12.1.

I still do not see any otel console logging related to the use of the fetch API.

nhz-io commented 1 month ago

Well, lets go all the way...

Refering to: examples/esm-http-ts/index.ts as a baseline and doing slight modifications:

  1. Create an instrumentation.js file next to your index.js, and place all your instrumentation logic into it:
import { diag, DiagConsoleLogger, DiagLogLevel } from '@opentelemetry/api'

import { Resource } from '@opentelemetry/resources'
import { SEMRESATTRS_SERVICE_NAME } from '@opentelemetry/semantic-conventions'
import { ConsoleSpanExporter, SimpleSpanProcessor } from '@opentelemetry/sdk-trace-base'
import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node'
import { registerInstrumentations } from '@opentelemetry/instrumentation'
import { UndiciInstrumentation } from '@opentelemetry/instrumentation-undici'

// For troubleshooting, set the log level to DiagLogLevel.DEBUG / INFO
diag.setLogger(new DiagConsoleLogger(), DiagLogLevel.INFO)

const resource = new Resource({ [SEMRESATTRS_SERVICE_NAME]: 'otel-example' })
const exporter = new ConsoleSpanExporter()
const processor = new SimpleSpanProcessor(exporter)
const tracerProvider = new NodeTracerProvider({ resource })

tracerProvider.addSpanProcessor(processor)
tracerProvider.register()

registerInstrumentations({ instrumentations: [new UndiciInstrumentation()] })

This is needed because instrumentation-undici is an edge-case that does not really do any monkey patching. In case of actual modules that you DO want to monkey patch, you need instrumentations setup before your main imports.

  1. Leave only your main logic in index.js

    const response = await fetch('https://github.com/')
    const body = await response.text()
  2. Modify your start script to import instrumentations before the main:

    {
    "scripts": {
        "start": "node --import ./instrumentation.js index.js"
    }
    }

Then, running npm start yields:

{
  resource: {
    attributes: {
      'service.name': 'otel-example',
      'telemetry.sdk.language': 'nodejs',
      'telemetry.sdk.name': 'opentelemetry',
      'telemetry.sdk.version': '1.24.1'
    }
  },
  traceId: 'ec3e159a030a29f6c87aa64ec0afc617',
  parentId: undefined,
  traceState: undefined,
  name: 'GET',
  id: '3579d06d1473d9bb',
  kind: 2,
  timestamp: 1717446317060000,
  duration: 376098.959,
  attributes: {
    'http.request.method': 'GET',
    'http.request.method_original': 'GET',
    'url.full': 'https://github.com/',
    'url.path': '/',
    'url.query': '',
    'url.scheme': 'https',
    'server.address': 'github.com',
    'server.port': 443,
    'user_agent.original': 'node',
    'network.peer.address': '140.82.121.3',
    'network.peer.port': 443,
    'http.response.status_code': 200
  },
  status: { code: 0 },
  events: [],
  links: []
}

P.S.

Look at: https://github.com/open-telemetry/opentelemetry-js/pull/3698

mattcollier commented 1 month ago

@nhz-io Thank you! I'm getting traction. I understand why/how separating the instrumentation.js can be useful, but as a simpler minimal runnable example it does work if everything is in one file as shown here:

https://github.com/digitalbazaar/otel-example/blob/5ce90b62246aa154c189bdd265dbfaad7c341eb3/index.js#L22

I spent a few minutes trying unsuccessfully to adapt your example to using the NodeSDK constructor. Is it possible or desirable to document that method here in this issue as well?

nhz-io commented 1 month ago

For NodeSDK Use-case

The instrumentation.js would be (Nearly identical but see this issue):

import { diag, DiagConsoleLogger, DiagLogLevel } from '@opentelemetry/api'

import { Resource } from '@opentelemetry/resources'
import { SEMRESATTRS_SERVICE_NAME } from '@opentelemetry/semantic-conventions'
import { ConsoleSpanExporter, SimpleSpanProcessor } from '@opentelemetry/sdk-trace-base'
import { NodeSDK } from '@opentelemetry/sdk-node'
import { UndiciInstrumentation } from '@opentelemetry/instrumentation-undici'

// For troubleshooting, set the log level to DiagLogLevel.DEBUG / INFO
diag.setLogger(new DiagConsoleLogger(), DiagLogLevel.INFO);

const resource = new Resource({ [SEMRESATTRS_SERVICE_NAME]: 'otel-example' })
const exporter = new ConsoleSpanExporter()
const processor = new SimpleSpanProcessor(exporter)
const undiciInstrumentation = new UndiciInstrumentation()

const sdk = new NodeSDK({
  resource,
  autoDetectResources: false,
  spanProcessors: [processor],
  instrumentations: [undiciInstrumentation],
})

sdk.start()

If you dont set autoDetectResources: false, you are going to have OTEL complain about:

Accessing resource attributes before async attributes settled

See this: https://github.com/open-telemetry/opentelemetry-js/issues/4638

mattcollier commented 1 month ago

I see the example in the readme for Undici is using CommonJS syntax and also not using the NodeSDK constructor.

Is there guidance about what the preferred methods are for demonstration purposes? https://github.com/open-telemetry/opentelemetry-js-contrib/tree/main/plugins/node/instrumentation-undici

nhz-io commented 1 month ago

IMHO, make 2 demos, for CJS and ESM respectively. Both are in use (And this status is to stay this way for indefinite amount of time - there are no deprecation plans for CJS and i doubt that there ever will be)