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

[APM] Optimize synthtrace #116091

Merged
merged 5 commits into from
Oct 26, 2021
Merged

Conversation

dgieselaar
Copy link
Member

@dgieselaar dgieselaar commented Oct 25, 2021

Improve performance (10x) of synthtrace by using worker threads and sequential event ids.

TBD:

  • Update docs

Improve performance of synthtrace by using worker threads and sequential event ids.
@dgieselaar dgieselaar added Team:APM All issues that need APM UI Team support release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Oct 25, 2021
@dgieselaar dgieselaar marked this pull request as ready for review October 25, 2021 12:35
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

Comment on lines 106 to 108
const limiter = pLimit(workers);

return Promise.all(new Array(numBatches).fill(undefined).map((_) => limiter(processNextBatch)));
Copy link
Member

@sorenlouv sorenlouv Oct 25, 2021

Choose a reason for hiding this comment

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

Can we speed this up even more by making it multi-threaded using the cluster module or pm2?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I now see you are using worker_threads which will add the multi-core support I was alluding to above.

return;
}
uploadNextBatch();
const worker = new Worker(Path.join(__dirname, './upload_next_batch.js'), {
Copy link
Member

@sorenlouv sorenlouv Oct 25, 2021

Choose a reason for hiding this comment

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

Unfortunate that we have to refer to javascript files. Is something like this possible?

const { Worker, isMainThread } = require("worker_threads");

if (isMainThread) {
  // start new worker
  const worker = new Worker(__filename)
} else {
  // run as worker
}

Copy link
Member Author

Choose a reason for hiding this comment

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

we'd have to write everything in JavaScript (supported by node v16) and not use any compiler, I think.

Copy link
Member

Choose a reason for hiding this comment

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

Why would we have to write everything in javascript? The code is executed at runtime where __filename would resolve to the javascript file (even if the source is typescript).

Copy link
Member Author

Choose a reason for hiding this comment

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

you'd have to build it first though?

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

Nice improvement!

Comment on lines 54 to 56
const document = {};

Object.assign(document, esDocumentDefaults);
Copy link
Member

Choose a reason for hiding this comment

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

nit: what about:

Suggested change
const document = {};
Object.assign(document, esDocumentDefaults);
const document = {
...esDocumentDefaults
};

Copy link
Member Author

Choose a reason for hiding this comment

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

performance, trying to squeeze as much perf out of this function as possible, as it's called very often.

Comment on lines 47 to 49
Object.assign(values, event, {
'@timestamp': new Date(event['@timestamp']!).toISOString(),
'timestamp.us': event['@timestamp']! * 1000,
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
Object.assign(values, event, {
'@timestamp': new Date(event['@timestamp']!).toISOString(),
'timestamp.us': event['@timestamp']! * 1000,
const values = {
...event,
'@timestamp': new Date(event['@timestamp']!).toISOString(),
'timestamp.us': event['@timestamp']! * 1000,

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@dgieselaar dgieselaar merged commit 9d9eeb0 into elastic:master Oct 26, 2021
@dgieselaar dgieselaar deleted the optimize-synthtrace branch October 26, 2021 10:42
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.16 Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 116091

jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 26, 2021
…-migrate-away-from-injected-css-js

* 'master' of github.com:elastic/kibana: (347 commits)
  [Upgrade Assistant] Disable UI by default in 8.0 (elastic#115782)
  [Uptime] Added permission for new tls alert (elastic#116107)
  [APM] Optimize synthtrace (elastic#116091)
  Fix ux/apm inspector panel (elastic#116188)
  [RAC]: add experimental badge to alerts (elastic#116090)
  Unskip jest handled promise rejections (elastic#116021)
  [Lens] Improve tick placement for binary formatter (elastic#116158)
  chore: rename getApmHref to getLegacyApmHref (elastic#115689)
  [Security Solution] Validate ipv4/CIDR with format x.x.x.x/xx (elastic#116127)
  [Fleet] Use data stream name in query to get data stream info (elastic#115805)
  [Uptime] TLS and TLS legacy alert translation mismatch (elastic#116113)
  New field for integrations field (elastic#116175)
  Set required to false until the input is not visited (elastic#116099)
  Enable interactive setup by default (elastic#116141)
  Add not ready response to interactive setup (elastic#116138)
  Hide or button if needed (elastic#116124)
  [ML] Adding datafeed api tests (elastic#116133)
  Add page title to index advanced page (elastic#116134)
  chore: rename functions in aggregated_transactions helper  (elastic#116001)
  Fix bug where number rendered as date (elastic#116224)
  ...

# Conflicts:
#	x-pack/plugins/reporting/server/lib/screenshots/observable.ts
#	x-pack/plugins/reporting/server/lib/screenshots/open_url.ts
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 28, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 116091 or prevent reminders by adding the backport:skip label.

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 116091 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v7.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants