Skip to content

Commit 1b77ef8

Browse files
rochdevclaude
andcommitted
fix(openfeature): address review feedback on tests and SSI scope
- Fix test:openfeature script so the instrumentation spec actually runs: mocha treats a brace-only glob segment as a literal filename, so add a wildcard segment that both mocha and verify-exercised-tests resolve. - Add real esbuild/webpack black-box tests that bundle @openfeature/server-sdk (not marked external) and assert dd-trace's bundler-instrumentation mechanism still bridges the real event emitter into the vendored provider. - Correct comments in proxy.js and the dd-trace-api plugin: SSI itself is fixed by this PR's vendoring; the only remaining gap is that the dd-trace-api shim has no openfeature handoff. Generated with Claude Code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent caa112c commit 1b77ef8

9 files changed

Lines changed: 244 additions & 1 deletion

File tree

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
#!/usr/bin/env node
2+
'use strict'
3+
4+
/* eslint-disable no-console */
5+
6+
// Black-box coverage for the vendored flagging provider under esbuild. Unit tests on
7+
// `server-sdk-bridge.js` fake the real `@openfeature/server-sdk` emitter in plain JS and
8+
// never touch a bundler, so they cannot catch a regression in the generic bundler
9+
// instrumentation mechanism (`modulesOfInterest` / `dd-trace:bundler:load`) that the
10+
// `openfeature-server-sdk` instrumentation relies on to see the app's inlined require of
11+
// `@openfeature/server-sdk` -- deliberately left out of `external` below so the plugin has
12+
// to intercept it, the same as a real customer bundle would. This builds and runs
13+
// `openfeature-app.js`, which fails unless the bundled provider is real AND the bridged
14+
// emitter actually fires.
15+
16+
const fs = require('fs')
17+
const path = require('path')
18+
const assert = require('assert')
19+
const { execFileSync } = require('child_process')
20+
const esbuild = require('esbuild')
21+
const ddPlugin = require('../../esbuild') // dd-trace/esbuild
22+
23+
const OUTFILE = path.join(__dirname, 'openfeature-out.js')
24+
25+
async function main () {
26+
try {
27+
await esbuild.build({
28+
entryPoints: [path.join(__dirname, 'openfeature-app.js')],
29+
outfile: OUTFILE,
30+
bundle: true,
31+
platform: 'node',
32+
target: 'node18',
33+
plugins: [ddPlugin],
34+
external: [
35+
'pg', 'mysql2', 'better-sqlite3', 'sqlite3', 'mysql', 'mariadb', 'oracledb', 'pg-query-stream', 'tedious',
36+
'@yaacovcr/transform',
37+
'@datadog/native-appsec', '@datadog/native-iast-taint-tracking', '@datadog/native-metrics',
38+
'@datadog/pprof', '@datadog/libdatadog',
39+
],
40+
})
41+
42+
const runOutput = execFileSync(process.execPath, [OUTFILE], { encoding: 'utf8' })
43+
assert(
44+
runOutput.includes('PROVIDER_OK'),
45+
`bundled app did not load a working OpenFeature provider:\n${runOutput}`
46+
)
47+
48+
console.log('ok')
49+
} finally {
50+
fs.rmSync(OUTFILE, { force: true })
51+
}
52+
}
53+
54+
main().catch((error) => {
55+
console.error(error)
56+
process.exitCode = 1
57+
})

integration-tests/esbuild/index.spec.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,12 @@ esbuildVersions.forEach((version) => {
9595
})
9696
})
9797

