Fix journald logger #1674

dstepanov-yadro · 2025-03-13T07:45:04Z

dstepanov-yadro commented

2025-03-13 07:45:04 +00:00

lvl and encoding are redundant: lvl used directly for zap.Core creation and console encoder created directly by calling zapcore.NewConsoleEncoder
journald logger did not use sampling as console logger. Console logger sampling creates by config.Build call as follows: 6d482535bd/config.go (L283)

1. lvl and encoding are redundant: lvl used directly for zap.Core creation and console encoder created directly by calling `zapcore.NewConsoleEncoder` 2. journald logger did not use sampling as console logger. Console logger sampling creates by `config.Build` call as follows: https://github.com/uber-go/zap/blob/6d482535bdd97f4d97b2f9573ac308f1cf9b574e/config.go#L283 Fix #1673

dstepanov-yadro added 2 commits 2025-03-13 07:45:05 +00:00

[#1673 ] logger: Drop unused fields a13e12e3de

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>

[#1673 ] logger: Add sampling for journald logger

DCO action / DCO (pull_request) Successful in 30s

Details

Vulncheck / Vulncheck (pull_request) Successful in 1m5s

Details

Build / Build Components (pull_request) Successful in 1m26s

Details

Pre-commit hooks / Pre-commit (pull_request) Successful in 1m29s

Details

Tests and linters / Run gofumpt (pull_request) Successful in 2m23s

Details

Tests and linters / Lint (pull_request) Successful in 2m46s

Details

Tests and linters / Staticcheck (pull_request) Successful in 2m47s

Details

Tests and linters / Tests (pull_request) Successful in 3m1s

Details

Tests and linters / Tests with -race (pull_request) Successful in 3m40s

Details

Tests and linters / gopls check (pull_request) Successful in 3m47s

Details

e59b426d39

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>

fyrchik approved these changes 2025-03-13 07:59:00 +00:00

pkg/util/logger/logger.go Outdated

					
				@ -172,0 +171,4 @@

					if c.Sampling.Hook != nil {

						samplerOpts = append(samplerOpts, zapcore.SamplerHook(c.Sampling.Hook))

					}

					samplingCore := zapcore.NewSamplerWithOptions(

fyrchik commented

2025-03-13 07:58:57 +00:00

What if c.Sampling.Hook == nil, do we need this wrapper?

What if `c.Sampling.Hook == nil`, do we need this wrapper?

dstepanov-yadro commented

2025-03-13 08:03:53 +00:00

Yes.
Console logger uses zap's production config: 6d482535bd/config.go (L161) . Production config defines, that zap will log only first 100 log entries for each level for 1 second. If there are more than 100 log entries in 1 second for level, then zap will log only 100-th log entries. For example, if there are 300 log entries for level warning, then only 0...99, 100, 200, 300 log entries will be logged.
Sampling hook only notifies if log entry was sampled or dropped.

Yes. Console logger uses `zap`'s production config: https://github.com/uber-go/zap/blob/6d482535bdd97f4d97b2f9573ac308f1cf9b574e/config.go#L161 . Production config defines, that zap will log only first 100 log entries for each level for 1 second. If there are more than 100 log entries in 1 second for level, then zap will log only 100-th log entries. For example, if there are 300 log entries for level `warning`, then only 0...99, 100, 200, 300 log entries will be logged. Sampling hook only notifies if log entry was sampled or dropped.

fyrchik commented

2025-03-13 08:24:38 +00:00

Hm, it was unexpected to me that we have this enabled by default.

fyrchik marked this conversation as resolved

fyrchik referenced this pull request

2025-03-13 08:24:56 +00:00

Disable log sampling by default #1675

fyrchik added this to the v0.45.0 milestone 2025-03-13 08:42:39 +00:00

fyrchik commented

2025-03-13 08:43:03 +00:00

Please, mention Fix #1673 in the OP.

Please, mention `Fix #1673` in the OP.