test(log-capture): add comprehensive pino log-capture test suites [2/2]#8987
Conversation
|
Overall package sizeSelf size: 6.27 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.1.0 | 101.28 kB | 840.46 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ad467e738
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| assert.ok(record.pid, 'should have pid') | ||
| assert.ok(record.hostname, 'should have hostname') | ||
| assert.ok(record.time, 'should have time') | ||
| assert.ok(record.level !== undefined, 'should have level') |
There was a problem hiding this comment.
This assertion trips the repo's no-restricted-syntax ESLint rule against assert.ok(a !== b); running ESLint on the added test file fails here, so the lint job will block the PR even though the test logic is fine. Use assert.notStrictEqual(record.level, undefined, ...) or another strict assertion for this check.
Useful? React with 👍 / 👎.
BenchmarksBenchmark execution time: 2026-06-22 18:10:55 Comparing candidate commit 4e872dd in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1952 metrics, 13 unstable metrics.
|
1ad467e to
6daf344
Compare
bce6622 to
9d44a89
Compare
6daf344 to
d683d88
Compare
9d44a89 to
23e1b02
Compare
d683d88 to
b52a856
Compare
23e1b02 to
3d12747
Compare
b52a856 to
ef89303
Compare
3d12747 to
ac2f01a
Compare
ef89303 to
200488d
Compare
ac2f01a to
5a27ddb
Compare
200488d to
662cbe6
Compare
5a27ddb to
3c1650c
Compare
285-line test suite split across two files: pino/test/log_capture.spec.js (137 lines): Unit-level tests for the apm:pino:log:json diagnostic channel. Verifies that the channel fires with the correct JSON payload for each log level, that capture-only mode (logInjection: false) does not inject dd fields, and that log capture is a no-op when logCaptureEnabled is false. datadog-instrumentations/test/pino.spec.js (139 lines): Instrumentation-layer integration tests using withVersions. Verifies the JSON channel emits the full serialized record (pid, hostname, level, time, msg) across supported pino releases, and that the channel is silent when the pino instrumentation is not loaded. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
662cbe6 to
4e872dd
Compare
There was a problem hiding this comment.
Pull request overview
Adds an instrumentation-layer test suite to validate the pino log-capture diagnostic channel (apm:pino:log:json) behavior end-to-end through the datadog-instrumentations shimmer hooks, across supported pino versions.
Changes:
- Introduces
pino.spec.jsinstrumentation tests that assert the JSON-line channel fires onlogger.info()and that the serialized record contains expected core fields. - Adds a “no subscribers” scenario intended to validate the
hasSubscribersfast-path guard in the pino instrumentation wrapper.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('should not emit to json channel when there are no subscribers', () => { | ||
| // PinoPlugin is disabled (logInjection=false, logCaptureEnabled=false), so the only | ||
| // way hasSubscribers can be true is if external code subscribed — there is none here. | ||
| assert.strictEqual(jsonCh.hasSubscribers, false) | ||
|
|
||
| logger.info('no subscriber test') | ||
| }) |
| setImmediate(() => { | ||
| assert.ok(captured, 'json channel should have fired') | ||
| const record = JSON.parse(captured.line) | ||
| assert.strictEqual(record.msg, 'capture test') | ||
| done() | ||
| }) |
| setImmediate(() => { | ||
| assert.ok(captured, 'json channel should have fired') | ||
| const record = JSON.parse(captured.line) | ||
| assert.ok(record.pid, 'should have pid') | ||
| assert.ok(record.hostname, 'should have hostname') | ||
| assert.ok(record.time, 'should have time') | ||
| assert.ok(record.level !== undefined, 'should have level') | ||
| assert.strictEqual(record.msg, 'full record test') | ||
| done() | ||
| }) |
| setImmediate(() => { | ||
| assert.ok(captured, 'json channel should have fired') | ||
| const record = JSON.parse(captured.line) | ||
| assert.strictEqual(record.msg, 'with extra field') | ||
| assert.strictEqual(record.extra, 'field') | ||
| done() | ||
| }) |
|
The bot concerns are valid. I'll set this to draft, feel free to undraft once the changes have been made. |
What does this PR do?
Adds two dedicated test suites that together exercise the full log-capture path through the pino plugin introduced in #8986.
Motivation
The pino plugin's log-capture hook operates at the serialization boundary — it intercepts the already-serialised JSON string before it reaches the output stream. The basic smoke tests in #8986 confirm the channel fires, but deeper behavioral questions need their own focused suites:
pino/test/log_capture.spec.js(unit-level, 137 lines): Does the channel emit the right payload for every log level? Does capture-only mode (logInjection: false) leave the record unmodified? Is the channel silent whenlogCaptureEnabledis false? These tests run without a real pino install — they drive the plugin's channel subscription directly.datadog-instrumentations/test/pino.spec.js(instrumentation-layer, 139 lines): Does the channel fire through the full shimmer/instrumentation stack across supported pino versions (viawithVersions)? Does it carry all expected fields —pid,hostname,level,time,msg? Is it silent when the instrumentation is not loaded? These tests catch regressions that unit tests cannot, because the shimmer transforms the pino write path before the plugin's hook runs.Additional Notes
This is PR 2 of 2 splitting the original #8966:
Stacks on #8986. No production code changes — this PR is purely tests.
Test plan
PLUGINS=pino ./node_modules/.bin/mocha packages/datadog-plugin-pino/test/log_capture.spec.jsPLUGINS=pino ./node_modules/.bin/mocha packages/datadog-instrumentations/test/pino.spec.js🤖 Generated with Claude Code