Do not require appsec modules when disabling appsec if they have not been required before #4244
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Overall package sizeSelf size: 6.3 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2024-04-18 15:25:33 Comparing candidate commit 043efb7 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 261 metrics, 5 unstable metrics. |
iunanua
force-pushed
the
igor/do-not-require-appsec-when-disabling
branch
from
2a8f2a0
to
aaf4bfa
Compare
|
We should take care of this point where appsec could be enabled via RC. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4244 +/- ##
==========================================
- Coverage 73.16% 69.19% -3.98%
==========================================
Files 245 1 -244
Lines 10442 198 -10244
Branches 33 33
==========================================
- Hits 7640 137 -7503
+ Misses 2802 61 -2741 ☔ View full report in Codecov by Sentry. |
iunanua
commented
Apr 18, 2024
packages/dd-trace/src/proxy.js
Outdated
| @@ -58,7 +79,7 @@ class Tracer extends NoopProxy { | |||
| } | |||
|
|
|||
| if (config.remoteConfig.enabled && !config.isCiVisibility) { | |||
| const rc = remoteConfig.enable(config) | |||
| const rc = remoteConfig.enable(config, this) | |||
There was a problem hiding this comment.
here we could pass to RC this._modules.appsec instead this. That way RC wouldn't need to know where to obtain the appsec module. It'd directly invoke enable or disable.
There was a problem hiding this comment.
good point, but i feel like we have this habit of passing the tracer instance a bit everywhere, so, up to you 🤷
There was a problem hiding this comment.
ok, I'll leave it as it is
There was a problem hiding this comment.
It's a habit we want to revert ASAP, so I'd use the boy scout rule here if possible.
iunanua
changed the title
uurien
reviewed
Apr 18, 2024
uurien
previously approved these changes
Apr 18, 2024
simon-id
reviewed
Apr 18, 2024
simon-id
reviewed
Apr 18, 2024
Co-authored-by: simon-id <simon.id@datadoghq.com>
|
there are some integration tests for es-build. Let me take a look |
Qard
approved these changes
Apr 19, 2024
|
esbuild tests are working as expected 👍 |
|
@iunanua where are they running ? i can't find them in the CI |
|
AFAIK they are not in the CI. |
uurien
approved these changes
Apr 23, 2024
simon-id
requested changes
Apr 23, 2024
simon-id
approved these changes
Apr 23, 2024
tlhunter
pushed a commit
that referenced
this pull request
Apr 25, 2024
…been required before (#4244) * mark if a module has been required before * Add esm bundler comment * Rename TracerModule as LazyModule * Handle appsec enablement/disablement via RC * Test module.enable call arguments * Check appsec and iast are not enabled by default * Check appsec and iast are not enabled via RC when they are not enabled by default * Update packages/dd-trace/src/proxy.js Co-authored-by: simon-id <simon.id@datadoghq.com> * Pass appsec module to RC instead of the tracer --------- Co-authored-by: simon-id <simon.id@datadoghq.com>
tlhunter
pushed a commit
that referenced
this pull request
Apr 25, 2024
…been required before (#4244) * mark if a module has been required before * Add esm bundler comment * Rename TracerModule as LazyModule * Handle appsec enablement/disablement via RC * Test module.enable call arguments * Check appsec and iast are not enabled by default * Check appsec and iast are not enabled via RC when they are not enabled by default * Update packages/dd-trace/src/proxy.js Co-authored-by: simon-id <simon.id@datadoghq.com> * Pass appsec module to RC instead of the tracer --------- Co-authored-by: simon-id <simon.id@datadoghq.com>
tlhunter
pushed a commit
that referenced
this pull request
Apr 25, 2024
…been required before (#4244) * mark if a module has been required before * Add esm bundler comment * Rename TracerModule as LazyModule * Handle appsec enablement/disablement via RC * Test module.enable call arguments * Check appsec and iast are not enabled by default * Check appsec and iast are not enabled via RC when they are not enabled by default * Update packages/dd-trace/src/proxy.js Co-authored-by: simon-id <simon.id@datadoghq.com> * Pass appsec module to RC instead of the tracer --------- Co-authored-by: simon-id <simon.id@datadoghq.com>
tlhunter
pushed a commit
that referenced
this pull request
Apr 29, 2024
…been required before (#4244) * mark if a module has been required before * Add esm bundler comment * Rename TracerModule as LazyModule * Handle appsec enablement/disablement via RC * Test module.enable call arguments * Check appsec and iast are not enabled by default * Check appsec and iast are not enabled via RC when they are not enabled by default * Update packages/dd-trace/src/proxy.js Co-authored-by: simon-id <simon.id@datadoghq.com> * Pass appsec module to RC instead of the tracer --------- Co-authored-by: simon-id <simon.id@datadoghq.com>
tlhunter
pushed a commit
that referenced
this pull request
Apr 29, 2024
…been required before (#4244) * mark if a module has been required before * Add esm bundler comment * Rename TracerModule as LazyModule * Handle appsec enablement/disablement via RC * Test module.enable call arguments * Check appsec and iast are not enabled by default * Check appsec and iast are not enabled via RC when they are not enabled by default * Update packages/dd-trace/src/proxy.js Co-authored-by: simon-id <simon.id@datadoghq.com> * Pass appsec module to RC instead of the tracer --------- Co-authored-by: simon-id <simon.id@datadoghq.com>
tlhunter
pushed a commit
that referenced
this pull request
Apr 29, 2024
…been required before (#4244) * mark if a module has been required before * Add esm bundler comment * Rename TracerModule as LazyModule * Handle appsec enablement/disablement via RC * Test module.enable call arguments * Check appsec and iast are not enabled by default * Check appsec and iast are not enabled via RC when they are not enabled by default * Update packages/dd-trace/src/proxy.js Co-authored-by: simon-id <simon.id@datadoghq.com> * Pass appsec module to RC instead of the tracer --------- Co-authored-by: simon-id <simon.id@datadoghq.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Use
LazyModules to handle how a module is loaded, enabled and disabled by theTracer.It takes into account ESM bundles keeping requires in place.
Also handles enablements/disablements via RC
Motivation
do not require a module to disable it if it hasn't enabled before
Plugin Checklist
Additional Notes