ESLint: Require await inside async functions by watson · Pull Request #5263 · DataDog/dd-trace-js

watson · 2025-02-13T12:56:04Z

What does this PR do?

Enable the require-await ESLint rule. The rule isn't enabled in tests, where the extra created promises doesn't harm anyone.

Motivation

If a function is marked as async, V8 will create a few extra promises behind the scenes when awaiting it. If the function doesn't itself contain any awaits, these extra promises just adds overhead that isn't needed.

Plugin Checklist

Additional Notes

Be aware: This might break some things that expect the function to return a promise. We'll have to go over each of the places where the PR removes the async keyword and check if the function returns something that should be a promise, but isn't.

There might be a few places, especially places were we use shimmer.wrap where we might actually want to keep the async keyword!

watson · 2025-02-13T12:56:21Z

ESLint: Require await inside async functions #5263 👈 (View in Graphite)
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

github-actions · 2025-02-13T12:56:56Z

Overall package size

Self size: 8.81 MB
Deduped: 95.01 MB
No deduping: 95.53 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.4.0 | 29.44 MB | 29.44 MB | | @datadog/native-appsec | 8.4.0 | 19.25 MB | 19.26 MB | | @datadog/native-iast-taint-tracking | 3.3.0 | 13.77 MB | 13.78 MB | | @datadog/pprof | 5.5.1 | 9.79 MB | 10.17 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.8.0 | 2.6 MB | 2.74 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 3.1.0 | 1.06 MB | 1.46 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 835.4 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.2 | 53.63 kB | 53.63 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

codecov · 2025-02-13T12:57:39Z

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.44%. Comparing base (8cd547e) to head (5dcfccf).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
...ackages/datadog-instrumentations/src/mocha/main.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5263      +/-   ##
==========================================
+ Coverage   80.42%   80.44%   +0.01%     
==========================================
  Files         491      492       +1     
  Lines       21847    21879      +32     
==========================================
+ Hits        17570    17600      +30     
- Misses       4277     4279       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

datadog-datadog-prod-us1 · 2025-02-13T13:02:15Z

Datadog Report

Branch report: watson/eslint-require-await
Commit report: b05ca98
Test service: dd-trace-js-integration-tests

✅ 0 Failed, 669 Passed, 0 Skipped, 14m 11.49s Total Time

pr-commenter · 2025-02-13T13:07:26Z

Benchmarks

Benchmark execution time: 2025-02-26 09:58:06

Comparing candidate commit 5dcfccf in PR branch watson/eslint-require-await with baseline commit 8cd547e in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 918 metrics, 15 unstable metrics.

rochdev · 2025-02-15T00:22:49Z

Actually, can we instead have a rule to block async/await entirely? Promises too. We've had a non-written rule about never using promises in the codebase since day 1 because it can slow down async_hooks significantly.

watson · 2025-02-15T07:26:22Z

@rochdev I wouldn't mind doing that, but I feel that's a different PR

bengl

LGTM except that one request change, and I'd like to make sure @juan-fernandez reviews the test-framework instrumentation code.

BridgeAR

I used the PR to familiarize myself with a bit more of the code and I looked very deeply into each usage. All of these changes are LGTM

juan-fernandez

test optimization's perspective here.

I'm not against the change but I don't find it particularly useful: the asyncs need to be replaced by a comment or it'll make it harder to know that an async operation is possible in the wrapper. This makes legibility worse IMO.

As for the performance improvements: I understand the improvement, but is it really applying? These wrapper functions are async because the original ones are async. Example: https://github.com/jestjs/jest/blob/93ca9d3cf6db4b88393ab02b8d0007bcdf30e4bb/packages/jest-reporters/src/CoverageReporter.ts#L107

is wrapping an async function into a sync one what we want here?

This said, again, I don't oppose the change 😄

tlhunter · 2025-02-19T19:42:37Z

Actually, can we instead have a rule to block async/await entirely? Promises too. We've had a non-written rule about never using promises in the codebase since day 1 because it can slow down async_hooks significantly.

async/await is totally fine in tests

rochdev · 2025-02-19T23:50:09Z

