Enable log injection by default for structured loggers #5859
Conversation
Overall package sizeSelf size: 9.71 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.7.0 | 35.02 MB | 35.02 MB | | @datadog/native-appsec | 8.5.2 | 19.33 MB | 19.34 MB | | @datadog/native-iast-taint-tracking | 4.0.0 | 11.72 MB | 11.73 MB | | @datadog/pprof | 5.8.2 | 9.56 MB | 9.93 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.5.3 | 2.95 MB | 5.6 MB | | @datadog/wasm-js-rewriter | 4.0.1 | 2.85 MB | 3.58 MB | | @datadog/native-metrics | 3.1.1 | 1.02 MB | 1.43 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.14.0 | 120.58 kB | 841.68 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.1 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.2 | 53.63 kB | 53.63 kB | | istanbul-lib-coverage | 3.2.2 | 34.37 kB | 34.37 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | dc-polyfill | 0.1.9 | 25.11 kB | 25.11 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.2 | 23.54 kB | 23.54 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | mutexify | 1.4.0 | 5.71 kB | 8.74 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.4 | 3.96 kB | 3.96 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5859 +/- ##
=======================================
Coverage 80.74% 80.75%
=======================================
Files 462 463 +1
Lines 19918 19926 +8
=======================================
+ Hits 16083 16091 +8
Misses 3835 3835 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Datadog ReportBranch report: ✅ 0 Failed, 1259 Passed, 0 Skipped, 21m 21.15s Total Time |
BenchmarksBenchmark execution time: 2025-06-18 18:33:04 Comparing candidate commit a6086f9 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 1264 metrics, 59 unstable metrics. |
…race-js into ida613/enable-log-injection
ida613
changed the title
Thank you Ruben! I added this commit to check if each message is structured for winston. This might cause some CI failures but you can treat it as a draft for now. Please let me know if you have any feedback! |
|
I've removed winston for now for a separate PR |
|
I've added this test to see what would happen if we try to inject trace and span ids when winston logs simple strings, it seems like the log gets converted to JSON by the log server and the IDs are still injected properly. I'm not sure if this log server mimics something downstream which receives the logs. If so, I guess it doesn't really matter if winston logs are structured or unstructured, everything becomes structured anyways along the way. |
There was a problem hiding this comment.
So after looking into it again, it seems our instrumentation makes sure it is always structured. That in itself is probably theoretically an issue but I am not aware of a bug report about that. It would just fail at an earlier stage, so we do not have to worry about the code here.
In case we use inheritance as suggested here, we have to either make the method public or use the semi public way of using the underscore. I personally would just use composition instead, to prevent the need for that. Inheritance is mostly not easy to follow makes makes things more complicated, not easier.
Should it block this PR? Probably not, I think we have to generally look into it closer soon.
| @@ -174,7 +174,7 @@ describe('test visibility automatic log submission', () => { | |||
| ) | |||
| childProcess.on('exit', () => { | |||
| assert.include(testOutput, 'Hello simple log!') | |||
| assert.notInclude(testOutput, 'span_id') | |||
| assert.include(testOutput, 'span_id') | |||
There was a problem hiding this comment.
The test message is now a bit confusing, since this is actually the part that should verify that the logs are not submitted.
The test uses winston, so that's why it is now automatically enabled and for the test to behave as expected, we'd have to use a different logger instead.
It is the logger being used in automatic-log-submission-test.js. That requires ./logger and that instruments winston. That would have to change.
There was a problem hiding this comment.
if this PR is merged as is, without changes to the test optimization logic, this test would not make sense, so it'd have to be removed
What does this PR do?
Set
config.logInjectiondefault tostructured, in which case log injection is enabled for structured loggers and disabled for unstructured loggers.When
config.logInjection=true, log injection is enabled for both structured and unstructured loggersWhen
config.logInjection=false, log injection is disabled for both structured and unstructured loggersThis change breaks the configuration default, so please let me know if there are any concerns regarding how this change is handled!
Motivation
Plugin Checklist
Additional Notes