Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DI] Add namespace to all DI related config options #5077

Merged
merged 4 commits into from
Jan 9, 2025

Conversation

watson
Copy link
Collaborator

@watson watson commented Jan 8, 2025

What does this PR do?

This affects all DI related config options, where the experimental namespace is removed and, and a dynamicInstrumentation namespace is introduced.

Changes:

  • experimental.dynamicInstrumentationEnabled -> dynamicInstrumentation.enabled
  • experimental.dynamicInstrumentationRedactedIdentifiers -> dynamicInstrumentation.redactedIdentifiers
  • experimental.dynamicInstrumentationRedactionExcludedIdentifiers -> dynamicInstrumentation.redactionExcludedIdentifiers

Motivation

  • There's no longer any need to use the experimental namespace as we're gearing up for a limited public preview of the DI feature.
  • Prefixing all DI config options with dynamicInstrumentation quickly gets tedious. Using a namespace is a much better experience.

Plugin Checklist

Additional Notes

Normally this would be considered a breaking change. But since the DI feature is behind a feature flag, we know that nobody is currently using it (besides me). So I'd say that it's safe to make this change without also adding a fallback for users using the old naming convention.

If we didn't do this now, it would be much harder to change in the future once we launch this feature.

Copy link
Collaborator Author

watson commented Jan 8, 2025

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

@watson watson mentioned this pull request Jan 8, 2025
2 tasks
@watson watson self-assigned this Jan 8, 2025
Copy link

github-actions bot commented Jan 8, 2025

Overall package size

Self size: 8.44 MB
Deduped: 94.78 MB
No deduping: 95.3 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.3.0 | 29.43 MB | 29.43 MB | | @datadog/native-appsec | 8.4.0 | 19.25 MB | 19.26 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.6.1 | 2.59 MB | 2.73 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 | 826.22 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 | | 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 | | 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 | | 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

@pr-commenter
Copy link

pr-commenter bot commented Jan 8, 2025

Benchmarks

Benchmark execution time: 2025-01-09 15:43:52

Comparing candidate commit 62dfe22 in PR branch watson/DEBUG-3310/di-config-namespace with baseline commit b36ce05 in branch master.

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

Base automatically changed from watson/DEBUG-2630/add-redaction to master January 8, 2025 09:32
This affects all DI related config options, where the `experimental`
namespace is removed and, and a `dynamicInstrumentation` namespace is
introduced.

Changes:

- `experimental.dynamicInstrumentationEnabled` -> `dynamicInstrumentation.enabled`
- `experimental.dynamicInstrumentationRedactedIdentifiers` ->
  `dynamicInstrumentation.redactedIdentifiers`
- `experimental.dynamicInstrumentationRedactionExcludedIdentifiers` ->
  `dynamicInstrumentation.redactionExcludedIdentifiers`
@watson watson force-pushed the watson/DEBUG-3310/di-config-namespace branch from f8c0232 to a321a49 Compare January 8, 2025 09:58
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.15%. Comparing base (b36ce05) to head (a321a49).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5077      +/-   ##
==========================================
- Coverage   88.97%   82.15%   -6.83%     
==========================================
  Files          11        5       -6     
  Lines         635      325     -310     
==========================================
- Hits          565      267     -298     
+ Misses         70       58      -12     

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

@watson watson marked this pull request as ready for review January 9, 2025 06:20
@watson watson requested review from a team as code owners January 9, 2025 06:20
@watson
Copy link
Collaborator Author

watson commented Jan 9, 2025

I know the tests validating the config options for use with telemetry are failing, but I want this PR approved before I merge the PR in dd-go (https://github.com/DataDog/dd-go/pull/164428) to update the config_norm_rules.json file, as updating the file in dd-go, system-tests and dd-trace-js has to happen in very quick succession so that this PR can be merge, as CI in master will be broken for a short period of time until all 4 PR's has been merged

@simon-id
Copy link
Member

simon-id commented Jan 9, 2025

I would suggest to keep the experimental option to be retrocompatible, and have the non experimental have priority (there is even a test for this in config.spec.js), just like we did for AppSec back in the days

@watson
Copy link
Collaborator Author

watson commented Jan 9, 2025

@simon-id this feature has never been used by anybody as it's behind a feature flag that we haven't enabled for anyone, so it's safe to rename these config options for now until we start dogfooding and opening up for limited public preview

@simon-id
Copy link
Member

simon-id commented Jan 9, 2025

system test failure is the config telemetry field mismatch

@watson watson enabled auto-merge (squash) January 9, 2025 17:56
@watson watson merged commit 6e5d2e8 into master Jan 9, 2025
309 checks passed
@watson watson deleted the watson/DEBUG-3310/di-config-namespace branch January 9, 2025 17:58
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.

2 participants