Skip to content

Conversation

@wpessers
Copy link
Contributor

@wpessers wpessers commented Mar 2, 2025

This PR fixes #1723, fixes #1600, fixes #1611
Added all the instrumentations that exist in the otel auto-instrumentations-node metapackage. Also removed most of the defaults. Right now only defaults (besides aws-lambda and aws sdk instrumentation) left are:

dns
http
net

I can see 2 possible changes to this:

  1. Remove dns and net from the defaults. This was already discussed before in the community, and I personally also question them being enabled by default. I think opt-in for these may make more sense.
  2. Add undici to defaults. I'm personally not 100% sure about this one. This would fix users not seeing traces when using nodejs native fetch. But then again, not everyone uses native fetch.

As of now the PR is still a work in progress. I'm still working on a proposal to refactor the logic for the dynamic imports. As the wrapper code has gotten a bit bloated with all newly added instrumentations: wrapper.ts

Also, as this was discussed in the otel faas working group. The changes do indeed not noticably impact package size and cold starts. After adding the extra instrumentation packages, the package size increased by ~40KB. I ran some tests and noticed no difference in cold start times.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 2, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@wpessers wpessers marked this pull request as ready for review March 3, 2025 22:41
@wpessers wpessers requested a review from a team as a code owner March 3, 2025 22:41
@wpessers
Copy link
Contributor Author

wpessers commented Mar 4, 2025

Separate comment for readability. After discussion with maintainers I've reverted the change where we alter the default instrumentations. This PR's purpose is only to support more out-of-the-box instrumentation and therefore we don't want it to be breaking. Removing defaults is, however, breaking so that will be a separate PR.

@wpessers
Copy link
Contributor Author

wpessers commented Mar 4, 2025

As requested I also ran some quick tests to see impact on the actual OTEL wrapper initialization times.

  • In my testing, one lambda uses the OTEL_NODE_ENABLED_INSTRUMENTATIONS env var to only load the http instrumentation. I will refer to this lambda as the light lambda.
  • The other one does not set the env var and thus loads all instrumentations, I will refer to this lambda as the heavy lambda.

These are the results after running 40 times (data in ms):

Statistic light_old light_new heavy_old heavy_new
Min 194 196 230 219
Max 260 265 358 341
Median 211 228 253 270.5
Mean 214.475 228.525 259.7 271

Here's the raw data: otel_wrapper_init_times_ms.csv

Sample size n=40 may not be a lot. But these results do look assuring that the changes have minimal to no impact on wrapper initialization time. If further testing is required, I can of course alter the parameters and/or sample size to get more data.

@wpessers
Copy link
Contributor Author

wpessers commented Mar 8, 2025

Fixed @serkan-ozal's comment about cucumber instrumentation. Also removed instrumentation for generic-pool and lru-memoizer as these don't make sense to use in lambda functions.

Copy link
Member

@pragmaticivan pragmaticivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

https://media.giphy.com/media/3o7abB06u9bNzA8lu8/source.gif

@pragmaticivan
Copy link
Member

Btw, it seems the failures in check-links are legit, so we probably need to fix these links later in a new PR. Not related to this PR though.

@tylerbenson tylerbenson added the javascript Pull requests that update Javascript code label Mar 12, 2025
@serkan-ozal serkan-ozal merged commit a4e8697 into open-telemetry:main Mar 12, 2025
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

javascript Pull requests that update Javascript code

Projects

None yet

4 participants