WIP: logger: Filter entries by tags provided in config #1619
No reviewers
Labels
No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-node#1619
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "acid-ant/frostfs-node:feature/log-tags"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Signed-off-by: Anton Nikiforov an.nikiforov@yadro.com
3d122022ff
to3fc783c164
WIP: logger: Filter log entries by tags provided in configto WIP: logger: Filter entries by tags provided in config3fc783c164
to32dca3ad29
32dca3ad29
to3982cf3070
3982cf3070
tod60252c150
WIP: logger: Filter entries by tags provided in configto logger: Filter entries by tags provided in configd60252c150
toe17e1f4b9d
logger: Filter entries by tags provided in configto WIP: logger: Filter entries by tags provided in configWhy allowed list but not disallowed list?
@ -42,4 +44,3 @@
if err != nil {
return err
}
logPrm.PrependTimestamp = cfg.GetBool("logger.timestamp")
As it was: a global variable, in which two fields were redefined, and the other fields remained the same.
How it became: a local variable, for which we set two fields.
Looks like this code was lost:
The was done intentionally because on reloading config by SIGHUP this new parameters just set and never used. There are no code which use it after reloading.
@ -17,0 +19,4 @@
at *atomic.Uint32
m uint32
tl map[uint32]zapcore.Level
mtx *sync.RWMutex
Looks like that
mtx *sync.RWMutex
protectsat
&tl
, but these variables change simultaneously. Looks like it could be just one atomic.Thanks, there are no necessity to use
sync.RWMutex
- code updated.@ -0,0 +10,4 @@
const (
BitMain uint8 = iota
BitMorph
Bit
should be an implementation detail, I would rather useTagMorph
Ok, renamed.
@ -381,8 +383,6 @@ type internals struct {
appCfg *config.Config
log *logger.Logger
This part seems unnecessary, why do we move this field to
cfg
?Right, removed these changes.
@ -142,0 +124,4 @@
return nil, err
}
var at atomic.Uint32
at.Store(pat)
Where does
at
name come from?It's a short version of
allowedTags
.Ah, ok.
It looked like a short version of
ATomic
which could've been wrong.@ -180,0 +173,4 @@
func (l *Logger) Reload(prm Prm) error {
l.mtx.Lock()
defer l.mtx.Unlock()
at, tl, err := parseAllowedTags(prm.AllowedTags)
parseAllowedTags
can be invoked without havingl.mtx
taken.Right, rewrote this part with usage of package
atomic
.@ -180,1 +179,4 @@
}
l.lvl.SetLevel(prm.level)
l.at.Store(at)
clear(l.tl)
We have already allocated
tl
inparseAllowedTags
.Why do we do this
clear
and copy?This part updated, please review.
e17e1f4b9d
to8b3cb41d5a
8b3cb41d5a
toc6be5a71f2
WIP: logger: Filter entries by tags provided in configto logger: Filter entries by tags provided in configc6be5a71f2
toe3d2319eb0
e3d2319eb0
toc02f69e3e2
c02f69e3e2
to3d4dcf7d55
@ -42,4 +44,3 @@
if err != nil {
return err
}
logPrm.PrependTimestamp = cfg.GetBool("logger.timestamp")
Where did
PrependTimestamp
part go?Removed because it is never used for logger reloading.
I don't understand, it was in this function before, it surely has to do something, no?
It is used only when we instantiate the new logger here and here. During reloading, we just set it and do nothing.
@ -405,3 +407,3 @@
// stops node's maintenance.
func (c *internals) stopMaintenance(ctx context.Context) {
func (c *cfg) stopMaintenance(ctx context.Context) {
Forgot to revert?
Right, thanks.
@ -1337,2 +1347,3 @@
}
components := c.getComponents(ctx, logPrm)
components := c.getComponents(ctx)
Why don't we put it in
components
as before?Oh, right, reverted. Now it looks much simpler.
@ -0,0 +69,4 @@
}
}
func parseAllowedTags(tags []string) (uint32, map[uint32]zapcore.Level, error) {
Do we have the format described somewhere?
It is hard to understand what this function should do vs what it actually does.
Thanks, I've added doc for the function.
@ -0,0 +77,4 @@
var v uint32
for _, str := range tags {
sep := strings.Index(str, ":")
var tag, level string
This and the following
if
could be replaced withtag, level, _ := strings.Cut(str, ":")
Fixed.
3d4dcf7d55
tof94a8625fa
New commits pushed, approval review dismissed automatically according to repository settings
f94a8625fa
to4b6a8ca12d
4b6a8ca12d
to1c14ab2ee1
1c14ab2ee1
tob41cb386cb
@ -0,0 +9,4 @@
func Benchmark_loggerLog(b *testing.B) {
ctx := context.Background()
num := 10000
Here's from your benchmark results:
As for me this average time is not correct because you don't use
b.N
. I think the shown number of iterations (1e9) is used when calculating the average time, not the actual number you use (1e4).If you want to specify number of iterations, you can use
-benchtime
flag, for example-benchtime=1m
(1 minute) or-benchtime=10000x
(10000 times).Thanks, change a bit benchmark to use
b.N
. Commit messages were updated too.b41cb386cb
to27f11febb9
Minor changes for benchmark.
logger: Filter entries by tags provided in configto WIP: logger: Filter entries by tags provided in config27f11febb9
to09cc4b60dd
New commits pushed, approval review dismissed automatically according to repository settings
09cc4b60dd
to5bf54e18ab
Added new behavior - log level for tag should be independent of the default log level.
WIP: logger: Filter entries by tags provided in configto logger: Filter entries by tags provided in config@ -14,6 +16,9 @@ import (
type Logger struct {
z *zap.Logger
lvl zap.AtomicLevel
at *atomic.Uint32
Please give these variables more meaningful names.
Added comments for each new variable.
5bf54e18ab
tocb1792c69b
cb1792c69b
toa775656620
New commits pushed, approval review dismissed automatically according to repository settings
Minor changes in docs.
@ -30,6 +30,11 @@ func validateConfig(c *config.Config) error {
return fmt.Errorf("invalid logger destination: %w", err)
}
err = logger.ValidateAllowedTags(loggerconfig.AllowedTags(c))
Why have you chosen an approach different from
SetLevelString
andSetDestination
(i.e. "parse, don't validate")?Calling validation is easy to forget, with
SetAllowedTags
it will become mandatory.Agree, added method
SetAllowedTags
.@ -0,0 +25,4 @@
TagGetSvc
TagSearchSvc
TagPolicer
TagReplicator
Could you please put private constant
tagLast
here and add a test that checkstagLast <= 32
?I am not sure go vet catches
uint32(1 << 32)
problem.Nice question.
Looks like it is redundant to check for
tagLast <= 32
, we will fail at runtime.@ -0,0 +7,4 @@
"github.com/stretchr/testify/require"
)
func Benchmark_loggerLog(b *testing.B) {
Could you make this benchmark more fine-grained?
Specifically, we have 2 degrees of freedom:
That gives us 4 different benchmarks, that we would like to compare against each other.
Extended number of benchmarks, divided them by separate functions to have ability to execute them separately in
cli
. In other case, it is impossible to determine in output test result -go
hide name of the benchmark unpredictably.a775656620
toe67f943a6d
e67f943a6d
toeed9e131dc
logger: Filter entries by tags provided in configto WIP: logger: Filter entries by tags provided in configView command line instructions
Checkout
From your project repository, check out a new branch and test the changes.