Skip to content

Avoid expensive log.Valuer evaluation for disallowed level#5297

Merged
alvinlin123 merged 2 commits intocortexproject:masterfrom
damnever:perf/log
May 10, 2023
Merged

Avoid expensive log.Valuer evaluation for disallowed level#5297
alvinlin123 merged 2 commits intocortexproject:masterfrom
damnever:perf/log

Conversation

@damnever
Copy link
Contributor

@damnever damnever commented Apr 28, 2023 •
edited
Loading

A lot of failed operations in memcach_client.SetAsync will consume a lot of CPU:
image

goos: darwin
goarch: arm64
pkg: github.com/cortexproject/cortex/pkg/util/log
                      │   old.txt    │               new.txt               │
                      │    sec/op    │   sec/op     vs base                │
DisallowedLogLevel-10   1594.5n ± 6%   418.2n ± 1%  -73.77% (p=0.000 n=20)

                      │   old.txt   │              new.txt               │
                      │    B/op     │    B/op     vs base                │
DisallowedLogLevel-10   1567.0 ± 0%   623.0 ± 0%  -60.24% (p=0.000 n=20)

                      │  old.txt   │              new.txt               │
                      │ allocs/op  │ allocs/op   vs base                │
DisallowedLogLevel-10   23.00 ± 0%   13.00 ± 0%  -43.48% (p=0.000 n=20)

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@damnever damnever force-pushed the perf/log branch 2 times, most recently from a77c82f to 95975ad Compare April 28, 2023 02:53
@damnever damnever mentioned this pull request Apr 28, 2023
Merged
2 tasks
@damnever damnever force-pushed the perf/log branch 3 times, most recently from 19b2fbc to f6cfaea Compare April 28, 2023 06:17
alanprot
alanprot reviewed May 3, 2023
View reviewed changes
pkg/util/log/log.go
@@ -72,24 +69,32 @@ type PrometheusLogger struct {
// NewPrometheusLogger creates a new instance of PrometheusLogger which exposes
// Prometheus counters for various log levels.
func NewPrometheusLogger(l logging.Level, format logging.Format) (log.Logger, error) {
Copy link
Member

@alanprot alanprot May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not being used anymore?

Copy link
Contributor Author

@damnever damnever May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used by other packages.

@alanprot
Copy link
Member

alanprot commented May 3, 2023

Can you explain what is the functional change here? are we missing/changing something?

@damnever
Copy link
Contributor Author

damnever commented May 4, 2023 •
edited
Loading

@alanprot I agree, the change is simple but not easy to understand, the old code is like this:

logger0 := NewXXXLogger(...)
logger1 := level.NewFilter(logger0, lvl)
logger2 := log.With(logger1, "ts", log.DefaultTimestampUTC, "caller", log.Caller(3))

the new code is like this:

logger0 := NewXXXLogger(...)
logger1 := log.With(logger0, "ts", log.DefaultTimestampUTC, "caller", log.Caller(5))
logger2 := level.NewFilter(logger1, lvl)

the order matters since the logger2.Log does level filtering first then call the logger1.Log on demand, the logger1.Log evaluates the log.Valuer such as log.Caller..

I have add the reference link in the comments: go-kit/log#14 (comment)

@damnever
Avoid expensive log.Valuer evaluation for disallowed levels
247f8be 
Signed-off-by: Xiaochao Dong (@damnever) <the.xcdong@gmail.com>
@damnever damnever requested a review from alanprot May 9, 2023 02:27
@alanprot
Copy link
Member

alanprot commented May 9, 2023

NIce! Ok..

@damnever
Merge branch 'master' into perf/log
1ad7767 
Signed-off-by: Xiaochao Dong <the.xcdong@gmail.com>
alvinlin123
alvinlin123 approved these changes May 10, 2023
View reviewed changes
Copy link
Contributor

@alvinlin123 alvinlin123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an awesome enhancement, thank you so much!

@alvinlin123 alvinlin123 merged commit 2e6e4ae into cortexproject:master May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alanprot alanprot alanprot approved these changes

+1 more reviewer

@alvinlin123 alvinlin123 alvinlin123 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@damnever @alanprot @alvinlin123