Skip to content

perf(ffe): fix FEATURE_FLAGGING_AND_EXPERIMENTATION CI regression on Node.js - #7488

Merged
rochdev merged 3 commits into
mainfrom
rochdev/ffe-flush-exposures
Sep 11, 2026
Merged

perf(ffe): fix FEATURE_FLAGGING_AND_EXPERIMENTATION CI regression on Node.js#7488
rochdev merged 3 commits into
mainfrom
rochdev/ffe-flush-exposures

Conversation

@rochdev

@rochdev rochdev commented Aug 10, 2026

Copy link
Copy Markdown
Member

Summary

  • FEATURE_FLAGGING_AND_EXPERIMENTATION regressed from <3 minutes to 7+ minutes in dd-trace-js CI (nodejs express4 shard).
  • Two independent costs were identified from the real CI job log/timing breakdown, and both are fixed here:

1. Exposures: waiting on a periodic flush timer instead of flushing immediately

  • tests/ffe/test_exposures.py's wait_for_exposure_event/wait_for_min_exposure_count helpers were purely polling interfaces.agent.wait_for(..., timeout=30) for exposure events, at the mercy of the exposures writer's periodic flush interval.
  • This adds a weblog.get("/flush") call before polling, so the weblog forces its exposures writer to flush immediately via the existing /flush endpoint pattern, instead of waiting on the timer.
  • Wired the Node.js side: the 4 Node weblogs (express, express4-typescript, fastify, nextjs) now publish the ffe:writers:flush diagnostics channel (already consumed inside dd-trace-js) from their /flush handlers.
  • weblog.get("/flush") is a no-op for weblogs/languages that don't implement the route, so this is safe across all libraries.
  • Confirmed in CI: test_exposures.py now completes in ~30s instead of stalling on the writer's timer.

2. EVP flagevaluation: missing_feature tests were still executing under xfail, burning their full timeout

  • missing_feature (and bug, etc.) map to pytest.mark.xfail, not skip (utils/_decorators.py) — the test body still runs. dd-trace-js has no implementation of /api/v2/flagevaluation at all, so tests/ffe/test_flag_eval_evp.py's 8 non-@scenario_crash classes each ran to completion and burned their full 30s wait_for timeout before failing as expected. That's ~240s, the actual dominant cost of the scenario.
  • Adds @slow (existing skip_if_xfail mechanism, already used on 3 sibling classes in this file via @scenario_crash) to the remaining 8 classes. @slow only converts a test to a hard skip when it's paired with the manifest's declaration marker for the current library — so this only skips for libraries where the feature is currently declared missing (today: Node.js for all 8, plus golang for the 3 ObserveFullData_* classes it doesn't yet implement).
  • Self-healing: once a library implements the feature and its manifest entry is removed, @slow becomes a no-op and the test runs for real. Confirmed against golang's actual CI timing (the one library that implements this today) that real event delivery is on the order of 1-2s, nowhere near the 30s ceiling, so no added flakiness risk anywhere.

Test plan

  • Re-run the FEATURE_FLAGGING_AND_EXPERIMENTATION scenario for nodejs (express4/express4-typescript/fastify/nextjs) in CI and confirm it completes well under 3 minutes.
  • Confirm tests/ffe/test_exposures.py still passes for nodejs.
  • Confirm tests/ffe/test_flag_eval_evp.py still passes for golang (only the 3 ObserveFullData_* classes should skip there).
  • Confirm other languages are unaffected.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

CODEOWNERS have been resolved as:

tests/ffe/test_exposures_datadog_agent.py                               @DataDog/feature-flagging-and-experimentation-sdk
tests/ffe/test_flag_eval_evp.py                                         @DataDog/feature-flagging-and-experimentation-sdk
utils/build/docker/nodejs/express/app.js                                @DataDog/system-tests-reviewers
utils/build/docker/nodejs/express4-typescript/app.ts                    @DataDog/system-tests-reviewers
utils/build/docker/nodejs/fastify/app.js                                @DataDog/system-tests-reviewers
utils/build/docker/nodejs/nextjs/src/app/flush/route.js                 @DataDog/system-tests-reviewers

@datadog-datadog-us1-prod

datadog-datadog-us1-prod Bot commented Aug 10, 2026

Copy link
Copy Markdown

Tests

All CI checks and tests passed.

🎉 All green!

🧪 All tests passed
❄️ No new flaky tests detected

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: f90923f | Docs | View more details | Give us feedback!

@rochdev rochdev changed the title perf(ffe): flush exposure writer instead of waiting on its periodic timer perf(ffe): fix FEATURE_FLAGGING_AND_EXPERIMENTATION CI regression on Node.js Aug 11, 2026
@rochdev
rochdev force-pushed the rochdev/ffe-flush-exposures branch from 67e5129 to 9424bdc Compare September 8, 2026 16:37
@rochdev
rochdev marked this pull request as ready for review September 8, 2026 22:52
@rochdev
rochdev requested review from a team as code owners September 8, 2026 22:52
@rochdev
rochdev requested review from dd-oleksii and greghuels and removed request for a team September 8, 2026 22:52
Comment thread utils/build/docker/nodejs/express4-typescript/app.ts
@rochdev
rochdev force-pushed the rochdev/ffe-flush-exposures branch from 9424bdc to afaa444 Compare September 10, 2026 16:49
@rochdev
rochdev force-pushed the rochdev/ffe-flush-exposures branch from afaa444 to 8184c9e Compare September 11, 2026 00:25
rochdev and others added 3 commits September 11, 2026 15:09
…imer

FEATURE_FLAGGING_AND_EXPERIMENTATION regressed to 7+ minutes in CI.
test_exposures.py's wait_for_exposure_event/wait_for_min_exposure_count
helpers were polling for up to 30s waiting on the exposures writer's
periodic flush. Force an immediate flush via the existing /flush
endpoint (already wired to a ffe:writers:flush diagnostics channel in
dd-trace-js) before polling, so events land immediately instead of on
the writer's timer.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tricted-require

express/fastify's eslint config bans require('diagnostics_channel')
via n/no-restricted-require. The node: prefixed specifier resolves to
the same core module and isn't covered by the restriction.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…iming out

These tests were still executing under xfail (missing_feature maps to
xfail, not skip), so each one burned its full wait_for timeout (~30s)
waiting for an event that can never arrive on libraries without the
feature. @slow pairs with the manifest's declaration marker to convert
those to an unconditional skip, which is a no-op once a library
implements the feature and the manifest entry is removed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@cbeauchesne
cbeauchesne force-pushed the rochdev/ffe-flush-exposures branch from 8184c9e to f90923f Compare September 11, 2026 13:09
@rochdev
rochdev merged commit e751553 into main Sep 11, 2026
713 checks passed
@rochdev
rochdev deleted the rochdev/ffe-flush-exposures branch September 11, 2026 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants