enchancement(internal_logs_source)!: decouple internal_logs log level from vector startup log level#25776
Conversation
…from vector startup log level
github-actions
Bot
added
docs review on hold
![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: 57bd04dc96
ℹ️ 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".
| // The broadcast channel feeding all `internal_logs` sources is filtered at the most | ||
| // verbose level requested by any such source; events below this source's own level are | ||
| // dropped from its stream in `run`. | ||
| crate::trace::raise_broadcast_max_level(self.level.into()); |
There was a problem hiding this comment.
When internal_logs.level is set to debug or trace, this raise happens only while the source is being built, but tracing is initialized earlier in App::prepare_from_opts before config loading and topology/source construction. Any DEBUG/TRACE events emitted during config load or before this build call are filtered out by the initial INFO broadcast filter and never enter the early buffer, so a level = "trace" source still misses verbose startup logs it is meant to collect. The new tests only emit after start_source_with_config, so they don't cover this startup window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The startup log level is now based on Vector's flags (-v/-q/VECTOR_LOG=), instead of being hard coded to info. There's no solution here that satisfies all cases:
- If we make the default level trace, we unnecessarily process lots of logs during startup, even if there's no
internal_logssource at all. This seems like a pretty steep cost for a very niche win. - For any other static value, a
level = "trace"source will miss out on startup logs - Relying on Vector's flags, as I've chosen to do in the latest commit, means that if you run VECTOR_LOG=off, you won't catch any of the startup logs via the source.
Given these tradeoffs, I'm happy to go with any of these options, but I'd lean towards the last one
| /// subscriber was installed before the source was built. | ||
| pub fn raise_broadcast_max_level(level: LevelFilter) { | ||
| let new = level_filter_to_usize(level); | ||
| let old = BROADCAST_MAX_LEVEL.fetch_max(new, Ordering::SeqCst); |
There was a problem hiding this comment.
Because this uses fetch_max with no matching path to lower or reset the global level, a hot reload that removes a trace/debug internal_logs source or lowers it to info/error leaves the broadcast layer permanently enabled at the old verbose level. From then on, even accepted configurations that no longer request verbose internal logs still pay to enable those callsites, convert events to LogEvent, and send them through the broadcast path until process restart.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I hadn't considered config reloading. This is now fixed.
| static BROADCAST_MAX_LEVEL: AtomicUsize = AtomicUsize::new(DEFAULT_BROADCAST_MAX_LEVEL); | ||
|
|
||
| /// The default broadcast verbosity (3 = `INFO`). | ||
| const DEFAULT_BROADCAST_MAX_LEVEL: usize = 3; |
There was a problem hiding this comment.
The broadcast filter now starts at INFO unconditionally, before the config is known. In runs with VECTOR_LOG=off/--quiet and no internal_logs source, or with only level = "error"/"warn" sources, INFO callsites are still enabled for this layer; during startup they are converted into LogEvents and buffered because the early buffer is present, and after startup each event still enters BroadcastLayer and checks the buffer/sender before being dropped. Previously the broadcast layer used the console filter, so those callsites stayed disabled in these environments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Similar to discussion at #25776 (comment)
1 participant
Summary
This PR should close #12399. It allows individual
internal_logssources to configure their own minimum log level, instead of being tied to Vector's startup config from flags/environment variables.Vector configuration
How did you test this PR?
I ran vector locally against the above config, changing its
level = "debug"throughout, with various VECTOR_LOG levels configured.Change Type
Is this a breaking change?
This is only a breaking change for users who ran vector with a non-default log level (
-q/-v/VECTOR_LOG=) and relied on that log level for configuring theirinternal_logssource(s). Not necessarily a significantly breaking change, but still worth mentioning.Does this PR include user facing changes?
no-changeloglabel to this PR.References
Notes
@vectordotdev/vectorto reach out to us regarding this PR.pre-pushhook, please see this template.make fmtmake check-clippy(if there are failures it's possible some of them can be fixed withmake clippy-fix)make testgit merge origin masterandgit push.Cargo.lock), pleaserun
make build-licensesto regenerate the license inventory and commit the changes (if any). More details on the dd-rust-license-tool.