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

[@opentelemetry/instrumentation] require performance grows linearly with each instrumentation plugin #3029

Closed
mhassan1 opened this issue Jun 9, 2022 · 13 comments · Fixed by #3161
Assignees
Labels
bug Something isn't working priority:p3 Bugs which cause problems in user apps not related to correctness (performance, memory use, etc)

Comments

@mhassan1
Copy link
Contributor

mhassan1 commented Jun 9, 2022

What version of OpenTelemetry are you using?

@opentelemetry/api@^1.1.0

What version of Node are you using?

v16.14.2

Please provide the code you used to setup the OpenTelemetry SDK

new NodeSDK({
  instrumentations: [getNodeAutoInstrumentations()]
})

What did you do?

const { getNodeAutoInstrumentations } = require('@opentelemetry/auto-instrumentations-node')
const { NodeSDK } = require('@opentelemetry/sdk-node')
const { performance: { now } } = require('perf_hooks')

new NodeSDK({
  instrumentations: [
    // toggle this
    getNodeAutoInstrumentations()
  ]
})

const start = now()

require('ajv') // or any moderately sized package

const duration = now() - start

console.log(Math.floor(duration), 'ms')

// with `instrumentations: [getNodeAutoInstrumentations()]` -> 100 ms
// with `instrumentations: []` -> 30 ms

What did you expect to see?

require should not be much slower when instrumentation is enabled

What did you see instead?

require is 3-5x slower when instrumentation is enabled

Additional context

  • We have a large Node.js application that normally takes 20-30 seconds for startup. With getNodeAutoInstrumentations(), it takes 2.5 minutes.
  • We noticed that time taken for require grows linearly with the number of enabled instrumentation plugins. In our large application, each additional instrumentation plugin adds 5-10 seconds to startup time.
  • I could be way off, but I wonder if the problem could be related to the way @opentelemetry/instrumentation uses require-in-the-middle: since each instrumentation plugin invokes RITM, does the require function end up as a deeply nested patch on top of a patch on top of a patch?
@mhassan1 mhassan1 added the bug Something isn't working label Jun 9, 2022
@mhassan1 mhassan1 changed the title [@opentelemetry/instrumentation] require performance grows linearly with each instrumentation [@opentelemetry/instrumentation] require performance grows linearly with each instrumentation plugin Jun 9, 2022
@rauno56
Copy link
Member

rauno56 commented Jun 15, 2022

I could be way off, but I wonder if the problem could be related to the way @opentelemetry/instrumentation uses require-in-the-middle: since each instrumentation plugin invokes RITM, does the require function end up as a deeply nested patch on top of a patch on top of a patch?

No... you are actually absolutely right. We've been aware of this issue from the beginning, but haven't gotten around to fixing this.

RITM just patches require every time. Each last patch is added on top of the last one.

If there's no other takes I'd like to take a stab at this.

@rauno56 rauno56 self-assigned this Jun 15, 2022
@dyladan
Copy link
Member

dyladan commented Jun 17, 2022

I think the way to handle this is to have a single wrapper that catches all of the instrumented packages and looks up the appropriate instrumentation.

@dyladan dyladan added the priority:p3 Bugs which cause problems in user apps not related to correctness (performance, memory use, etc) label Jun 28, 2022
@dyladan
Copy link
Member

dyladan commented Jun 28, 2022

Assigning priority 3 since this causes a performance issue in end user applications

@mhassan1
Copy link
Contributor Author

@rauno56 Did you ever have a chance to look into this? I created a draft PR, and I'm looking for feedback: #3161.

@rauno56
Copy link
Member

rauno56 commented Sep 5, 2022

I had a rough WIP before other stuff took over my priority. Can take a look, or make my WIP available if you are interested.

@flex-jonghyen
Copy link

flex-jonghyen commented Sep 7, 2022

I have same issue. I made nextjs app with @ant-design/icons.
@ant-deisng/icons has 700+ icons. So, it makes super slow my app.

I post 2 pic. first one is turn on this, second one turn off when first response my next app page.
It makes almost double up

Untitled

2

@mhassan1
Copy link
Contributor Author

@dyladan Thanks for resolving this. When can we expect the next release of @opentelemetry/instrumentation?

@dyladan
Copy link
Member

dyladan commented Oct 24, 2022

Today I hope

@mhassan1
Copy link
Contributor Author

mhassan1 commented Nov 1, 2022

@dyladan I'm assuming there's something blocking the release. Is there anything I can do to help?

@legendecas
Copy link
Member

Release in progress at #3340

@stebet
Copy link

stebet commented Apr 4, 2024

@dyladan Is it possible this regressed in the lates OTel package release (1.8.0)? We saw a huge jump in the startup time of one of our bigger services after updating the packages yesterday

@dyladan
Copy link
Member

dyladan commented Apr 4, 2024

Of course anything is possible, but I think it's unlikely that this particular issue regressed. Can you open a new issue for your startup time issue and provide your exact package versions and setup code?

@ishmaelthedestroyer
Copy link

ishmaelthedestroyer commented Nov 7, 2024

Confirmed that we're experiencing this same issue using 1.9.0. Boot up times for our services were 22 - 23 seconds, now taking 53 - 55 seconds. Observing this when working with @sentry/node@8.37.1 which uses this under the covers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p3 Bugs which cause problems in user apps not related to correctness (performance, memory use, etc)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants