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

mock elastic-apm-node in @kbn/test jest preset #120324

Merged
merged 7 commits into from
Dec 7, 2021

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Dec 3, 2021

Summary

Fix #117255

In #117255 we investigated a memory leak in jest environment, that lead to elastic-apm-node patching native modules at runtime (please refer to the issue for more details).

This PR fixes it by mocking elastic-apm-node in our @kbn/test preset

Thanks to @FrankHassanabad and @academo for providing the scripts helping identifying the leak and starting the investigation.

How to test

You need to run tests that are identified as leaking using --runInBand, and confirm that no memory leaks with this PR's branch.

I created a x-pack/plugins/alerting/server/sample_test.ts file with the following test:

import { plugin } from '../../actions/server';

describe('identifying the leak', () => {
  test('is hard', () => {
    Object.keys(plugin);
  });
});

and used the following script (credits to @FrankHassanabad):

#!/bin/sh

# Set your kibana home here
KIBANA_HOME=/path/to/elastic/kibana
# Set your kibana project here
KIBANA_PROJECT=x-pack/plugins/alerting/jest.config.js
# When it does leak set this to around 2 gigs to get memory snapshots without issues.
MAX_MEMORY=500
# Set this to true to activate "--inspect-brk" below
CHROME_INSPECT=false

