feat(logging): add hook-based log capture with HTTP forwarding#7674
Open
litianningdatadog wants to merge 2 commits intomasterfrom
Open
feat(logging): add hook-based log capture with HTTP forwarding#7674litianningdatadog wants to merge 2 commits intomasterfrom
litianningdatadog wants to merge 2 commits intomasterfrom
Conversation
litianningdatadog
changed the title
Contributor
Overall package sizeSelf size: 5.57 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.0.1 | 82.56 kB | 817.39 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
|
✨ Fix all issues with BitsAI or with Cursor
|
BenchmarksBenchmark execution time: 2026-04-23 16:07:29 Comparing candidate commit 08adc7e in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1345 metrics, 99 unstable metrics. |
litianningdatadog
force-pushed
the
tianning.li/log-capture-hook-approach
branch
from
1d185ce to
7a9d4e8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7674 +/- ##
==========================================
+ Coverage 73.67% 73.76% +0.09%
==========================================
Files 775 776 +1
Lines 36215 36326 +111
==========================================
+ Hits 26680 26796 +116
+ Misses 9535 9530 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
litianningdatadog
force-pushed
the
tianning.li/log-capture-hook-approach
branch
3 times, most recently
from
f2cd92c to
2d4a829
Compare
litianningdatadog
force-pushed
the
tianning.li/log-capture-hook-approach
branch
6 times, most recently
from
e039ca3 to
945de7f
Compare
litianningdatadog
force-pushed
the
tianning.li/log-capture-hook-approach
branch
3 times, most recently
from
aa04e8e to
7f7948e
Compare
litianningdatadog
force-pushed
the
tianning.li/log-capture-hook-approach
branch
3 times, most recently
from
ad7ba6c to
8ff09a9
Compare
litianningdatadog
force-pushed
the
tianning.li/log-capture-hook-approach
branch
2 times, most recently
from
9aa259f to
791d03b
Compare
rochdev
requested changes
Apr 7, 2026
litianningdatadog
force-pushed
the
tianning.li/log-capture-hook-approach
branch
from
791d03b to
23b81c2
Compare
litianningdatadog
changed the title
litianningdatadog
force-pushed
the
tianning.li/log-capture-hook-approach
branch
7 times, most recently
from
640df93 to
039cfd0
Compare
rochdev
reviewed
Apr 10, 2026
Member
rochdev
left a comment
rochdev
left a comment
There was a problem hiding this comment.
One last small thing and then I think this should be good to go! I would like to see an integration test for this as well with a mock mini agent, but that can be added as a follow up PR.
litianningdatadog
force-pushed
the
tianning.li/log-capture-hook-approach
branch
2 times, most recently
from
c944ddb to
644a65e
Compare
litianningdatadog
force-pushed
the
tianning.li/log-capture-hook-approach
branch
from
644a65e to
b923d1a
Compare
rochdev
requested changes
Apr 14, 2026
litianningdatadog
force-pushed
the
tianning.li/log-capture-hook-approach
branch
from
b923d1a to
f34da68
Compare
rochdev
requested changes
Apr 14, 2026
| const { storage } = require('../../../datadog-core') | ||
|
|
||
| describe('Subscription', () => { |
Member
There was a problem hiding this comment.
I think this is only needed because Subscription was used directly. Instead, this.addSub() should be used.
| } | ||
|
|
||
| module.exports = Plugin | ||
| module.exports.Subscription = Subscription |
Member
There was a problem hiding this comment.
This should not be needed, all subscriptions should be done through addSub.
| try { | ||
| // Always include dd trace context in captured records even if logInjection is off. | ||
| const msg = this.config.logInjection ? arg.message : messageProxy(arg.message, holder) | ||
| require('../log-capture/sender').add(JSON.stringify(msg)) |
Member
There was a problem hiding this comment.
Why require here instead of at the top of the file? This could be a hot path and it's better to avoid unnecessary abstractions.
Comment on lines
+49
to
+50
| * Override to prevent the generic apm:${id}:log capture path from running for pino. | ||
| * Pino capture is handled via the apm:pino:json channel in #jsonCaptureSub. |
Member
There was a problem hiding this comment.
What about cases where json is not used at all? Do we just skip these logs entirely? If we do, then that implementation seems incomplete? And if we don't, then why even bother with log? We could just use json for everything at that point.
Contributor
Author
There was a problem hiding this comment.
Good question. apm:pino:json actually fires for all standard pino output, including pino-pretty, because pino always serializes internally via asJson before writing to any estination. So no logs are silently dropped. The reason we can't use apm:pino:log instead is that for pino ≥5.14 it fires from the mixin hook, which only carries partial data (the mixin fields, not the full log record). apm:pino:json fires from the asJson wrapper and always has the complete record, which is why we use it exclusively for capture.
| if (!config.logCaptureEnabled) return | ||
|
|
||
| const captureSender = require('../log-capture/sender') | ||
| captureSender.configure({ |
Member
There was a problem hiding this comment.
This should happen outside of plugins. Right now the tracer gets these options, passes them to the plugins so that the plugins can then go back to the tracer to configure it. This not only requires unnecessary code but it also technically broken because it means 2 plugins could be configured differently and then the last one wins which is broken.
litianningdatadog
force-pushed
the
tianning.li/log-capture-hook-approach
branch
4 times, most recently
from
4cd1f62 to
4772168
Compare
Captures JSON log records from Winston, Bunyan, and Pino via diagnostic
channels and forwards them as NDJSON to a configurable HTTP(S) intake
without requiring transports or stream shims in user code.
Architecture:
- LogPlugin base subscribes to apm:${id}:log for winston/bunyan capture
- PinoPlugin overrides _captureEnabled to suppress the generic path and
instead subscribes to apm:pino:json for fully-serialized records
- Sender uses a one-shot setTimeout (armed only when records are buffered)
- PINO_JSON_CHANNEL is defined once in the instrumentation and imported
by PinoPlugin to avoid duplication
Config options (all with DD_ env var equivalents):
- logCaptureEnabled (default: true in Lambda Lite, false elsewhere)
- logCaptureHost / logCapturePort (default: localhost:10517)
- logCapturePath / logCaptureProtocol / logCaptureFlushIntervalMs
- logCaptureMaxBufferSize / logCaptureTimeoutMs
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
litianningdatadog
force-pushed
the
tianning.li/log-capture-hook-approach
branch
from
4772168 to
f37288f
Compare
Resolves conflicts: - .github/workflows/platform.yml: accept master's version (PR #8039 moved instrumentation jobs into .github/workflows/instrumentation.yml) - .github/workflows/instrumentation.yml: re-add instrumentation-pino job at the new location - regenerate packages/dd-trace/src/config/generated-config-types.d.ts
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Background:
https://datadoghq.atlassian.net/browse/SVLS-8572
What does this PR do?
Introduces a new log capture feature that forwards log records from Winston, Bunyan, and Pino to a configurable HTTP/HTTPS intake endpoint, without requiring per-instance transport injection.
How it works:
apm:<logger>:logdiagnostic channels (already firing for trace injection) inside the genericLogPluginbase classapm:pino:jsonchannel that publishes the complete serialized JSON line fromasJsonSymfor all Pino versions, bypassing the partial mixin data limitation of ≥5.14. This subscription lives inPinoPlugin(notLogPlugin) to keep the generic base free of pino-specific knowledgelog-capture/sendermodule manages a batched NDJSON buffer with a one-shotsetTimeoutflush (armed only when records are buffered) and configurable max buffer size and HTTP/HTTPS transportArchitecture:
LogPlugin._captureEnabledis a protected getter (returns!!config.logCaptureEnabled) thatPinoPluginoverrides to returnfalse, suppressing the generic capture path for pino while leaving trace injection intactLogPlugin._configureSender(config)is a protected method that configures the shared sender; it is a no-op whenlogCaptureEnabledis false — the sender goes dormant naturally once no more records are addedPINO_JSON_CHANNELis defined once in the pino instrumentation (the publisher) and imported byPinoPlugin(the subscriber)Configuration (env / init options):
Motivation
Alternative to the transport-injection approach (PR #7522). By hooking into channels that already fire on every log write, this avoids per-instance injection, WeakSet de-duplication, and the duplicate-injection issues observed with Winston's
createLogger.Additional Notes
Key design decisions:
logInjectionandlogCaptureEnabledare independent — enabling only capture does not injectddinto the actual log output, butddtrace context is always included in the captured JSON sent to the intakemixinSym(used for trace injection) only provides partial mixin fields. A separatewrapAsJsonForCapturewrapper onasJsonSymcaptures the complete JSON line post-serialization and publishes toapm:pino:jsonsetTimeout(re-armed only when records are buffered) rather thansetInterval, avoiding idle event-loop wakeups. There is nostop()— when capture is disabled, records simply stop being added and the sender goes dormantflushis registered inbeforeExitHandlersonce when first configured and never removed, ensuring buffered records are flushed on process exit regardless of runtime config changes\nfrom each record before joining (Pino'sasJsonappends\n; naïve join would produce double blank lines)res.resume()to release sockets back to the poollogCapture*config fields are always forwarded throughplugin_manager._getSharedConfig()(no conditional guard)Performance: Pino overhead is ~200–400 ns/write (no extra serialization). Winston/Bunyan overhead is ~3–25 µs/write dominated by
JSON.stringify.