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

decide what to do for fastify auto-instrumentation #556

Open
trentm opened this issue Jan 23, 2025 · 4 comments · May be fixed by #616
Open

decide what to do for fastify auto-instrumentation #556

trentm opened this issue Jan 23, 2025 · 4 comments · May be fixed by #616

Comments

@trentm
Copy link
Member

trentm commented Jan 23, 2025

OTel JS contrib has instr-fastify for v3, v4, v5. It is being deprecated, disabled by default in auto-instrumentations-node and will eventually be removed in favour of fastify/otel (from the fastify community). The new fastify/otel supports only v5 for now. It is also not auto-instrumentation: the user must manually await fastifyApp.register(fastifyInstrumentation.plugin());

This issue is about what we want EDOT Node.js to do for fastify instrumentation.

@david-luna
Copy link
Member

By looking at the adoption dashboard:

  • there is a 4-5% of elastic-apm-node agents that are instrumenting fastify
  • almost no OTEL agents are using @opentelemetry/instrumentation-fastify

I think having the instrumentation by default would encourage that 4-5% to switch from elastic-apm-node to EDOT since in most cases the switch will basically consist on install EDOT and configure without doing any change to the app code.

If we do not add it the switch process would require an extra step which is adding the instrumentation manually. If they're using require(..).start() method at the top of the main file to instrument the app they need to actually change the code (remove it). And adding that extra line is part of that switch task. We do not have numbers on which start method is used for fastify apps but I guess part of them is using the require/start method. So is not a bad idea just to include in the docs a specific section for fastify users.

One of the possible drawbacks (if EDOT does not include the instr) would be that fastify apps prior to v5 won't have instrumentation at all. I doubt the fastify team will add compatibility to previous versions in its OTEL plugin. However the user is capable of adding it by:

  • installing @opentelemetry/instrumentation-fastify
  • setting OTEL_NODE_ENABLED_INSTRUMENTATIONS=fastify,http,...

Is not complicated but they will need to list all instrumentations just to get fastify in. Maybe if we get a better system to select and append instrumentations via env (ref: #481) we could provide this solution to users of old fastify versions.

@trentm
Copy link
Member Author

trentm commented Feb 7, 2025

Is not complicated but they will need to list all instrumentations just to get fastify in.

We could consider the + syntax: OTEL_NODE_ENABLED_INSTRUMENTATIONS=+fastify.

So is not a bad idea just to include in the docs a specific section for fastify users.

Definitely agree with this.
I think we should have a specific doc/section for a lot of the instrumentations/packages/frameworks.

I don't yet have a feel for the usage of the new fastify otel instrumentation.
For example, could we do this:

  1. Have it so @opentelemetry/instrumentation-fastify is on by default for fastify@4 users. (Do we need to fork that instrumentation to support this?)
  2. Have it so @fastify/otel is automatically on by default for fastify@5 and later users. (Can we add a @elastic/auto-instrumentation-fastify, or whatever package name, that does the small RITM patching to add the Fastify otel plugin to any created fastify app?)
  3. Have a config option that says "please make it work the same was as vanilla otel". (I've been thinking about having a ELASTIC_OTEL_COMPATIBLE=true option that is a general boolean that makes EDOT work like vanilla OTel as much as possible. Something like Vim's compatible option. I'm not sure if this is a useful idea.

@trentm
Copy link
Member Author

trentm commented Feb 7, 2025

We are planning to add support for Fastify v4 for @fastify/otel

From opentelemetry-js-contrib issue 2647: the tracker for instr-fastify changes.

@trentm
Copy link
Member Author

trentm commented Feb 7, 2025

David and I discussed:

  • For GA we are NOT going to consider the + syntax for OTEL_NODE_ENABLED_INSTRUMENTATIONS.
  • For GA: Dothe same as vanilla. Have a doc for fastify that shows the (long) OTEL_NODE_(EN|DIS)ABLED_INSTRUMENTATIONS=... to use.

Later consider:

  1. Adding a new @elastic/opentelemetry-instrumentation-fastify instr that uses @fastify/otel to automatically turn on Fastify instr. It is possible that @fastify/otel eventually tries to do auto-instr? User would use OTEL_NODE_DISABLED_INSTRUMENTATIONS=@elastic/opentelemetry-instrumentation-fastify to disable it.
  2. Also consider ELASTIC_OTEL_INSTRUMENTATION_FASTIFY_ENABLED=true (true by default in EDOT Node.js) to enable/disable this instrumentation. This envvar name is inspired by Java's otel.instrumentation.[shortname].enabled=false|true config property. However, this is getting a little off track for just fastify.

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

Successfully merging a pull request may close this issue.

2 participants