Support for ESM

[tylersmalley]

Since v12, Node.js has supported ESM. We should investigate the best way to support ESM in the Kibana project.

Our node Babel preset current transpiles down to commonjs. Can we instead use ESM globally?
Does APM support ESM? @watson mentioned there might still be an issue hooking into the loader.

[elasticmachine]

Pinging @elastic/kibana-operations (Team:Operations)

[watson]

After just having a chat with @trentm, I think it should be possible to "hack" our implementation of the APM agent to work despite running as ESM (which officially isn't supported by the agent):

1. Since the agent currently doesn't have a way to hook into the ESM loader, we would have to manually call require() for the modules we want to instrument (and that needs to be instrumented in order for context propagation to not break) before they are imported elsewhere in the source code. I think this is doable.
2. Those modules that we want to instrument must not have separate CJS and ESM code. Else we're instrumenting the wrong code.

I think (besides core modules) that we only care about @elastic/elasticsearch and hapi - possibly also bluebird.

[trentm]

Possibly also the following. (I'm reading through Kibana's current "dependencies" set and comparing to modules for which our APM has some sort of instrumentation.)

• elasticsearch
• handlerbars

[watson]

Getting support for ESM in Kibana is becoming exceedingly important as more and more modules that we rely upon from npm has switched to being ESM only. So we're essentially stuck on older non-maintained versions of many of our dependencies which might contain bugs and security vulnerabilities.

@elastic/kibana-operations Is APM the only thing holding us back, or are there other issues?

@trentm any updates on the APM front, or is our best best still to "hack" the implementation as you suggested previously?

[spalger]

I agree with you @watson, we really need to figure out how to at least support using ESM only packages from NPM.

[trentm]

any updates on the APM front

We are currently investigating ESM support (elastic/apm-agent-nodejs#2343) but this doesn't promise full ESM support for the 8.5 timeframe. So in the near term the best bet is still the "hack", unfortunately.

On the "hack": In addition to @elastic/elasticsearch and @hapi/hapi, we will need to make sure that core modules like http, https, maybe http2 get instrumented.

[watson]

@spalger I would suggest that we create a proof-of-concept that uses the hack @trentm described. The actual hack part of the implementation shouldn't take that much longer compared to the time it takes to do the other required changes. That would give us an idea if this is shippable or if we need to wait for full APM support.

One other ask I have is that we afterwards investigate how much work it would take to backport this to 7.17. Otherwise it will make it almost impossible to properly maintain that branch.

I'll happily help with this if it's needed.

[spalger]

I'm unclear what the "hack" we're talking about is.

I agree that whatever we do here will probably need to be backported or re-implemented on 7.17.

My knowledge about what is necessary on a technical level is pretty low. I've set the "type" property in a package.json file and used import() statements instead of require() a couple times, but I would really need to do a good deal of research to figure out how to apply this to the Kibana repository. Maybe we could get together sometime and you can share your knowledge of the options we have @watson? My primary goal is ensuring that Kibana keeps working as is but can also import ESM-only modules from NPM.

Do you have an idea of what types of changes would be required in the repository besides this APM "hack" which I'm assuming would be mostly invisible to most people working in the repository?

[trentm]

The "hack" is to:

1. Manually have the Kibana JS code do require('foo') for each 'foo' module that you want/need the Node.js APM agent to instrument before any other Kibana JS code does import ... from 'foo'.
2. Also, ensure that for the 'foo' modules in question that the use of require('foo') and import ... from 'foo' results in loading the same files on disk. There are some packages that ship separate CommonJS and ESM files. This hack won't work on packages that do that.

It is possible this could all be contained to https://github.com/elastic/kibana/blob/main/packages/kbn-apm-config-loader/src/init_apm.ts but I'm not positive. The hack would be to explicitly add the following after the apm.start(...):

require('http');
require('https');
require('@hapi/hapi');
require('@elastic/elasticsearch');
// ... possibly others like bluebird (ew), handlebars

When I played with that hack a while back (notes here: https://gist.github.com/trentm/ceb76cd5b1763811fc6290dd4040819e#another-workaround-user-side-only), I had put this file in a ".cjs" file so that require is actually defined. Is that possible with the Kibana build system?

Possibly another path is to get "require" back in an ESM module file (https://nodejs.org/api/module.html#modulecreaterequirefilename), notes here: https://gist.github.com/trentm/ceb76cd5b1763811fc6290dd4040819e#a-user-workaround-use-require-for-your-esm-files

[spalger]

Thanks for the info @trentm, I'm still unclear on the rules around what can load what when it comes to ESM and commonjs, but that idea of using commonjs to instrument the important modules before they're loaded via ESM makes sense to me.

I can imagine that hack might work today and break when we upgrade these services without people really noticing, and possibly in a way that prevents us from using this hack...

Have we considered an alternate approach that would allow us to consume ESM-only packages from commonjs... what if we wrote a commonjs wrapper around ESM-only packages, which can async-load the package with import() statements in the commonjs exports... Not a great solution but seems slightly more reliable until APM can figure out how it's going to support ESM going forward.

One such package I'm thinking of is execa. If we need to get to v6 then we could wrap it with @kbn/execa and only load the actual execa module when you call one of the methods exported from @kbn/execa... Seems like it would work for that specific example but there are likely libraries which export things we need synchronously where this wouldn't work...

[trentm]

Have we considered an alternate approach ...

Currently the APM agent hooks into require('foo') and will only do its instrumentation if that 'foo' string is one of its instrumented module names. So any commonjs wrapper would have to take that over that module name. I'm not sure if that could easily be made to work. I might be misunderstanding your idea, however.

What about having a test file that ensures expected module instrumentations are working:

• the test file does the initApm setup the same as the kibana cli
• APM is configured to talk to a local mock APM server (e.g. https://github.com/elastic/apm-agent-nodejs/blob/main/test/_mock_apm_server.js)
• every module we expect to be instrumented is import ...d ESM-style and a basic use case of the module
• the test asserts that the mock APM server received some tracing data for that usage

This might also guard against other ways that APM instrumentation can stop: For example if Kibana updates to a newer version of Hapi or @elastic/elasticsearch that the current APM agent doesn't yet support.

[spalger]

Sorry, yeah, my idea was more about sticking with commonjs so that APM continues to work but allowing Kibana to consume ESM-only dependencies until APM is ready to instrument native ESM

[trentm]

Sorry, yeah, my idea was more about sticking with commonjs so that APM continues ...

Ah, I understand now. Yah, that works, but perhaps sucks for you having to write those wrappers and training Kibana devs to use the wrapped versions.

[spalger]

Yeah, we have the ESLint powers to ensure people use the wrappers, but it's certainly not ideal. Though it should be relatively limited to packages which both moved to ESM-only, and have unfixed security issues in the pre-ESM-only versions.

[watson]

@trentm It looks like the APM agent now has ESM support: elastic/apm-agent-nodejs#3381

Does that mean that we don't need to do any of the workarounds mentioned above for the APM agent to work if Kibana switches to use ESM directly?

Also, I'm not sure if this is relevant for ESM compatibility with the APM agent, but Kibana is currently running Node.js 18 but will soon be upgrading to Node.js 20 (most likely within the next few months)

[trentm]

@watson The APM agent's ESM support is documented here: https://www.elastic.co/guide/en/apm/agent/nodejs/current/esm.html
The ESM support has some limitations to consider (and perhaps prioritize some work for Kibana if it is seriously going to switch to running ESM):

• Currently Node v20 is not "supported" (as mentioned at https://www.elastic.co/guide/en/apm/agent/nodejs/current/esm.html#esm-compat-node). feat: enable ESM support for Node v20 apm-agent-nodejs#3469 is the working issue for this. The current blocker for this is getExports used for node v20, v18.19.0 doesn't handle reexports nodejs/import-in-the-middle#29 (for which there is a PR, but that PR has languished a bit). In some cases, an app won't hit these issues, so it might be fine for Kibana.
• The core Node.js functionality being used to support instrumenting ESM import is experimental and is changing quickly. For example, the are three recent issues at https://github.com/DataDog/import-in-the-middle/issues that might indicate problems Kibana could run into.
• The ESM support in the Node.js APM agent involves working through each of our instrumentations to add the support and sometimes re-work some of the instrumentation for ESM. Currently we have only worked through some of the instrumentations. https://www.elastic.co/guide/en/apm/agent/nodejs/current/esm.html#esm-compat-modules "hapi" is notably not yet supported. ESM support, stage 2 apm-agent-nodejs#3445 is the working issue for this.
• The current ESM support requires loading the ESM hook via NODE_OPTIONS or a command-line argument to node. That'll mean changing how elastic-apm-node is used in Kibana for startup. I don't know if that'll be difficult.
• Very recent work in Node.js v20.6.0 adds support for a new node --import ... flag that differs from the current node --require ... for CommonJS support and node --experimental-loader ... for the current ESM support. There hasn't been any work on the Node.js APM agent support/use this yet.
• The possible performance impact of the import-in-the-middle loader used by the APM agent to support shimming/patching ESM modules has not be investigated at all. I don't want to make up issues though. I don't have any indication that there is a noticeable perf impact, but the import-in-the-middle shimming is more expensive than require-in-the-middle.

Aside: What are the advantages of moving Kibana to using ESM?

[watson]

Thanks for the very thorough explanation of the current state!

What are the advantages of moving Kibana to using ESM?

Every day more and more 3rd party packages we depend on via npm switch to be pure ESM. We can't upgrade to those as long as we don't support ESM. This becomes especially critical once a bug or security issue is only fixed in the ESM-only version of the package.