Skip to content

PROF-10329: Separate SSI telemetry and heuristics activation#4592

Merged
szegedi merged 4 commits intomasterfrom
szegedi/four-state-profiling
Aug 14, 2024
Merged

PROF-10329: Separate SSI telemetry and heuristics activation#4592
szegedi merged 4 commits intomasterfrom
szegedi/four-state-profiling

Conversation

@szegedi
Copy link
Copy Markdown
Contributor

@szegedi szegedi commented Aug 9, 2024

What does this PR do?

Currently DD_PROFILING_ENABLED=auto will trigger SSI telemetry emission, just as if DD_INJECTION_ENABLED=profiler was specified. However, the two are not identical in requirements for emitting telemetry.

  • Telemetry should be emitted if and only if DD_INJECTION_ENABLED is defined with any value, and DD_PROFILING_ENABLED=false is not explicitly specified.
  • Heuristics should activate if and only if DD_INJECTION_ENABLED=profiler or DD_PROFILING_ENABLED=auto are specified.

We need to separate the two.

  • This PR starts off by simplifying the env var configuration around SSI. Instead of _applyEnvironment() deriving values for profiling.ssi and profiling.heuristicsEnabled, it now just mirrors the values for DD_PROFILING_ENABLED and DD_INJECTION_ENABLED. To this end, profiling.enabled is no longer a boolean, as it now needs to support four values (true, false, auto, undefined) instead of three ones (true, false, undefined.) For less confusion around boolean-ey value names, we now use a string with values "enabled", "disabled", and "auto" (while keeping undefined.)
  • We completely removed profiling.ssi and profiling.heuristicsEnabled in favor of the now 4-state profiling.enabled and the newly introduced injectionEnabled that is an array of parsed values from DD_INJECTION_ENABLED.
  • We touched up telemetry.logCollection as it, too, used to be derived in _applyEnvironment(). Now it got moved to its rightful place in _applyCalculated().
  • Finally, we use these new configuration values to revamp ssi-heuristics.js and its uses. We managed to eliminate the frankly unnecessary profiling enabled config too – if profiling is not enabled, we short circuit the path to creating any profiling related objects now in proxy.js.

Motivation

SSI telemetry and heuristics are actually orthogonal concerns. We also need the separation to later cleanly implement the emission of SSI information in profiles' system info.

@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented Aug 9, 2024

Benchmarks

Benchmark execution time: 2024-08-12 13:22:16

Comparing candidate commit e8685d9 in PR branch szegedi/four-state-profiling with baseline commit 77c1d07 in branch master.

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

@szegedi szegedi force-pushed the szegedi/four-state-profiling branch from afd1ae9 to df6b40a Compare August 11, 2024 07:49
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 11, 2024

Overall package size

Self size: 6.95 MB
Deduped: 58.17 MB
No deduping: 58.45 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.0.1 | 15.59 MB | 15.6 MB | | @datadog/native-iast-taint-tracking | 3.1.0 | 12.27 MB | 12.28 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.4.1 | 2.14 MB | 2.23 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.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.8.1 | 71.67 kB | 785.15 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 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 | | lru-cache | 7.14.0 | 74.95 kB | 74.95 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 | | 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 | | path-to-regexp | 0.1.7 | 6.78 kB | 6.78 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
Copy link
Copy Markdown

codecov Bot commented Aug 11, 2024

Codecov Report

Attention: Patch coverage is 56.92308% with 28 lines in your changes missing coverage. Please review.

Project coverage is 82.83%. Comparing base (77c1d07) to head (df6b40a).

Files Patch % Lines
packages/dd-trace/src/profiling/ssi-heuristics.js 30.00% 21 Missing ⚠️
packages/dd-trace/src/proxy.js 66.66% 5 Missing ⚠️
packages/dd-trace/src/config.js 94.73% 1 Missing ⚠️
packages/dd-trace/src/profiler.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4592       +/-   ##
===========================================
+ Coverage   68.33%   82.83%   +14.49%     
===========================================
  Files         254      256        +2     
  Lines       11195    11260       +65     
  Branches       33       33               
===========================================
+ Hits         7650     9327     +1677     
+ Misses       3545     1933     -1612     

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

@szegedi szegedi force-pushed the szegedi/four-state-profiling branch from df6b40a to 1460941 Compare August 12, 2024 12:34
Adds injectionEnabled config.
profiling.enabled is four-state now (enabled, disabled, auto, undefined)
Telemetry emission and heuristics are now independent.
@szegedi szegedi force-pushed the szegedi/four-state-profiling branch from 1460941 to cf688e6 Compare August 12, 2024 12:52
@szegedi szegedi marked this pull request as ready for review August 12, 2024 13:26
@szegedi szegedi requested a review from a team as a code owner August 12, 2024 13:26
Comment thread packages/dd-trace/src/config.js
@szegedi szegedi merged commit 4df8c4d into master Aug 14, 2024
@szegedi szegedi deleted the szegedi/four-state-profiling branch August 14, 2024 07:30
@szegedi szegedi changed the title PROF-10320: Separate SSI telemetry and heuristics activation PROF-10329: Separate SSI telemetry and heuristics activation Aug 14, 2024
bengl pushed a commit that referenced this pull request Aug 29, 2024
* Telemetry emission and heuristics are now independent.
* profiling.enabled is four-state now (true, false, auto, undefined)
* Adds injectionEnabled config
* Removes profiling.{ssi, heuristicsEnabled} config.
* Move derived telemetry.logCollection value computation to _applyCalculated()
* Tidy _merge()
bengl pushed a commit that referenced this pull request Aug 29, 2024
* Telemetry emission and heuristics are now independent.
* profiling.enabled is four-state now (true, false, auto, undefined)
* Adds injectionEnabled config
* Removes profiling.{ssi, heuristicsEnabled} config.
* Move derived telemetry.logCollection value computation to _applyCalculated()
* Tidy _merge()
bengl pushed a commit that referenced this pull request Aug 30, 2024
* Telemetry emission and heuristics are now independent.
* profiling.enabled is four-state now (true, false, auto, undefined)
* Adds injectionEnabled config
* Removes profiling.{ssi, heuristicsEnabled} config.
* Move derived telemetry.logCollection value computation to _applyCalculated()
* Tidy _merge()
bengl pushed a commit that referenced this pull request Aug 30, 2024
* Telemetry emission and heuristics are now independent.
* profiling.enabled is four-state now (true, false, auto, undefined)
* Adds injectionEnabled config
* Removes profiling.{ssi, heuristicsEnabled} config.
* Move derived telemetry.logCollection value computation to _applyCalculated()
* Tidy _merge()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants