make tracer config available to plugins #3235

tlhunter · 2023-06-12T22:15:20Z

What does this PR do?

currently we need to shove a lot of random values into _getSharedConfig to pass from tracer config to plugin config
with these changes we'll no longer need to edit that list
there are plenty of situations where a plugin needs access to global tracer configuration

Motivation

it's annoying that global configuration settings can't be accessed within a plugin

Plugin Checklist

github-actions · 2023-06-12T22:16:03Z

Overall package size

Self size: 4.31 MB
Deduped: 60.67 MB
No deduping: 60.71 MB

Dependency sizes

name	version	self size	total size
@datadog/pprof	2.2.1	14.24 MB	15.12 MB
@datadog/native-iast-taint-tracking	1.4.1	14.85 MB	14.86 MB
@datadog/native-appsec	3.2.0	13.38 MB	13.39 MB
protobufjs	7.1.2	2.76 MB	6.55 MB
@datadog/native-iast-rewriter	2.0.1	2.09 MB	2.1 MB
@opentelemetry/core	1.3.1	784.66 kB	1.37 MB
@datadog/native-metrics	2.0.0	898.77 kB	1.3 MB
@opentelemetry/api	1.4.1	780.32 kB	780.32 kB
opentracing	0.14.7	194.81 kB	194.81 kB
semver	7.3.8	88.2 kB	118.6 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.0.1	59.52 kB	59.52 kB
ignore	5.2.0	48.87 kB	48.87 kB
import-in-the-middle	1.3.5	34.34 kB	38.81 kB
istanbul-lib-coverage	3.2.0	29.34 kB	29.34 kB
retry	0.10.1	27.44 kB	27.44 kB
lodash.uniq	4.5.0	25.01 kB	25.01 kB
limiter	1.1.5	23.17 kB	23.17 kB
lodash.kebabcase	4.1.1	17.75 kB	17.75 kB
lodash.pick	4.4.0	16.33 kB	16.33 kB
node-abort-controller	3.0.1	14.33 kB	14.33 kB
crypto-randomuuid	1.0.0	11.18 kB	11.18 kB
diagnostics_channel	1.1.0	7.07 kB	7.07 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}

codecov · 2023-06-12T22:19:17Z

Codecov Report

Merging #3235 (e013f23) into master (7b645b9) will increase coverage by 17.85%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master    #3235       +/-   ##
===========================================
+ Coverage   68.16%   86.02%   +17.85%     
===========================================
  Files         184      190        +6     
  Lines        6990     7451      +461     
  Branches       33       33               
===========================================
+ Hits         4765     6410     +1645     
+ Misses       2225     1041     -1184

Impacted Files	Coverage Δ
packages/dd-trace/src/plugin_manager.js	`98.43% <100.00%> (-0.03%)`	⬇️
packages/dd-trace/src/plugins/plugin.js	`66.66% <100.00%> (+2.15%)`	⬆️

... and 62 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pr-commenter · 2023-06-12T22:23:47Z

Benchmarks

Comparing candidate commit e013f23 in PR branch tlhunter/simplify-config with baseline commit 7b645b9 in branch master.

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

tlhunter · 2023-06-12T22:40:34Z

packages/dd-trace/src/plugins/database.js

@@ -38,13 +38,17 @@ class DatabasePlugin extends StoragePlugin {
  }

  injectDbmQuery (query, serviceName, isPreparedStatement = false) {
-    if (this.config.dbmPropagationMode === 'disabled') {
+    const dbmPropagationMode = this.getConfig('dbmPropagationMode')


This is an example of how plugins should be loading overridable configuration options.

This is adding overhead in possibly hot paths.

I've removed getConfig

tlhunter · 2023-06-13T16:17:23Z

packages/dd-trace/src/plugins/plugin.js

    this._subscriptions = []
    this._enabled = false
    this._tracer = tracer
+    this.config = {} // plugin-specific configuration, unset until .configure() is called


This value is set later but I like having its presence be a little more explicit.

Qard · 2023-06-13T18:15:11Z

packages/dd-trace/src/plugins/plugin.js

+  // helper method to retrieve a configuration value that is expected to be overridden
+  getConfig (name) {
+    if (name in this.config) {
+      return this.config[name]
+    }
+
+    if (name in this._tracerConfig) {
+      return this._tracerConfig[name]
+    }


I think I would prefer config objects for each plugin be pre-computed so we don't have to call getConfig and do all this checking at runtime every time. Could try nesting plugin configs under the global configs something like config.plugins.$plugin_name and compute the full config object ahead of time. If you want to retain the fall-through logic it could also do something like Object.setPrototypeOf(pluginConfig, tracerConfig) or probably replace tracerConfig with something a little more specifically tuned to just the things we've explicitly marked as what we want to be available to plugins.

I'm not sure I like the idea of automatic fallback on everything as there's then a key conflict situation to worry about where we may want a global config available which shares a name with a plugin config or we may not want a particular config setting if not explicitly provided. 🤔

There could indeed be conflicts, and not all configurations should be available to plugins either.

The fall through logic seems like a bit of a hack and getConfig was just moving that around a little bit. I've simplified the PR to not use fallback logic and instead expect plugins to know if the configuration is plugin-specific or tracer-global.

rochdev · 2023-06-13T20:33:11Z

packages/dd-trace/src/plugins/database.js

@@ -38,13 +38,17 @@ class DatabasePlugin extends StoragePlugin {
  }

  injectDbmQuery (query, serviceName, isPreparedStatement = false) {
-    if (this.config.dbmPropagationMode === 'disabled') {
+    const dbmPropagationMode = this.getConfig('dbmPropagationMode')


This is adding overhead in possibly hot paths.

rochdev · 2023-06-13T20:34:36Z

packages/dd-trace/src/plugins/plugin.js

+  // helper method to retrieve a configuration value that is expected to be overridden
+  getConfig (name) {
+    if (name in this.config) {
+      return this.config[name]
+    }
+
+    if (name in this._tracerConfig) {
+      return this._tracerConfig[name]
+    }


There could indeed be conflicts, and not all configurations should be available to plugins either.

rochdev · 2023-06-13T20:35:22Z

packages/dd-trace/src/plugins/plugin.js

@@ -26,10 +26,23 @@ class Subscription {
 }

 module.exports = class Plugin {
-  constructor (tracer) {
+  constructor (tracer, tracerConfig) {


Passing this here means that if the tracer config is updated, the update won't propagate anymore.

tracerConfig is an object, so wouldn't modifying properties of that object propagate everywhere?

Qard

I think the differentiated config approach is reasonable, but I feel like we should probably do a more complete refactoring of the config system at some point. 🤔

tlhunter · 2023-06-13T22:25:05Z

I think the differentiated config approach is reasonable, but I feel like we should probably do a more complete refactoring of the config system at some point. 🤔

I agree 100%

change applied

tlhunter added the semver-minor label Jun 12, 2023

tlhunter force-pushed the tlhunter/simplify-config branch from 8d8339b to 1d75d1a Compare June 12, 2023 22:17

tlhunter force-pushed the tlhunter/simplify-config branch from 1d75d1a to 67ae56f Compare June 12, 2023 22:38

tlhunter commented Jun 12, 2023

View reviewed changes

tlhunter added semver-patch and removed semver-minor labels Jun 12, 2023

tlhunter marked this pull request as ready for review June 12, 2023 22:48

tlhunter requested a review from a team as a code owner June 12, 2023 22:48

tlhunter force-pushed the tlhunter/simplify-config branch from 67ae56f to 0654b7a Compare June 12, 2023 22:48

tlhunter changed the title ~~simplicfy config and stop overloading _getSharedConfig~~ simplify config and stop overloading _getSharedConfig Jun 12, 2023

tlhunter force-pushed the tlhunter/simplify-config branch from 0654b7a to c6797e4 Compare June 13, 2023 16:16

tlhunter commented Jun 13, 2023

View reviewed changes

Qard reviewed Jun 13, 2023

View reviewed changes

rochdev previously requested changes Jun 13, 2023

View reviewed changes

tlhunter force-pushed the tlhunter/simplify-config branch from c6797e4 to ede03e1 Compare June 13, 2023 21:24

make tracer config available to plugins

e013f23

tlhunter force-pushed the tlhunter/simplify-config branch from ede03e1 to e013f23 Compare June 13, 2023 21:31

tlhunter changed the title ~~simplify config and stop overloading _getSharedConfig~~ make tracer config available to plugins Jun 13, 2023

Qard approved these changes Jun 13, 2023

View reviewed changes

tlhunter merged commit 05d8c18 into master Jun 13, 2023

tlhunter deleted the tlhunter/simplify-config branch June 13, 2023 22:26

nsavoire pushed a commit that referenced this pull request Jun 20, 2023

make tracer config available to plugins (#3235)

dac33c6

nsavoire pushed a commit that referenced this pull request Jun 20, 2023

make tracer config available to plugins (#3235)

a183535

nsavoire pushed a commit that referenced this pull request Jun 21, 2023

make tracer config available to plugins (#3235)

18d8196

nsavoire mentioned this pull request Jun 21, 2023

v4.3.0 proposal #3267

Merged

This was referenced Jun 21, 2023

v2.37.0 proposal #3268

Merged

v3.24.0 proposal #3266

Merged

tlhunter added a commit that referenced this pull request Jun 23, 2023

make tracer config available to plugins (#3235)

ed290a2

tlhunter added a commit that referenced this pull request Jun 23, 2023

make tracer config available to plugins (#3235)

463ef6d

tlhunter added a commit that referenced this pull request Jun 23, 2023

make tracer config available to plugins (#3235)

9f6551f

This was referenced Jun 27, 2023

dbmPropagationMode error on v3.24.0 #3296

Closed

plugin manager: allow plugin reconfiguration #3332

Closed

erikvanbrakel mentioned this pull request Jul 5, 2024

[DEVX-3565] add neo4j plugin for v4.41 mazedesignhq/dd-trace-js#15

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make tracer config available to plugins #3235

make tracer config available to plugins #3235

tlhunter commented Jun 12, 2023 •

edited

Loading

github-actions bot commented Jun 12, 2023 •

edited

Loading

codecov bot commented Jun 12, 2023 •

edited

Loading

pr-commenter bot commented Jun 12, 2023 •

edited

Loading

tlhunter Jun 12, 2023

rochdev Jun 13, 2023

tlhunter Jun 13, 2023

tlhunter Jun 13, 2023

Qard Jun 13, 2023 •

edited

Loading

rochdev Jun 13, 2023

tlhunter Jun 13, 2023

rochdev Jun 13, 2023

rochdev Jun 13, 2023

rochdev Jun 13, 2023

tlhunter Jun 13, 2023

Qard left a comment

tlhunter commented Jun 13, 2023

make tracer config available to plugins #3235

make tracer config available to plugins #3235

Conversation

tlhunter commented Jun 12, 2023 • edited Loading

What does this PR do?

Motivation

Plugin Checklist

github-actions bot commented Jun 12, 2023 • edited Loading

Overall package size

Dependency sizes

codecov bot commented Jun 12, 2023 • edited Loading

Codecov Report

pr-commenter bot commented Jun 12, 2023 • edited Loading

Benchmarks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qard Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qard left a comment

Choose a reason for hiding this comment

tlhunter commented Jun 13, 2023

tlhunter commented Jun 12, 2023 •

edited

Loading

github-actions bot commented Jun 12, 2023 •

edited

Loading

codecov bot commented Jun 12, 2023 •

edited

Loading

pr-commenter bot commented Jun 12, 2023 •

edited

Loading

Qard Jun 13, 2023 •

edited

Loading