dd-trace-js icon indicating copy to clipboard operation
dd-trace-js copied to clipboard

Update standard to remove eslint-plugin-node

Open Qard opened this issue 1 year ago • 5 comments

What does this PR do?

Update to latest version of eslint-config-standard so we can eliminate eslint-plugin-node.

Motivation

eslint-plugin-node triggers vuln scanner warnings so we should get it out of our dependency tree.

Security

Datadog employees:

  • [ ] If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • [x] This PR doesn't touch any of that.

Qard avatar Mar 06 '24 22:03 Qard

Overall package size

Self size: 6.24 MB Deduped: 61.19 MB No deduping: 61.94 MB

Dependency sizes

name version self size total size
@datadog/native-iast-taint-tracking 1.7.0 16.71 MB 16.72 MB
@datadog/native-appsec 7.1.0 14.37 MB 14.38 MB
@datadog/pprof 5.1.0 8.83 MB 9.68 MB
protobufjs 7.2.5 2.77 MB 6.56 MB
@datadog/native-iast-rewriter 2.3.0 2.15 MB 2.24 MB
@opentelemetry/core 1.14.0 872.87 kB 1.47 MB
@datadog/native-metrics 2.0.0 898.77 kB 1.3 MB
@opentelemetry/api 1.4.1 780.32 kB 780.32 kB
import-in-the-middle 1.7.3 67.62 kB 731.01 kB
pprof-format 2.0.7 588.12 kB 588.12 kB
msgpack-lite 0.1.26 201.16 kB 281.59 kB
opentracing 0.14.7 194.81 kB 194.81 kB
semver 7.5.4 93.4 kB 123.8 kB
@datadog/sketches-js 2.1.0 109.9 kB 109.9 kB
lodash.sortby 4.7.0 75.76 kB 75.76 kB
lru-cache 7.14.0 74.95 kB 74.95 kB
ipaddr.js 2.1.0 60.23 kB 60.23 kB
ignore 5.2.4 51.22 kB 51.22 kB
int64-buffer 0.1.10 49.18 kB 49.18 kB
shell-quote 1.8.1 44.96 kB 44.96 kB
istanbul-lib-coverage 3.2.0 29.34 kB 29.34 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
node-abort-controller 3.1.1 16.89 kB 16.89 kB
jest-docblock 29.7.0 8.99 kB 12.76 kB
crypto-randomuuid 1.0.0 11.18 kB 11.18 kB
path-to-regexp 0.1.7 6.78 kB 6.78 kB
koalas 1.0.2 6.47 kB 6.47 kB
methods 1.1.2 5.29 kB 5.29 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

github-actions[bot] avatar Mar 06 '24 22:03 github-actions[bot]

Codecov Report

Attention: Patch coverage is 50.00000% with 48 lines in your changes are missing coverage. Please review.

Project coverage is 85.06%. Comparing base (c9e1082) to head (448500a). Report is 1 commits behind head on master.

Files Patch % Lines
packages/dd-trace/src/plugins/index.js 11.62% 38 Missing :warning:
packages/dd-trace/src/telemetry/index.js 72.72% 3 Missing :warning:
packages/datadog-instrumentations/src/mocha.js 0.00% 1 Missing :warning:
.../appsec/iast/taint-tracking/taint-tracking-impl.js 0.00% 1 Missing :warning:
packages/dd-trace/src/plugins/plugin.js 0.00% 1 Missing :warning:
packages/dd-trace/src/plugins/tracing.js 0.00% 1 Missing :warning:
packages/dd-trace/src/plugins/util/web.js 75.00% 1 Missing :warning:
packages/dd-trace/src/serverless.js 0.00% 1 Missing :warning:
packages/dd-trace/src/span_processor.js 0.00% 1 Missing :warning:
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4146   +/-   ##
=======================================
  Coverage   85.06%   85.06%           
=======================================
  Files         247      247           
  Lines       10953    10953           
  Branches       33       33           
=======================================
  Hits         9317     9317           
  Misses       1636     1636           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 06 '24 22:03 codecov[bot]

Benchmarks

Benchmark execution time: 2024-03-20 18:25:39

Comparing candidate commit 448500acb9542e881797069db62878163b5c932e in PR branch update-standard with baseline commit c9e10822c125fb3c42cc0006dd1d8b01fb76fbe4 in branch master.

Found 1 performance improvements and 0 performance regressions! Performance is the same for 258 metrics, 7 unstable metrics.

scenario:plugin-graphql-with-depth-off-18

  • 🟩 max_rss_usage [-145.864MB; -82.720MB] or [-15.323%; -8.690%]

pr-commenter[bot] avatar Mar 06 '24 22:03 pr-commenter[bot]

Just a thought, but wouldn't it be a much smaller change to adjust the rules to be as they were before instead of changing 306 files? Not against the change per se, but it's very likely to cause conflicts with every open branch, and the goal of the PR is to fix a vulnerability and not to change our rules.

rochdev avatar Mar 07 '24 16:03 rochdev

The whole point of using standard is to not debate style issues and just adopt what it says, so if we go and carve out a bunch of exceptions that seems like it's opening up debate about which rules we should or should not follow. Personally I felt it was better to just make the changes to adapt, though I did go for just using a bunch of per-line rule exceptions in a bunch of places just to keep the delta relatively minimal.

If we would prefer to just use standard as a base and build our own ruleset on top of that then we should probably have a discussion here about which of the new rules should be kept and which should be changed. Thoughts?

Qard avatar Mar 07 '24 17:03 Qard

Apparently I completely missed that #4032 already existed. We could just land that if you prefer how it did things. 🤔

Qard avatar Mar 07 '24 18:03 Qard

We could just land that if you prefer how it did things.

I don't have a strong opinion. It just felt a bit much to change 300+ files to solve a vulnerability and I would generally prefer this type of change to be as focused as possible. But at the same time, as you said updating might mean additional rules which we would want in the long run anyway. Not blocking either way, it was just an observation.

rochdev avatar Mar 07 '24 21:03 rochdev