[#461] Configure logger sampling policy #495

Merged
alexvanin merged 1 commit from pogpp/frostfs-s3-gw:feature/461_logger_sampling_pcy into master 2024-09-27 06:39:00 +00:00
Member

Closes #461

Closes #461
pogpp added 1 commit 2024-09-20 11:34:35 +00:00
[#461] Configure logger sampling policy
All checks were successful
/ DCO (pull_request) Successful in 36s
/ Vulncheck (pull_request) Successful in 1m14s
/ Builds (pull_request) Successful in 1m16s
/ Lint (pull_request) Successful in 2m17s
/ Tests (pull_request) Successful in 1m26s
94f28ee961
Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
dkirillov reviewed 2024-09-23 07:40:24 +00:00
@ -1067,0 +1077,4 @@
samplingCore := zapcore.NewSamplerWithOptions(
l.Core(),
time.Second,
Member

What about configuring this value (I don't mind current behavior)?
Also we should mention all new params in doc/configuration.md

What about configuring this value (I don't mind current behavior)? Also we should mention all new params in `doc/configuration.md`
dkirillov marked this conversation as resolved
pogpp was assigned by dkirillov 2024-09-23 07:40:31 +00:00
dkirillov reviewed 2024-09-23 12:51:38 +00:00
@ -82,1 +81,3 @@
cfgLoggerDestination = "logger.destination"
cfgLoggerLevel = "logger.level"
cfgLoggerDestination = "logger.destination"
cfgLoggerSamplingInitial = "logger.sampling.initial"
Member

We need flag that allow disable sampling at all

We need flag that allow disable sampling at all
dkirillov marked this conversation as resolved
pogpp force-pushed feature/461_logger_sampling_pcy from 94f28ee961 to 6b5c0c1478 2024-09-23 14:15:50 +00:00 Compare
dkirillov requested review from storage-services-committers 2024-09-24 06:28:59 +00:00
dkirillov requested review from storage-services-developers 2024-09-24 06:28:59 +00:00
pogpp force-pushed feature/461_logger_sampling_pcy from 6b5c0c1478 to 93c7a37a3a 2024-09-24 11:11:43 +00:00 Compare
dkirillov reviewed 2024-09-24 11:56:30 +00:00
@ -787,6 +794,10 @@ func newSettings() *viper.Viper {
// logger:
v.SetDefault(cfgLoggerLevel, "debug")
v.SetDefault(cfgLoggerDestination, "stdout")
v.SetDefault(cfgLoggerSamplingEnabled, true)
Member

Should we disable it by default?

Should we disable it by default?
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-09-24 11:57:49 +00:00
@ -1067,0 +1082,4 @@
// are recorded in a one-second interval. Every other log entry will be dropped within the interval since
// cfgLoggerSamplingThereafter is specified here.
if v.GetBool(cfgLoggerSamplingEnabled) {
Member

As I understand this won't work as expected because we have already built logger wit production config that has enabled sampling

As I understand this won't work as expected because we have already built logger wit production config that has enabled sampling
dkirillov marked this conversation as resolved
alexvanin requested changes 2024-09-24 12:00:18 +00:00
Dismissed
@ -1033,3 +1044,3 @@
return newStdoutLogger(lvl)
return newStdoutLogger(v, lvl)
case destinationJournald:
return newJournaldLogger(lvl)
Owner

Why sampler configuration only affects stdout logger? I think both loggers can use it.

Why sampler configuration only affects stdout logger? I think both loggers can use it.
alexvanin marked this conversation as resolved
@ -1067,0 +1091,4 @@
)
l = zap.New(samplingCore)
}
Owner

Can we set this before calling Build? You can set sampling config in c as it is done in c.Encoding and c.EncoderConfig.

Can we set this before calling Build? You can set sampling config in `c` as it is done in `c.Encoding` and `c.EncoderConfig`.
Author
Member
https://github.com/uber-go/zap/blob/master/config.go#L37
Member

We need rewrite initialization then (e.g. TrueCloudLab/frostfs-s3-lifecycler#21)

We need rewrite initialization then (e.g. https://git.frostfs.info/TrueCloudLab/frostfs-s3-lifecycler/pulls/21)
alexvanin marked this conversation as resolved
@ -58,0 +58,4 @@
sampling:
enabled: true
initial: 100
thereafter: 100
Owner

interval is missing?

`interval` is missing?
alexvanin marked this conversation as resolved
@ -371,2 +371,4 @@
level: debug
destination: stdout
sampling:
initial: 100
Owner

enabled is missing?

`enabled` is missing?
Owner

still missing

still missing
alexvanin marked this conversation as resolved
pogpp force-pushed feature/461_logger_sampling_pcy from 93c7a37a3a to 864b5f26ab 2024-09-25 10:05:33 +00:00 Compare
pogpp force-pushed feature/461_logger_sampling_pcy from 864b5f26ab to 3d3765fe24 2024-09-25 11:46:18 +00:00 Compare
alexvanin reviewed 2024-09-25 13:21:22 +00:00
@ -1077,1 +1098,3 @@
c.Level = zap.NewAtomicLevelAt(lvl)
func newJournaldLogger(v *viper.Viper, lvl zapcore.Level) *Logger {
c := zap.NewProductionEncoderConfig()
c.EncodeTime = zapcore.ISO8601TimeEncoder
Owner

One last suggestion.

Both jouranld and stdout logger require:

  • jsonEncoder
  • level

Can we make separate function to get console encoder and reuse it in both constructors? It will make code similar and much easier to read.

For example:

func consoleEncoder() zapcore.Encoder {
	c := zap.NewProductionEncoderConfig()
	c.EncodeTime = zapcore.ISO8601TimeEncoder
	return zapcore.NewConsoleEncoder(c)
}

func newJournaldLogger(v *viper.Viper, lvl zapcore.Level) *Logger {
	level := zap.NewAtomicLevelAt(lvl)
	consoleEnc := consoleEncoder()

	encoder := zapjournald.NewPartialEncoder(consoleEnc, zapjournald.SyslogFields)
	core := zapjournald.NewCore(level, encoder, &journald.Journal{}, zapjournald.SyslogFields)
	...
}

func newStdoutLogger(v *viper.Viper, lvl zapcore.Level) *Logger {
	level := zap.NewAtomicLevelAt(lvl)
	consoleEnc := consoleEncoder()

	stdout := zapcore.AddSync(os.Stdout)
	consoleOutCore := zapcore.NewCore(consoleEnc, stdout, level)
...
}
One last suggestion. Both jouranld and stdout logger require: * jsonEncoder * level Can we make separate function to get console encoder and reuse it in both constructors? It will make code *similar* and much easier to read. For example: ```go func consoleEncoder() zapcore.Encoder { c := zap.NewProductionEncoderConfig() c.EncodeTime = zapcore.ISO8601TimeEncoder return zapcore.NewConsoleEncoder(c) } func newJournaldLogger(v *viper.Viper, lvl zapcore.Level) *Logger { level := zap.NewAtomicLevelAt(lvl) consoleEnc := consoleEncoder() encoder := zapjournald.NewPartialEncoder(consoleEnc, zapjournald.SyslogFields) core := zapjournald.NewCore(level, encoder, &journald.Journal{}, zapjournald.SyslogFields) ... } func newStdoutLogger(v *viper.Viper, lvl zapcore.Level) *Logger { level := zap.NewAtomicLevelAt(lvl) consoleEnc := consoleEncoder() stdout := zapcore.AddSync(os.Stdout) consoleOutCore := zapcore.NewCore(consoleEnc, stdout, level) ... } ```
alexvanin marked this conversation as resolved
pogpp added 2 commits 2024-09-25 20:13:46 +00:00
[#461] Configure logger sampling policy
All checks were successful
/ DCO (pull_request) Successful in 36s
/ Builds (pull_request) Successful in 1m13s
/ Vulncheck (pull_request) Successful in 1m23s
/ Lint (pull_request) Successful in 2m15s
/ Tests (pull_request) Successful in 1m26s
864b5f26ab
Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
Merge remote-tracking branch 'origin/feature/461_logger_sampling_pcy' into feature/461_logger_sampling_pcy
Some checks failed
/ DCO (pull_request) Failing after 49s
/ Vulncheck (pull_request) Successful in 1m19s
/ Builds (pull_request) Successful in 1m30s
/ Lint (pull_request) Successful in 1m55s
/ Tests (pull_request) Successful in 1m31s
4feb884105
pogpp force-pushed feature/461_logger_sampling_pcy from 4feb884105 to c71873aab8 2024-09-25 20:15:06 +00:00 Compare
alexvanin approved these changes 2024-09-26 06:33:24 +00:00
Dismissed
alexvanin reviewed 2024-09-26 06:35:03 +00:00
@ -371,2 +371,4 @@
level: debug
destination: stdout
sampling:
` enabled: false
Owner

backtick symbol?

backtick symbol?
alexvanin marked this conversation as resolved
pogpp force-pushed feature/461_logger_sampling_pcy from c71873aab8 to b9bc2f59ca 2024-09-26 06:53:39 +00:00 Compare
pogpp dismissed alexvanin's review 2024-09-26 06:53:39 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

dkirillov reviewed 2024-09-26 06:55:31 +00:00
@ -1058,2 +1067,2 @@
c.Encoding = "console"
c.EncoderConfig.EncodeTime = zapcore.ISO8601TimeEncoder
func newStdoutLogger(v *viper.Viper, lvl zapcore.Level) *Logger {
stdout := zapcore.AddSync(os.Stdout)
Member

It seems before we use stderr (from zap production config). Is it ok that we change this?

It seems before we use `stderr` (from zap production config). Is it ok that we change this?
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-09-26 06:58:18 +00:00
@ -1085,3 +1102,4 @@
zapjournald.SyslogPid(),
})
if v.GetBool(cfgLoggerSamplingEnabled) {
Member

Can we hide this condition in separate function? Currently, we have exactly the same 8 lines here and in newStdoutLogger

Can we hide this condition in separate function? Currently, we have exactly the same 8 lines here and in `newStdoutLogger`
dkirillov marked this conversation as resolved
dkirillov reviewed 2024-09-26 06:59:14 +00:00
@ -1096,0 +1125,4 @@
encoder := zapcore.NewConsoleEncoder(c)
return encoder
Member

Maybe just

return zapcore.NewConsoleEncoder(c)
Maybe just ```golang return zapcore.NewConsoleEncoder(c) ```
dkirillov marked this conversation as resolved
pogpp force-pushed feature/461_logger_sampling_pcy from b9bc2f59ca to 277520e19b 2024-09-26 07:20:11 +00:00 Compare
pogpp force-pushed feature/461_logger_sampling_pcy from 277520e19b to 0041203d8f 2024-09-26 07:32:36 +00:00 Compare
pogpp added 1 commit 2024-09-26 07:33:22 +00:00
Merge branch 'master' into feature/461_logger_sampling_pcy
Some checks failed
/ DCO (pull_request) Failing after 1m14s
/ Vulncheck (pull_request) Successful in 1m31s
/ Builds (pull_request) Successful in 2m20s
/ Lint (pull_request) Successful in 2m37s
/ Tests (pull_request) Successful in 2m18s
e26b1db759
pogpp force-pushed feature/461_logger_sampling_pcy from e26b1db759 to 99f273f9af 2024-09-26 07:35:03 +00:00 Compare
dkirillov approved these changes 2024-09-26 08:10:25 +00:00
alexvanin approved these changes 2024-09-27 06:38:34 +00:00
alexvanin merged commit 99f273f9af into master 2024-09-27 06:39:00 +00:00
alexvanin deleted branch feature/461_logger_sampling_pcy 2024-09-27 06:39:05 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-services-developers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-s3-gw#495
No description provided.