# Useage if no arguments
if [ $# -eq 0 ]; then
  echo "Usage is ./copy_tests.sh <test name>"
  exit 1
fi

# Check if file exists or not
if [[ ! -f $1 ]]; then
  echo "File does not exist"
  exit 1
fi

# Gets the directory of your first argument
DIR="$(dirname "${1}")"

# Remove all the copied scripts even if cntrl-C is pressed or other issues.
trap "rm $DIR/script_copy_test_*.test.ts" EXIT

# Loops max times and copy the single test in place
for i in {1..100}
do
  cp $1 $DIR/script_copy_test_$i.test.ts
done

pushd $KIBANA_HOME;
if [ "$CHROME_INSPECT" = true ] ;
then
  node --max-old-space-size=$MAX_MEMORY --inspect-brk --expose-gc ./node_modules/.bin/jest --verbose --runInBand --logHeapUsage --detectOpenHandles --no-cache --config $KIBANA_PROJECT $DIR/script_copy_test_*.ts;
else
  node --max-old-space-size=$MAX_MEMORY --expose-gc ./node_modules/.bin/jest --verbose --runInBand --logHeapUsage --detectOpenHandles --no-cache --config $KIBANA_PROJECT $DIR/script_copy_test_*.ts;
fi
popd;

Using with

./memory_leak.sh x-pack/plugins/alerting/server/sample_test.ts

@pgayvallet pgayvallet added release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team v8.1.0 Feature:Unit Testing labels Dec 3, 2021
@pgayvallet pgayvallet marked this pull request as ready for review December 3, 2021 10:29
@pgayvallet pgayvallet requested review from vigneshshanmugam and a team as code owners December 3, 2021 10:29
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@elasticmachine
Copy link
Contributor

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

Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -0,0 +1,62 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Should we migrate this file to TS? I guess we don't want to, so we don't need to import the original library to match the types (and add the dependency). Is that right?

Copy link
Contributor Author

@pgayvallet pgayvallet Dec 3, 2021

Choose a reason for hiding this comment

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

I started with TS (which ensured I have a full mock as I used a jest.Mock<Agent> as the type of this structure when coding it), but:

  1. You can't use module.export =, and not 100% sure export default is exactly the same (plus I had to disable an eslint warning as default exports are not allowed here)
  2. I was forced to disable some other eslint warnings because jest is undeclared (file is not considered as a test file)
  3. I had an error while running kbn bootstrap because this module can't import type { Agent } from "elastic-apm-node" for some reason (probably related to bazel project dependencies)

so given that

  1. all other mocks in this folder are still in javascript

I assumed this was okay-ish

But I can retry using TS if we think this is preferable, @elastic/kibana-operations wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

I think your points are very valid! I'm just happy that we have them documented now in this PR discussion 🙂

Copy link
Contributor

@mshustov mshustov Dec 6, 2021

Choose a reason for hiding this comment

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

It might be okay to start with the current implementation, but in the long term, we need TypeScript to enforce the mock compatibility with an interface of the original package.

You can't use module.export =, and not 100% sure export default is exactly the same (plus I had to disable an eslint warning as default exports are not allowed here)

Maybe the Ops team can help with it?

I had an error while running kbn bootstrap because this module can't import type { Agent } from "elastic-apm-node" for some reason (probably related to bazel project dependencies)

elastic-apm-node is not declared as a TYPES_DEPS dependency in
https://github.com/elastic/kibana/blob/e527c3fe297d3e17c6d5a8a24b4ea58a676fc641/packages/kbn-test/BUILD.bazel

I was forced to disable some other eslint warnings because jest is undeclared (file is not considered as a test file)

jest types are declared for all *.ts files

"types": ["jest", "node"]
It should work out of the box 🤔

all other mocks in this folder are still in javascript

They are quite small and don't mock such a large API surface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jest types are declared for all *.ts files

My bad, it's actually only with .js extension. It worked fine with .ts files.

elastic-apm-node is not declared as a TYPES_DEPS dependency in

Thanks, that was just it.

Done in df6e05d

@pgayvallet
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

Checked out this branch and then commented all of the security_solution mocks:

  // moduleNameMapper: {
  // 'core/server$': '<rootDir>/x-pack/plugins/security_solution/server/__mocks__/core.mock.ts',
  // 'task_manager/server$':
  //   '<rootDir>/x-pack/plugins/security_solution/server/__mocks__/task_manager.mock.ts',
  // 'alerting/server$': '<rootDir>/x-pack/plugins/security_solution/server/__mocks__/alert.mock.ts',
  // 'actions/server$': '<rootDir>/x-pack/plugins/security_solution/server/__mocks__/action.mock.ts',
  // },

In the files:

modified:   x-pack/plugins/security_solution/server/client/jest.config.js
modified:   x-pack/plugins/security_solution/server/endpoint/jest.config.js
modified:   x-pack/plugins/security_solution/server/fleet_integration/jest.config.js
modified:   x-pack/plugins/security_solution/server/jest.config.js
modified:   x-pack/plugins/security_solution/server/lib/jest.config.js
modified:   x-pack/plugins/security_solution/server/search_strategy/jest.config.js
modified:   x-pack/plugins/security_solution/server/usage/jest.config.js
modified:   x-pack/plugins/security_solution/server/utils/jest.config.js

And I was able to run under 1 gig like so:

node --max-old-space-size=1000 ./node_modules/.bin/jest --runInBand --logHeapUsage --no-cache --config x-pack/plugins/security_solution/jest.config.dev.js x-pack/plugins/security_solution/server

server side. So I think this fixes the worst if there are any others.

The heap snapshots at the end of the test run was 756 MB:

 PASS  x-pack/plugins/security_solution/server/endpoint/routes/resolver/tree/utils/index.test.ts (756 MB heap size)

so I probably could have run the tests with less memory but I choose just 1 gig as that's what we have been doing previously.

@pgayvallet
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@mistic mistic left a comment

Choose a reason for hiding this comment

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

LGTM

@pgayvallet
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@pgayvallet pgayvallet merged commit f89887d into elastic:main Dec 7, 2021
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Dec 7, 2021
* mock `elastic-apm-node` in `@kbn/test` jest preset

* adapt kbn-apm-config-loader tests

* use TS for agent mock

Co-authored-by: Kibana Machine <[email protected]>
pgayvallet added a commit that referenced this pull request Dec 7, 2021
* mock `elastic-apm-node` in `@kbn/test` jest preset

* adapt kbn-apm-config-loader tests

* use TS for agent mock

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 7, 2021
…-chromium-before-compiling-pdf

* 'main' of github.com:elastic/kibana: (121 commits)
  FTR should use the new kibana_system user (elastic#120436)
  [Lens] Temporarily exclude Mosaic/Waffle from the suggestions list (elastic#120488)
  [Reporting] Fix slow CSV with large max size bytes (elastic#120365)
  [CTI] Threat Intel Card on Overview page needs to accommodate Fleet TI integrations -  (elastic#120459)
  [Dashboard] Added KibanaThemeProvider.  (elastic#120122)
  add more rule_registry unit tests (elastic#120323)
  [Lens] update xpack.lens.pie.smallValuesWarningMessage text (elastic#120478)
  [Fleet] Simplified package policy create API, enriching default values (elastic#119739)
  mock `elastic-apm-node` in `@kbn/test` jest preset (elastic#120324)
  [RAC] Rename occurrences of alert_type to rule_type in Infra (elastic#120455)
  [Security Solutions] Removes tech debt of exporting all from linter rule for timeline plugin (elastic#120437)
  [Workplace Search] Add API Keys view to replace Access tokens (elastic#120147)
  [Security Solutions] Removes tech debt of exporting all from linter rule for cases plugin in the server section (elastic#120411)
  Add support for threat.feed.name (elastic#120250)
  [Rule Registry] Rewrite APM registry rules for Observability (elastic#117740)
  [docs] Fix artifact layout formatting (elastic#119386)
  [Workplace Search] Add a technical preview of connecting GitHub via GitHub apps (elastic#119764)
  add osquery notes for 7.16 (elastic#120407)
  chore(NA): splits types from code on @kbn/docs-utils (elastic#120533)
  [Reporting] Decouple screenshotting plugin from the reporting (elastic#120110)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.test.ts
#	x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts
#	x-pack/plugins/reporting/server/lib/screenshots/observable.ts
#	x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts
TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
* mock `elastic-apm-node` in `@kbn/test` jest preset

* adapt kbn-apm-config-loader tests

* use TS for agent mock

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Unit Testing release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[securitySolution] Jest memory leaks
10 participants