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

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

Closed
mattcollier opened this issue May 23, 2024 · 11 comments · Fixed by open-telemetry/opentelemetry-js#4925
Assignees
Labels
bug Something isn't working information-requested

Comments

@mattcollier
Copy link
Contributor

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 mattcollier added the bug Something isn't working label May 23, 2024
@mattcollier mattcollier changed the title No modules instrumentation has been defined for '@opentelemetry/instrumentation-undici@0.2.0' [undici] No modules instrumentation has been defined for '@opentelemetry/instrumentation-undici@0.2.0' May 23, 2024
@mattcollier
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link

nhz-io commented Jun 2, 2024

The diag message is normal. instrumentation-undici does not patch any imported module:

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

@mattcollier
Copy link
Contributor Author

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
Copy link

nhz-io commented Jun 3, 2024

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()
  1. 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: open-telemetry/opentelemetry-js#3698

@mattcollier
Copy link
Contributor Author

@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
Copy link

nhz-io commented Jun 3, 2024

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: open-telemetry/opentelemetry-js#4638

@mattcollier
Copy link
Contributor Author

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
Copy link

nhz-io commented Jun 3, 2024

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)

@trentm
Copy link
Contributor

trentm commented Aug 16, 2024

^ I opened a PR to drop that diag.debug message. It isn't helpful and can cause confusion like this. As was stated in comments above, it is totally reasonable for instrumentation-undici to not ahve any modules to patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working information-requested
Projects
None yet
4 participants