98+
it('bundles the vendored OpenFeature provider and bridges the real event emitter', () => {
99+
execSync('node ./build-and-test-openfeature.js', {
100+
timeout,
101+
})
102+
})
103+
98104
it('injects Git metadata into bundled applications', () => {
99105
execSync('node ./build-and-test-git-tags.js', {
100106
timeout,
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
#!/usr/bin/env node
2+
'use strict'
3+
4+
// Entry for `build-and-test-openfeature.js`. The flagging provider now ships vendored
5+
// inside dd-trace itself (see `vendor/rspack.config.js`), so enabling it through the public
6+
// API must keep working once bundled -- there is no longer an optional peer to lose.
7+
// `@openfeature/server-sdk` is bundled (inlined) here rather than marked external, so this
8+
// also exercises whether the generic bundler instrumentation mechanism still lets dd-trace's
9+
// `openfeature-server-sdk` require-hook observe the app's own inlined require, bridging the
10+
// real `ProviderEvents`/`OpenFeatureEventEmitter` into the vendored provider instead of
11+
// leaving it on the deferred, event-dropping stand-in.
12+
13+
// eslint-disable-next-line import/order
14+
const tracer = require('../../').init({ // dd-trace
15+
experimental: { flaggingProvider: { enabled: true } },
16+
})
17+
18+
const assert = require('assert')
19+
20+
const provider = tracer.openfeature
21+
22+
assert.strictEqual(
23+
provider?.constructor?.name,
24+
'FlaggingProvider',
25+
`expected the real Datadog FlaggingProvider, got ${provider?.constructor?.name}`
26+
)
27+
28+
// Must be required after dd-trace has initialized: dd-trace's require-hook instrumentation
29+
// (`openfeature-server-sdk.js`) only bridges the real emitter into the vendored provider if
30+
// its hook is already installed by the time the app requires `@openfeature/server-sdk`.
31+
const { ProviderEvents } = require('@openfeature/server-sdk')
32+
33+
let receivedDetails
34+
provider.events.addHandler(ProviderEvents.Ready, (details) => {
35+
receivedDetails = details
36+
})
37+
provider.events.emit(ProviderEvents.Ready, { fired: true })
38+
39+
assert.deepStrictEqual(
40+
receivedDetails,
41+
{ fired: true },
42+
'the openfeature-server-sdk instrumentation did not bridge the real emitter into the bundled provider'
43+
)
44+
45+
// eslint-disable-next-line no-console
46+
console.log('PROVIDER_OK')
47+
process.exit(0)
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
#!/usr/bin/env node
2+
'use strict'
3+
4+
/* eslint-disable no-console */
5+
6+
// Black-box coverage for the vendored flagging provider under webpack. Unit tests on
7+
// `server-sdk-bridge.js` fake the real `@openfeature/server-sdk` emitter in plain JS and
8+
// never touch a bundler, so they cannot catch a regression in the generic bundler
9+
// instrumentation mechanism (`modulesOfInterest` / `dd-trace:bundler:load`) that the
10+
// `openfeature-server-sdk` instrumentation relies on to see the app's inlined require of
11+
// `@openfeature/server-sdk` -- deliberately left out of `externals` below so the plugin has
12+
// to intercept it, the same as a real customer bundle would. This builds and runs
13+
// `openfeature-app.js`, which fails unless the bundled provider is real AND the bridged
14+
// emitter actually fires.
15+
16+
const fs = require('fs')
17+
const path = require('path')
18+
const assert = require('assert')
19+
const { execFileSync } = require('child_process')
20+
const webpack = require('webpack')
21+
const DatadogWebpackPlugin = require('../../webpack') // dd-trace/webpack
22+
const experiments = require('./webpack-experiments')
23+
24+
const OUTFILE = path.join(__dirname, 'openfeature-out.js')
25+
26+
function build () {
27+
return new Promise((resolve, reject) => {
28+
webpack({
29+
mode: 'development',
30+
entry: path.join(__dirname, 'openfeature-app.js'),
31+
target: 'node',
32+
externalsType: 'commonjs',
33+
...(experiments && { experiments }),
34+
output: { filename: path.basename(OUTFILE), path: path.dirname(OUTFILE), hashFunction: 'sha256' },
35+
externals: [
36+
'diagnostics_channel',
37+
'pg', 'mysql2', 'better-sqlite3', 'sqlite3', 'mysql', 'oracledb', 'pg-query-stream', 'tedious',
38+
'@yaacovcr/transform',
39+
'@datadog/native-appsec', '@datadog/native-iast-taint-tracking', '@datadog/native-metrics',
40+
'@datadog/pprof', '@datadog/libdatadog',
41+
],
42+
plugins: [new DatadogWebpackPlugin()],
43+
}, (err, stats) => {
44+
if (err) return reject(err)
45+
if (stats.hasErrors()) return reject(new Error(stats.toString({ errors: true })))
46+
resolve()
47+
})
48+
})
49+
}
50+
51+
async function main () {
52+
try {
53+
await build()
54+
55+
const runOutput = execFileSync(process.execPath, [OUTFILE], { encoding: 'utf8' })
56+
assert(
57+
runOutput.includes('PROVIDER_OK'),
58+
`bundled app did not load a working OpenFeature provider:\n${runOutput}`
59+
)
60+
61+
console.log('ok')
62+
} finally {
63+
fs.rmSync(OUTFILE, { force: true })
64+
}
65+
}
66+
67+
main().catch((error) => {
68+
console.error(error)
69+
process.exitCode = 1
70+
})

integration-tests/webpack/index.spec.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,10 @@ webpackVersions.forEach((version) => {
5858
execSync('node ./build-and-test-skip-external.js', { timeout })
5959
})
6060

61+
it('bundles the vendored OpenFeature provider and bridges the real event emitter', () => {
62+
execSync('node ./build-and-test-openfeature.js', { timeout })
63+
})
64+
6165
it('injects Git metadata into bundled applications', () => {
6266
execSync('node ./build-and-test-git-tags.js', { timeout })
6367
})
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
#!/usr/bin/env node
2+
'use strict'
3+
4+
// Entry for `build-and-test-openfeature.js`. The flagging provider now ships vendored
5+
// inside dd-trace itself (see `vendor/rspack.config.js`), so enabling it through the public
6+
// API must keep working once bundled -- there is no longer an optional peer to lose.
7+
// `@openfeature/server-sdk` is bundled (inlined) here rather than marked external, so this
8+
// also exercises whether the generic bundler instrumentation mechanism still lets dd-trace's
9+
// `openfeature-server-sdk` require-hook observe the app's own inlined require, bridging the
10+
// real `ProviderEvents`/`OpenFeatureEventEmitter` into the vendored provider instead of
11+
// leaving it on the deferred, event-dropping stand-in.
12+
13+
// eslint-disable-next-line import/order
14+
const tracer = require('../../').init({ // dd-trace
15+
experimental: { flaggingProvider: { enabled: true } },
16+
})
17+
18+
const assert = require('assert')
19+
20+
const provider = tracer.openfeature
21+
22+
assert.strictEqual(
23+
provider?.constructor?.name,
24+
'FlaggingProvider',
25+
`expected the real Datadog FlaggingProvider, got ${provider?.constructor?.name}`
26+
)
27+
28+
// Must be required after dd-trace has initialized: dd-trace's require-hook instrumentation
29+
// (`openfeature-server-sdk.js`) only bridges the real emitter into the vendored provider if
30+
// its hook is already installed by the time the app requires `@openfeature/server-sdk`.
31+
const { ProviderEvents } = require('@openfeature/server-sdk')
32+
33+
let receivedDetails
34+
provider.events.addHandler(ProviderEvents.Ready, (details) => {
35+
receivedDetails = details
36+
})
37+
provider.events.emit(ProviderEvents.Ready, { fired: true })
38+
39+
assert.deepStrictEqual(
40+
receivedDetails,
41+
{ fired: true },
42+
'the openfeature-server-sdk instrumentation did not bridge the real emitter into the bundled provider'
43+
)
44+
45+
// eslint-disable-next-line no-console
46+
console.log('PROVIDER_OK')
47+
process.exit(0)

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@
6262
"test:llmobs:sdk:ci": "node scripts/c8-ci.js test:llmobs:sdk",
6363
"test:llmobs:plugins": "mocha \"packages/dd-trace/test/llmobs/plugins/@(${PLUGINS})/*.spec.js\"",
6464
"test:llmobs:plugins:ci": "yarn services && node scripts/c8-ci.js test:llmobs:plugins",
65-
"test:openfeature": "mocha \"packages/dd-trace/test/openfeature/**/*.spec.js\" \"packages/datadog-instrumentations/test/openfeature-server-sdk.spec.{js,mjs}\"",
65+
"test:openfeature": "mocha \"packages/dd-trace/test/openfeature/**/*.spec.js\" \"packages/datadog-instrumentations/test/**/openfeature-server-sdk.spec.js\"",
6666
"test:openfeature:ci": "node scripts/c8-ci.js test:openfeature",
6767
"test:plugins": "node --expose-gc ./node_modules/mocha/bin/mocha.js \"packages/datadog-plugin-@(${PLUGINS})/test/**/${SPEC:-*}*.spec.js\"",
6868
"test:plugins:ci": "yarn services && node scripts/c8-ci.js test:plugins",

packages/datadog-plugin-dd-trace-api/src/index.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,10 @@ module.exports = class DdTraceApiPlugin extends Plugin {
7878
}
7979

8080
// handleEvent('configure')
81+
// No handoff for `openfeature` yet: an application that calls into the tracer
82+
// exclusively through the `dd-trace-api` shim (instead of `require('dd-trace')`) cannot
83+
// reach `tracer.openfeature` through that shim. This does not affect SSI itself --
84+
// auto-injection and vendored-provider resolution work regardless of this gap.
8185
handleEvent('startSpan')
8286
handleEvent('wrap')
8387
handleEvent('trace')

packages/dd-trace/src/proxy.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,14 @@ class Tracer extends NoopProxy {
300300
}
301301

302302
/**
303+
* `tracer.openfeature` is only reachable through this proxy. SSI itself (auto-injecting
304+
* the tracer, resolving the vendored provider regardless of the customer's own
305+
* `node_modules` layout) is unaffected -- that is what this PR fixes. The remaining gap is
306+
* narrower: there is no `dd-trace-api` handoff for `openfeature` yet (see
307+
* `packages/datadog-plugin-dd-trace-api`), so an application that calls into the tracer
308+
* exclusively through the `dd-trace-api` shim -- instead of `require('dd-trace')` -- cannot
309+
* reach the flagging provider through that shim.
310+
*
303311
* @param {import('./config/config-base')} config
304312
*/
305313
#enableOpenfeature (config) {

0 commit comments

Comments
 (0)