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

feat: Add esbuild plugin for auto instrumentation (https://github.com/open-telemetry/opentelemetry-js/issues/4174) #1856

Open
wants to merge 73 commits into
base: main
Choose a base branch
from

Conversation

drewcorlin1
Copy link
Contributor

@drewcorlin1 drewcorlin1 commented Dec 10, 2023

Which problem is this PR solving?

Providing an esbuild plugin for auto instrumentation open-telemetry/opentelemetry-js#4174

Short description of the changes

This change adds a proof of concept for an esbuid plugin to auto instrument node JS apps. The approach at a high level is:

  1. Configure the packages + versions we care about intercepting + adding the instrumentation to (metapackages/esbuild-plugin-node/src/config), this leverages the getModuleDefinitions() methods on instrumentation classes.
  2. For matching packages, load the existing source, wrapping it in an IIFE and then getting the exported contents of that module
  3. Instantiating the relevant plugin (ie FastifyInstrumentation) and passing the existing module exports to the relevant InstrumentationNodeModuleDefinition's patch function (this gets a little more tricky for things like the AWS SDK that has many packages going on, but largely follows the same flow as the existing auto instrumentation).

There are a number of things worth noting

  • This approach ignores built in modules (ie instrumenting fs or fs/promises). The current assumption is that you will use existing auto-instrumentation from this repo for the built-in functions since they are not subject to the same issues with bundlers and the require()-intercepting based approach of patching third party modules (the problem is outlined in more detail in Support instrumentation of modules via bundler plugins opentelemetry-js#4174).
  • Configuration for the plugins is limited, notably in that functions will not be passed to the underlying plugins.

I'm hoping to discuss any concerns with the points noted above or anything else before expanding support to all modules in this repo. Let me know if I can provide any additional information!

The README and some descriptions are slightly misleading until these points are discussed.

Example

The plugin would be used as follows (assuming my app is in src/server.ts).

import { openTelemetryPlugin } from '@opentelemetry/esbuild-plugin-node';
import { build } from 'esbuild';

build({
  entryPoints: ['src/server.ts'],
  bundle: true,
  outfile: 'dist/server.js',
  target: 'node20',
  platform: 'node',
  sourcemap: true,
  plugins: [
    openTelemetryPlugin({
      externalModules: ['#node-web-compat'], // This package also causes jest issues
      pathPrefixesToIgnore: ['~/'], // We configure this to alias `src/` via `tsconfig#compilerOptions.paths`
      instrumentationConfig: {
        '@opentelemetry/instrumentation-aws-sdk': {
          suppressInternalInstrumentation: true
        },
      },
    }),
  ],
})

@drewcorlin1 drewcorlin1 requested a review from a team as a code owner December 10, 2023 22:10
@drewcorlin1 drewcorlin1 changed the title Add esbuild plugin for auto instrumentation (https://github.com/open-telemetry/opentelemetry-js/issues/4174) feat: Add esbuild plugin for auto instrumentation (https://github.com/open-telemetry/opentelemetry-js/issues/4174) Dec 10, 2023
@drewcorlin1 drewcorlin1 force-pushed the esbuild-plugin branch 5 times, most recently from 4f0e3fc to 3390bd6 Compare December 12, 2023 03:55
@drewcorlin1
Copy link
Contributor Author

Alternatively, maybe we could just not allow passing functions to the configs (like logHook) and only allow simple options (strings, booleans, numbers, etc) since this would likely just trip people up.

@drewcorlin1
Copy link
Contributor Author

drewcorlin1 commented Dec 17, 2023

I removed config parameters that are functions, still need to update types accordingly

@drewcorlin1
Copy link
Contributor Author

@blumamir @haddasbronfman could you take a look when you get a chance/let me know if I should be tagging someone else? Thanks!

@drewcorlin1
Copy link
Contributor Author

@pichlermarc apologies for the random tag, but I'm not sure who else to tag to get a review/discussion started on this. Could you help me out with that?

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Hi @drewcorlin1, thanks for reaching out. 🙂
This does look like a really cool feature, I have not had the time yet to look into this in detail, a few notes though:

  • keep in mind that with any new component like this we'll also need people to act as code owners for it, for that please add the new package to .github/component_owners.yml
    • ideally we'd need two or more people to maintain this new package, usually that's the author, as well as another person that's familiar with the tooling and the new plugin.
  • I think before accepting this new component we should discuss this with the other SIG members as I expect adding this to entail significant additional maintenance effort.
    • I will add this to the agenda for our next SIG meeting (Jan 04, 2024 Jan 03, 2024) to discuss it there. Feel free to join if you can. You can find the link to the agenda here https://github.com/open-telemetry/community#implementation-sigs (under JavaScript: SDK), the link to the meeting is in the linked Google Doc 🙂

@drewcorlin1
Copy link
Contributor Author

@pichlermarc thank you! I'm happy to join the SIG meeting. Is it Jan 3 or Jan 4 though, that link says meetings are on Wednesdays and this link says the Javascript SIG is on the 3rd?

@trentm
Copy link
Contributor

trentm commented Dec 22, 2023

Is it Jan 3 or Jan 4 though

Jan 3rd is Wednesday, no?

@drewcorlin1
Copy link
Contributor Author

drewcorlin1 commented Dec 22, 2023

Is it Jan 3 or Jan 4 though

Jan 3rd is Wednesday, no?

Yep, just that his message said the 4th. I'll plan on coming to the Javascript SIG meeting on the 3rd

@trentm
Copy link
Contributor

trentm commented Dec 22, 2023

Yep, just that his message said the 4th

Ah, duh. I misread. Yah, it'll be the 3rd. Or if not, you and I can hang out for a few minutes then come back the next day. :)

@pichlermarc
Copy link
Member

@drewcorlin1 Ah, yes 4th was a typo - it's January 3rd; sorry about the confusion.

@drewcorlin1
Copy link
Contributor Author

@Flarna could I bother you for another review on this? I'm trying to keep this up to date with main but am struggling to get reviews on it

@drewcorlin1
Copy link
Contributor Author

@pichlermarc @trentm bump on this please. I really want to push this along. If there's anything I can do (like working on similar plugins for other bundlers) I'm happy to do it, I just need some feedback/help moving this along.

@trentm
Copy link
Contributor

trentm commented Jun 12, 2024

@drewcorlin1 I've set a time for this coming Monday to do a first review of this.

@drewcorlin1
Copy link
Contributor Author

@drewcorlin1 I've set a time for this coming Monday to do a first review of this.

thank you so much!

@trentm
Copy link
Contributor

trentm commented Jun 18, 2024

@drewcorlin1 I have started reviewing this, but I sure didn't give myself enough time today. :) I'll keep working on it this week.

@drewcorlin1
Copy link
Contributor Author

@drewcorlin1 I have started reviewing this, but I sure didn't give myself enough time today. :) I'll keep working on it this week.

All good. Happy to add context in whatever way would be helpful too, just lmk

trentm added a commit to trentm/opentelemetry-js-contrib that referenced this pull request Jun 21, 2024
@NavaceSystem
Copy link

Looking forward to this.

@trentm
Copy link
Contributor

trentm commented Jun 24, 2024

@drewcorlin1 Again sorry for the delay. We had a hackathon week at work last week and I spent most of the week on work related to this. ... but I haven't written it up yet. And now I'll be away for a couple of days, so it'll be another delay before I have feedback and something to show.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet