Skip to content

[nodejs] fix remote config polling slowing down multiple tests #6808

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.

Sign up for GitHub

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

Draft
rochdev wants to merge 15 commits into
base: main
Choose a base branch
from
+11 −5
Draft

[nodejs] fix remote config polling slowing down multiple tests #6808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
c8ceec2
[nodejs] fix slowest tests
rochdev Apr 24, 2026
77646f0
also fix APPSEC_API_SECURITY_RC
rochdev Apr 24, 2026
fea7637
apply the same fix everywhere
rochdev Apr 24, 2026
02ccdf9
fix bug in wait_for
rochdev Apr 24, 2026
9417c54
fix sleep too short
rochdev Apr 24, 2026
a2db25c
fix dotnet
rochdev Apr 24, 2026
173493b
isolate the changes to nodejs
rochdev Apr 24, 2026
d907741
undo change
rochdev Apr 24, 2026
fa0032e
fix lint
rochdev Apr 24, 2026
1f59966
decrease poll interval and remove sleep for nodejs
rochdev Apr 25, 2026
49571fd
add back tiny sleep
rochdev Apr 25, 2026
bc70b59
remove need for sleep
rochdev Apr 25, 2026
00fea28
reduce polling observer timeout
rochdev Apr 25, 2026
73b9b1b
scope optimization only for nodejs
rochdev Apr 25, 2026
ee07ec9
undo unfruitiful changes
rochdev Apr 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions tests/debugger/test_debugger_inproduct_enablement.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,13 @@ class Test_Debugger_InProduct_Enablement_Dynamic_Instrumentation(debugger.BaseDe
"""

def setup_inproduct_enablement_di(self):
def _send_config(*, enabled: bool | None = None, reset: bool = True):
def _send_config(*, enabled: bool | None = None, reset: bool = True, wait_installed: bool = False):
probe = json.loads(self._probe_template)
probe["id"] = debugger.generate_probe_id("log")
self.set_probes([probe])

self.send_rc_apm_tracing_and_probes(dynamic_instrumentation_enabled=enabled, reset=reset)
if wait_installed:
self.wait_for_all_probes(statuses=["INSTALLED"], timeout=TIMEOUT)
self.send_weblog_request("/debugger/log")

self.initialize_weblog_remote_config()
Expand All @@ -41,10 +42,10 @@ def _send_config(*, enabled: bool | None = None, reset: bool = True):
_send_config()
self.di_initial_disabled = not self.wait_for_all_probes(statuses=["EMITTING"], timeout=TIMEOUT)

_send_config(enabled=True, reset=False)
_send_config(enabled=True, reset=False, wait_installed=context.library == "nodejs")
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Avoid runtime library branching in shared test setup

This introduces Node-specific behavior directly in shared test logic (context.library == "nodejs") instead of splitting behavior via manifests/separate test methods. Per .cursor/rules/pr-review.mdc (rule 2), runtime library branching in tests should be avoided because it hides per-library behavior differences inside one test path and makes scenario coverage and future maintenance error-prone.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

@rochdev rochdev Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is by design. There are a lot of hacks and workarounds in system tests which makes it difficult to make an improvement across the board. Limiting them to just one language avoids weird side-effects in other languages.

Copy link
Copy Markdown
Collaborator

@cbeauchesne cbeauchesne Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, we try to avoid any language-specific assertions. Unfortunately, yes, there are (too many) execpetions, but they are not an argument to add more 😉

FMI, this changes seems to me very legit (wait for explicit signal seems always better), it does not work for other libs ?

Now, it's not my decisions here, if debugger team is happy with this, I'm ok. Could you ask a review from them ? cc @tylfin

Copy link
Copy Markdown
Collaborator

@cbeauchesne cbeauchesne Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw the approval from @tylfin, so AGTM.

But I'm curious to know if we could apply this to other libs.

Copy link
Copy Markdown
Member Author

@rochdev rochdev Apr 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to validate that as system tests just keep blowing up in several languages (especially Java and PHP) even with no changes at all. They're also much much slower than Node, so timing specific things tend to impact them a lot. I'd rather start with Node as we know it works, and then expand if we want, or I can yolo every language if that's preferred (except the sleep because that cannot work in languages with asynchronous workers)

Copy link
Copy Markdown
Collaborator

@cbeauchesne cbeauchesne May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change seems to me very valid, and should be applied on all languages. WDYT @tylfin ?

Copy link
Copy Markdown
Member Author

@rochdev rochdev May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually splitting up the work as separate PRs, starting with #6851

self.di_explicit_enabled = self.wait_for_all_probes(statuses=["EMITTING"], timeout=TIMEOUT)

_send_config(reset=False)
_send_config(reset=False, wait_installed=context.library == "nodejs")
self.di_empty_config = self.wait_for_all_probes(statuses=["EMITTING"], timeout=TIMEOUT)

_send_config(enabled=False, reset=False)
Expand Down
1 change: 1 addition & 0 deletions utils/_context/_scenarios/debugger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ def configure(self, config: pytest.Config):
self.weblog_container.environment["DD_DYNAMIC_INSTRUMENTATION_UPLOAD_FLUSH_INTERVAL"] = "0.1"
elif library == "nodejs":
self.weblog_container.environment["DD_DYNAMIC_INSTRUMENTATION_UPLOAD_INTERVAL_SECONDS"] = "0.1"
self.weblog_container.environment["DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS"] = "0.2"
elif library in ("java", "dotnet"):
# Java and .NET use UPLOAD_FLUSH_INTERVAL in milliseconds
self.weblog_container.environment["DD_DYNAMIC_INSTRUMENTATION_UPLOAD_FLUSH_INTERVAL"] = "100"
Expand Down
3 changes: 3 additions & 0 deletions utils/_context/_scenarios/endtoend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,9 @@ def configure(self, config: pytest.Config):

library = self.weblog_infra.library_name

if library == "nodejs":
Copy link
Copy Markdown
Collaborator

@cbeauchesne cbeauchesne Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll make the default configuration for almost all scenarios different from the default configuration our customers are using. Can you rather add this on individual scenarios when there is an explicit advantage ?

Copy link
Copy Markdown
Member Author

@rochdev rochdev Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an explicit advantage everywhere to being faster. Doing something faster is not much different than what customers are using, it just happens faster. It's not like an option that changes behaviour where I would be more in agreement (and even then if the config exists we should test multiple values at that point)

self.weblog_infra.http_container.environment.setdefault("DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS", "0.5")

if self._library_interface_timeout is None:
if library == "java":
self.library_interface_timeout = 25
Expand Down
3 changes: 2 additions & 1 deletion utils/_remote_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,8 @@ def remote_config_applied(data: dict) -> bool:

library.wait_for(remote_config_applied, timeout=30)
# ensure the library has enough time to apply the config to all subprocesses
time.sleep(2)
if context.library.name != "nodejs":
time.sleep(2)
Comment on lines +144 to +145
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep a post-ACK delay for Node remote-config updates

send_state now skips the post-acknowledgement wait entirely for Node.js, even though this function explicitly needs a grace period for subprocess propagation. In setups that call send_state(...) and immediately issue a request (for example AppSec RC setup flows), this can race: the ACK can be observed while some worker processes are still on the previous config, producing intermittent false negatives only on Node.js. A short Node-specific delay (e.g., one poll interval) is still needed to preserve determinism.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

@rochdev rochdev Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to Claude this doesn't apply to Node.


return current_states

Expand Down
Loading