add support to api security sampling #4755

IlyasShabi · 2024-10-03T07:48:12Z

What does this PR do?

Implements a new API security sampling algorithm using an LRU cache with a 30s TTL
Delays schema extraction until the end of the request processing

Motivation

The current API security sampling method randomly selects 10% of requests. This PR introduces new algorithm based on request priority, utilizing an LRU cache with TTL for each { url, statusCode, method }

Delaying the schema extraction to the end of the request ensures that we have an accurate priority value

Plugin Checklist

github-actions · 2024-10-03T07:48:51Z

Overall package size

Self size: 7.86 MB
Deduped: 64.9 MB
No deduping: 65.24 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.2.1 | 19.18 MB | 19.19 MB | | @datadog/native-iast-taint-tracking | 3.2.0 | 13.9 MB | 13.91 MB | | @datadog/pprof | 5.4.1 | 9.76 MB | 10.13 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.5.0 | 2.51 MB | 2.65 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 3.0.1 | 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 | 826.22 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 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 | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 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 | | 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 | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 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}

pr-commenter · 2024-10-03T12:21:16Z

Benchmarks

Benchmark execution time: 2024-10-31 14:48:36

Comparing candidate commit dc18f3f in PR branch api-security-sampling with baseline commit 91c4371 in branch master.

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

index.d.ts

packages/dd-trace/src/appsec/api_security_sampler_cache.js

packages/dd-trace/src/appsec/index.js

packages/dd-trace/src/appsec/api_security_sampler.js

docs/test.ts

packages/dd-trace/src/appsec/api_security_sampler.js

integration-tests/standalone-asm.spec.js

uurien · 2024-10-11T13:15:42Z

packages/dd-trace/src/config.js

@@ -674,7 +674,7 @@ class Config {
      DD_API_SECURITY_ENABLED && isTrue(DD_API_SECURITY_ENABLED),
      DD_EXPERIMENTAL_API_SECURITY_ENABLED && isTrue(DD_EXPERIMENTAL_API_SECURITY_ENABLED)
    ))
-    this._setUnit(env, 'appsec.apiSecurity.requestSampling', DD_API_SECURITY_REQUEST_SAMPLE_RATE)
+    this._setValue(env, 'appsec.apiSecurity.sampleDelay', maybeFloat(DD_API_SECURITY_SAMPLE_DELAY))


why not _setUnit as previous requestSampling?

The previous requestSampling was a rate which ranged between 0 and 1.

this._setValue(obj, name, Math.min(Math.max(value, 0), 1))

sampleDelay represents a value in seconds

packages/dd-trace/src/appsec/api_security_sampler.js

simon-id

haven't reviewed it all yet but that should keep you busy:

packages/dd-trace/src/appsec/remote_config/capabilities.js

index.d.ts

packages/dd-trace/src/appsec/api_security_sampler.js

simon-id · 2024-10-12T10:14:15Z

packages/dd-trace/src/appsec/api_security_sampler.js

