feat(log): route the tracer's own diagnostics through tracing - #281
feat(log): route the tracer's own diagnostics through tracing#281iunanua wants to merge 3 commits into
Conversation
f81e4fd to
d83a7e4
Compare
🎉 All green!🧪 All tests passed 🔗 Commit SHA: 41d282b | Docs | Datadog PR Page | Give us feedback! |
The dd_debug!/dd_info!/dd_warn!/dd_error! macros wrote straight to stdout and stderr, which an application cannot capture, format or redirect. print_log now hands the message to `tracing` under the `datadog_opentelemetry` target when the application has installed a subscriber, and prints only when none exists, so a bare binary still sees its diagnostics. DD_LOG_LEVEL keeps deciding which diagnostics are emitted at all, independently of where they then go. Errors are still forwarded to instrumentation telemetry unconditionally: that must not depend on how the application routes logs. tracing's `log` feature is deliberately not enabled. Cargo features are additive, so switching it on here would turn `log` emission on for every tracing-instrumented crate in the application's graph; an application that wants that fallthrough can enable it in its own manifest. Trace-export error logging now runs inside an OpenTelemetry telemetry-suppressed scope. The SDK enters one around its own exporters, but Datadog span export runs on libdatadog's worker, so a bridged log pipeline could otherwise turn a failed export into more records to export. Two incidental fixes. dd_log!'s single-argument arm expanded to statements with a trailing semicolon, which is a future-incompatibility error in expression position. And test_default_max_level asserted a process-global value that ConfigBuilder::build writes, so it now asserts the default itself while test_max_level continues to cover the set/read round-trip. The propagator example configures the layers this implies and installs the subscriber before the tracer, so start-up diagnostics are routed rather than printed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
d83a7e4 to
e7f6e2d
Compare
`has_been_set` reports whether a dispatcher was ever installed anywhere in the process, and it is never cleared. It therefore stayed true after a scoped `with_default` scope exited, and on threads that never had a subscriber at all — including the tracer's own workers, where much of its diagnostics come from. Those events were handed to tracing's no-op dispatcher and silently lost, which is exactly what the printed fallback exists to prevent. Ask the current dispatcher what it is instead. A subscriber that is installed but filters this target out is still respected: that is the application's decision rather than an absent destination. `emit` now reports which destination it used, so the tests can assert on the routing decision without capturing the process's output streams. Three of the four new tests fail against the previous implementation: the predicate after a scoped subscriber exits, emission after it exits, and emission from another thread while one is in scope. The fourth pins the opposite direction — a subscriber that filters the event must not trigger the fallback. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7180133 to
5db4d4a
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5db4d4ac51
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Two more ways a subscriber can exist while the diagnostic still reaches nobody, both of which left `DD_LOG_LEVEL` ineffective: OpenTelemetry telemetry suppression. `SdkLogger::event_enabled` returns false while it is active, so an OpenTelemetry log bridge drops the event — and a bridge is often the only layer an application installs. Trace-export diagnostics are emitted inside a suppressed scope precisely so they cannot become telemetry, so in that setup export failures went completely silent. A compile-time level ceiling. Applications can enable tracing's `max_level_*` or `release_max_level_*` features, and because cargo features are additive that compiles our own callsites out. Nothing at runtime can bring them back, so a `release_max_level_warn` build lost every diagnostic below warn even with `DD_LOG_LEVEL=debug`. The rule is now explicit: fall back to the output streams when tracing is *incapable* of carrying the diagnostic — no subscriber, suppression active, or a ceiling below its level — and respect tracing when it is capable but chooses not to deliver, as with a target directive or a subscriber's own level hint. Printing over an application's filtering decision would be worse than staying quiet. The ceiling comparison takes the ceiling as an argument so it can be tested against values this build does not have. `every_level_reaches_the_host_subscriber` gained a precondition assert, since it assumes a TRACE ceiling. `export_errors_are_logged_with_opentelemetry_suppression_active` asserted the old contract — that the suppressed diagnostic reached the subscriber — so it is now `export_errors_are_not_handed_to_tracing`, covering the property that matters: a suppressed export-path diagnostic must not reach a bridged log pipeline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
What
dd_debug!/dd_info!/dd_warn!/dd_error!wrote straight to stdout and stderr, which an application cannot capture, format or redirect. They now go totracingunder thedatadog_opentelemetrytarget, and are printed only whentracingcannot deliver them — so nothing goes silent.DD_LOG_LEVELstill decides which diagnostics exist at all; the subscriber's filters decide where they go. For an app that already has a filtered subscriber, that meansDD_LOG_LEVEL=debugplus a filter admittingdatadog_opentelemetry=debug.Routing rule
Fall back to the streams when
tracingis incapable of carrying the diagnostic:dispatcher::has_been_set()cannot answer this — it stays true after a scopedwith_defaultexits and on threads that never had one, including the tracer's own workers.SdkLogger::event_enabledreturns false, so a log bridge — often the only layer installed — drops the event.tracing/release_max_level_warn, and additive features compile our callsites out; no runtime check can rescue them.Respect
tracingwhen it is capable but chooses not to deliver — a target directive, or a subscriber's own level hint. Printing over the application's filtering would be worse than staying quiet.Decisions
internal-logs, and runtime detection that also consultedlog::max_level(). Patches on request.logfeature not enabled. Additive features would turnlogemission on for every tracing-instrumented crate in the application's graph; an app that wants the fallthrough can enable it itself. Matchesopentelemetry-rust.LOG_TARGET = env!("CARGO_CRATE_NAME")— the underscored crate name, no literal.module_path!()would namecore::lograther than the emitting code, the same reasonfileandlinetravel as fields.!Send, so it wraps the synchronous logging rather than theasyncexport.🤖 Generated with Claude Code