You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "mrproliu (via GitHub)" <gi...@apache.org> on 2026/09/09 03:32:48 UTC
[PR] Pre setting logging level before flags parse [skywalking-banyandb]
mrproliu opened a new pull request, #1337:
URL: https://github.com/apache/skywalking-banyandb/pull/1337
## Current behavior
A deployment that asks for `error` still gets a burst of `debug`, `info` and `warn` on every start. On a cluster running with `BYDB_LOGGING_LEVEL=error` correctly set on all 17 containers, every BanyanDB container still emitted low-level lines, and all of them landed inside the very first second of the process:
```
container all levels seen within the first second
liaison info 1, debug 33, warn 2, error 22 info 1, debug 33, warn 2
data info 1, debug 33, warn 2, error 1 info 1, debug 33, warn 2
fodc-agent info 1, error 12 info 1
lifecycle info 1, debug 3, error 1 info 1, debug 3
backup info 1 info 1
fodc-proxy info 1, error 121 info 1
```
After that first second only `error` came through, so the level was in fact applied -- just not to anything logged before it took effect. The 33 `register flags` lines and the `maxprocs` line are the bulk of it, and no combination of flags or environment variables could silence them.
## Why it happens
`logger.Init` runs in the root command's `PersistentPreRunE`, i.e. when the command *executes*. Two things log earlier than that:
- `pkg/run/run.go:150` takes a logger with `logger.GetLogger(g.name)` and `run.go:244` writes `register flags` while the command tree is being *built*, which happens in `NewRoot` before any `PreRun` hook.
- `pkg/cgroups` resolves `GOMAXPROCS` in a package-level `init()`, so it logs before `main` even starts.
`GetLogger` on an uninitialized root falls through to `rootLogger.setDefault()`, and that function hard-coded `Level: "debug"`. Everything above therefore logged at `debug` unconditionally.
## The fix
`setDefault` now resolves its configuration from the two sources that *do* exist that early: the raw command line and the environment.
- `--logging-level` / `--logging-env` are read straight out of `os.Args` with a throwaway `pflag.FlagSet` that has `ParseErrorsAllowlist.UnknownFlags` set, so the real command's flags and the subcommand name are ignored while the parsing of the two flags stays identical to what cobra does later. No hand-rolled string matching, and both `--logging-level=error` and `--logging-level error` work.
- `BYDB_LOGGING_LEVEL` / `BYDB_LOGGING_ENV` are read directly, since `pkg/config` has not run yet.
- A flag wins over the environment, matching the order `config.Load` applies afterwards.
- An unparsable level is ignored and the fallback stays `debug`, rather than being fatal: `setDefault` panics on a logger error, and a typo in a level should not kill the process at its earliest moment with no usable message. `Init` still reports it properly a moment later.
- With nothing set, the configuration is unchanged (`prod` / `debug`).
`--logging-modules` and `--logging-levels` are deliberately *not* applied this early. They must have equal lengths or `getLogger` fails, and a failure here reaches the `panic` in `setDefault`; trading a crash-on-typo for filtering a few dozen startup lines is not worth it. They keep working normally once `Init` runs.
## Verification
Unit tests (`pkg/logger/setting_test.go`, 13 cases) cover both flag spellings, flag-over-environment precedence, unknown flags mixed in, unparsable and empty values, and a trailing flag with no value.
Running `banyand standalone` for 12s on its own ports and data directory:
| configuration | total output | log lines |
|---|---|---|
| nothing set | 137 | **130** (info 66 / debug 33 / warn 30) |
| `--logging-level=error` | 7 | **0** |
| `BYDB_LOGGING_LEVEL=error` | 7 | **0** |
All three runs wrote the same number of files into their data directories, confirming the service started normally rather than failing silently.
The 7 remaining lines are the ASCII banner (`fmt.Print` in `pkg/cmdsetup/root.go`) and the `terminated signal received` line (`fmt.Fprintf` in `pkg/signal/handler.go:62`). Neither goes through the logger, so no level affects them; both are printed once.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Fixes apache/skywalking#<issue number>.
- [ ] Update the [`CHANGES` log](https://github.com/apache/skywalking-banyandb/blob/main/CHANGES.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Pre setting logging level before flags parse [skywalking-banyandb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1337:
URL: https://github.com/apache/skywalking-banyandb/pull/1337#discussion_r3964399153
##########
banyand/metadata/discovery/dns/dns.go:
##########
@@ -321,7 +321,7 @@ func (s *Service) discoveryLoop(ctx context.Context) {
for {
if err := s.queryDNSAndUpdateNodes(ctx); err != nil {
- s.GetLogger().Err(err).Msg("failed to query DNS and update nodes")
+ s.GetLogger().Warn().Err(err).Msg("failed to query DNS and update nodes")
Review Comment:
This downgrade makes complete DNS discovery failures (and node-cache update failures returned by `queryDNSAndUpdateNodes`) invisible when operators configure the `error` level—the exact deployment mode targeted by this PR. These are runtime availability failures, not early startup noise, so preserve the existing error severity; otherwise diagnosis depends on metrics being available.
##########
pkg/logger/setting.go:
##########
@@ -53,16 +61,66 @@ func (rl *rootLogger) setDefault() {
if rl.done == 0 {
defer atomic.StoreUint32(&rl.done, 1)
var err error
- rl.l, err = getLogger(Logging{
- Env: "prod",
- Level: "debug",
- })
+ rl.l, err = getLogger(defaultLogging())
if err != nil {
panic(err)
}
}
}
+// defaultLogging returns the configuration the root logger falls back to until Init runs.
+func defaultLogging() Logging {
+ return earlyLogging(os.Args[1:], os.Getenv)
+}
+
+// earlyLogging resolves the logging configuration from the sources available before the
+// command tree exists, so that the lines emitted while it is built follow the level the
+// operator asked for. Flags win over the environment, the order config.Load applies later.
+// An unusable level is ignored rather than fatal here, because Init reports it with a
+// proper error once it runs.
+func earlyLogging(args []string, getenv func(string) string) Logging {
+ cfg := Logging{Env: "prod", Level: "debug"}
+ if env := getenv(envLoggingEnv); env != "" {
+ cfg.Env = env
+ }
+ cfg.Level = acceptLevel(cfg.Level, getenv(envLoggingLevel))
+ flagEnv, flagLevel := loggingFlagsFromArgs(args)
+ if flagEnv != "" {
+ cfg.Env = flagEnv
Review Comment:
An explicitly empty `--logging-env=` does not win over `BYDB_LOGGING_ENV`: `loggingFlagsFromArgs` returns only the value, so this condition treats “flag present with an empty value” as “flag absent” and retains the environment value. Cobra/pflag marks the real flag as changed, and `config.BindFlags` therefore does not apply the environment value; `Init` also explicitly accepts an empty environment. Return the flag's presence (`fs.Changed("logging-env")`) separately and apply the flag whenever it was provided, including an empty value; add this precedence case to the table test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Pre setting logging level before flags parse [skywalking-banyandb]
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily merged PR #1337:
URL: https://github.com/apache/skywalking-banyandb/pull/1337
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Pre setting logging level before flags parse [skywalking-banyandb]
Posted by "mrproliu (via GitHub)" <gi...@apache.org>.
mrproliu commented on code in PR #1337:
URL: https://github.com/apache/skywalking-banyandb/pull/1337#discussion_r3965324680
##########
banyand/metadata/discovery/dns/dns.go:
##########
@@ -321,7 +321,7 @@ func (s *Service) discoveryLoop(ctx context.Context) {
for {
if err := s.queryDNSAndUpdateNodes(ctx); err != nil {
- s.GetLogger().Err(err).Msg("failed to query DNS and update nodes")
+ s.GetLogger().Warn().Err(err).Msg("failed to query DNS and update nodes")
Review Comment:
Add a retry strategy to the query DNS log.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org