async/await is totally fine in tests

Absolutely, by codebase I meant the source code, not the tests, and even that has exceptions today (for example I believe at least profiling has been using promises for a while) but the main reason it was blocked in most places is that historically it would cause issues with async_hooks. Since we instrument promises, creating promises means we're paying double the cost (actually more because of GC). As pointed out though, it's technically out of scope of the PR in the sense that I was asking for something else, but I think it would make sense to make sure through linting that we're not using promises at all.

If a function is marked as async, V8 will create a few extra promises behind the scenes when awaiting it. If the function doesn't itself contain any awaits, these extra promises just adds overhead that isn't needed. The `require-await` rule isn't enabled in tests, where the extra created promises doesn't harm anyone.

This reverts commit b173d82.

If a function is marked as async, V8 will create a few extra promises behind the scenes when awaiting it. If the function doesn't itself contain any awaits, these extra promises just adds overhead that isn't needed. The `require-await` rule isn't enabled in tests, where the extra created promises doesn't harm anyone.

watson requested review from a team as code owners February 13, 2025 12:56

watson requested a review from tonyredondo February 13, 2025 12:56

watson self-assigned this Feb 13, 2025

watson added the semver-patch label Feb 13, 2025

IlyasShabi previously approved these changes Feb 13, 2025

View reviewed changes

watson requested a review from bengl February 18, 2025 14:43

watson dismissed IlyasShabi’s stale review via d78565a February 18, 2025 14:45

watson requested a review from IlyasShabi February 18, 2025 14:47

bengl requested changes Feb 18, 2025

View reviewed changes

Comment thread benchmark/sirun/get-results.js Outdated

watson force-pushed the watson/eslint-require-await branch from d78565a to 25835a5 Compare February 18, 2025 15:01

BridgeAR previously approved these changes Feb 18, 2025

View reviewed changes

watson dismissed BridgeAR’s stale review via 8a0931a February 19, 2025 06:22

watson force-pushed the watson/eslint-require-await branch from 25835a5 to 8a0931a Compare February 19, 2025 06:22

watson requested a review from juan-fernandez February 19, 2025 06:23

BridgeAR reviewed Feb 19, 2025

View reviewed changes

Comment thread packages/datadog-instrumentations/src/apollo-server-core.js Outdated

juan-fernandez reviewed Feb 19, 2025

View reviewed changes

watson added 2 commits February 26, 2025 10:11

Add comments

b173d82

Revert "Add comments"

5dcfccf

This reverts commit b173d82.

watson force-pushed the watson/eslint-require-await branch from 8a0931a to 5dcfccf Compare February 26, 2025 09:43

BridgeAR approved these changes Feb 26, 2025

View reviewed changes

BridgeAR requested a review from bengl February 26, 2025 09:46

bengl approved these changes Feb 26, 2025

View reviewed changes

watson merged commit 9bc5d34 into master Feb 27, 2025

watson deleted the watson/eslint-require-await branch February 27, 2025 04:50

watson mentioned this pull request Feb 27, 2025

v5.39.0 proposal #5337

Merged

Uh oh!

Conversation

watson commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Motivation

Plugin Checklist

Additional Notes

Uh oh!

watson commented Feb 13, 2025

Uh oh!

github-actions Bot commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overall package size

Uh oh!

codecov Bot commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

datadog-datadog-prod-us1 Bot commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Datadog Report

Uh oh!

pr-commenter Bot commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Uh oh!

rochdev commented Feb 15, 2025

Uh oh!

watson commented Feb 15, 2025

Uh oh!

bengl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

juan-fernandez left a comment

Choose a reason for hiding this comment

Uh oh!

tlhunter commented Feb 19, 2025

Uh oh!

rochdev commented Feb 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

watson commented Feb 13, 2025 •

edited

Loading

github-actions Bot commented Feb 13, 2025 •

edited

Loading

codecov Bot commented Feb 13, 2025 •

edited

Loading

datadog-datadog-prod-us1 Bot commented Feb 13, 2025 •

edited

Loading

pr-commenter Bot commented Feb 13, 2025 •

edited

Loading