+}
+
+function computeKey (req, res) {
+  const route = web.getContext(req).paths.join('') || ''


the interesting thing about getting the paths from web.js is that the result will depend on when you call it.
If this sampling is called in a router, it will be the route up to that router, but if it's called at the end of the request it will be the last router I think.
Is that a good or bad thing ?

integration-tests/standalone-asm.spec.js

packages/dd-trace/src/appsec/api_security_sampler.js

simon-id · 2024-10-15T15:12:06Z

packages/dd-trace/src/config.js

@@ -842,7 +842,6 @@ class Config {
    tagger.add(tags, options.tags)

    this._setBoolean(opts, 'appsec.apiSecurity.enabled', options.appsec.apiSecurity?.enabled)
-    this._setUnit(opts, 'appsec.apiSecurity.requestSampling', options.appsec.apiSecurity?.requestSampling)


no sampling replacement for programmatic options ?

According to the RFC there is no programmatic option for this, just an env var

Yeah the RFC rarely mention those as they're language dependant, it's really up to us to decide if we want to make them available or not. I'd say why not ?

AFAIK backend set the 30s window for better accuracy. It’s mainly for backend use, not user configuration.
the env var is just for testing, and changing it could be risky

packages/dd-trace/src/appsec/api_security_sampler.js

IlyasShabi force-pushed the api-security-sampling branch from 90fe2c8 to 6987a66 Compare October 3, 2024 12:12

IlyasShabi added appsec semver-minor labels Oct 3, 2024

IlyasShabi self-assigned this Oct 3, 2024

IlyasShabi force-pushed the api-security-sampling branch from e5a079e to ca8bb68 Compare October 3, 2024 15:09

IlyasShabi marked this pull request as ready for review October 3, 2024 15:17

IlyasShabi requested review from a team as code owners October 3, 2024 15:17

IlyasShabi commented Oct 3, 2024

View reviewed changes

index.d.ts Show resolved Hide resolved

CarlesDD reviewed Oct 4, 2024

View reviewed changes

packages/dd-trace/src/appsec/api_security_sampler_cache.js Outdated Show resolved Hide resolved

uurien reviewed Oct 4, 2024

View reviewed changes

packages/dd-trace/src/appsec/index.js Show resolved Hide resolved

packages/dd-trace/src/appsec/api_security_sampler.js Outdated Show resolved Hide resolved

uurien requested a review from iunanua October 4, 2024 10:43

IlyasShabi force-pushed the api-security-sampling branch 2 times, most recently from f87c0d0 to 7d0ace0 Compare October 7, 2024 12:22

IlyasShabi requested a review from a team as a code owner October 8, 2024 07:59

simon-id reviewed Oct 8, 2024

View reviewed changes

docs/test.ts Show resolved Hide resolved

iunanua reviewed Oct 8, 2024

View reviewed changes

packages/dd-trace/src/appsec/api_security_sampler.js Outdated Show resolved Hide resolved

IlyasShabi force-pushed the api-security-sampling branch 6 times, most recently from a84bfce to f5c3bd3 Compare October 10, 2024 17:43

iunanua reviewed Oct 11, 2024

View reviewed changes

integration-tests/standalone-asm.spec.js Outdated Show resolved Hide resolved

uurien reviewed Oct 11, 2024

View reviewed changes

IlyasShabi force-pushed the api-security-sampling branch from f5c3bd3 to 3ecd200 Compare October 11, 2024 14:58

simon-id reviewed Oct 12, 2024

View reviewed changes

iunanua reviewed Oct 14, 2024

View reviewed changes

integration-tests/standalone-asm.spec.js Outdated Show resolved Hide resolved

iunanua reviewed Oct 14, 2024

View reviewed changes

packages/dd-trace/src/appsec/api_security_sampler.js Outdated Show resolved Hide resolved

iunanua reviewed Oct 14, 2024

View reviewed changes

packages/dd-trace/src/appsec/api_security_sampler.js Outdated Show resolved Hide resolved

IlyasShabi force-pushed the api-security-sampling branch from 3ecd200 to a6b3cf7 Compare October 14, 2024 12:58

simon-id reviewed Oct 15, 2024

View reviewed changes

IlyasShabi added 6 commits October 16, 2024 16:34

add support to api security sampling

97c50a1

fix express plugin schema extraction

c7e2de5

use priority simpler to get span priority

a545050

use lru cache package

178246d

use route path instead of url

9b448b9

use route.path or url to generate the key

3edee48

IlyasShabi force-pushed the api-security-sampling branch 2 times, most recently from bb9deba to 3d07e45 Compare October 16, 2024 14:58

use ttlcache

499a914

IlyasShabi force-pushed the api-security-sampling branch from 3d07e45 to 499a914 Compare October 16, 2024 15:21

Merge branch 'master' into api-security-sampling

b6b7c3d

simon-id reviewed Oct 25, 2024

View reviewed changes

packages/dd-trace/src/appsec/api_security_sampler.js Outdated Show resolved Hide resolved

iunanua and others added 4 commits October 30, 2024 14:59

Merge branch 'master' into api-security-sampling

8652ca6

Fix standalone integration test

fe4a800

Increase test timeout

08f63c8

simplify force sample

dc18f3f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support to api security sampling #4755

add support to api security sampling #4755

IlyasShabi commented Oct 3, 2024

github-actions bot commented Oct 3, 2024 •

edited

Loading

pr-commenter bot commented Oct 3, 2024 •

edited

Loading

uurien Oct 11, 2024

IlyasShabi Oct 14, 2024 •

edited

Loading

simon-id left a comment

simon-id Oct 12, 2024

simon-id Oct 15, 2024

IlyasShabi Oct 17, 2024

simon-id Oct 17, 2024

IlyasShabi Oct 31, 2024

add support to api security sampling #4755

Are you sure you want to change the base?

add support to api security sampling #4755

Conversation

IlyasShabi commented Oct 3, 2024

What does this PR do?

Motivation

Plugin Checklist

github-actions bot commented Oct 3, 2024 • edited Loading

Overall package size

pr-commenter bot commented Oct 3, 2024 • edited Loading

Benchmarks

uurien Oct 11, 2024

Choose a reason for hiding this comment

IlyasShabi Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

simon-id left a comment

Choose a reason for hiding this comment

simon-id Oct 12, 2024

Choose a reason for hiding this comment

simon-id Oct 15, 2024

Choose a reason for hiding this comment

IlyasShabi Oct 17, 2024

Choose a reason for hiding this comment

simon-id Oct 17, 2024

Choose a reason for hiding this comment

IlyasShabi Oct 31, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 3, 2024 •

edited

Loading

pr-commenter bot commented Oct 3, 2024 •

edited

Loading

IlyasShabi Oct 14, 2024 •

edited

Loading