Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: can't show debug level logging message #1808

Merged
merged 2 commits into from
Aug 22, 2023

Conversation

zhaohuabing
Copy link
Member

What type of PR is this?

Fix: EG can't show debug level logging message because the sugar logger created in the WithName function is incorrect.

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@zhaohuabing zhaohuabing requested a review from a team as a code owner August 21, 2023 10:03
@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #1808 (fcf6399) into main (c5c675f) will increase coverage by 0.19%.
Report is 1 commits behind head on main.
The diff coverage is 80.22%.

@@            Coverage Diff             @@
##             main    #1808      +/-   ##
==========================================
+ Coverage   65.05%   65.24%   +0.19%     
==========================================
  Files          86       86              
  Lines       12281    12316      +35     
==========================================
+ Hits         7989     8036      +47     
+ Misses       3777     3769       -8     
+ Partials      515      511       -4     
Files Changed Coverage Δ
internal/ir/zz_generated.deepcopy.go 13.78% <38.46%> (+0.86%) ⬆️
internal/xds/translator/authentication.go 65.21% <66.66%> (-0.12%) ⬇️
internal/xds/translator/ratelimit.go 86.21% <66.66%> (-0.05%) ⬇️
internal/ir/xds.go 73.60% <70.37%> (+0.79%) ⬆️
internal/xds/translator/route.go 90.76% <83.33%> (+0.10%) ⬆️
internal/xds/translator/translator.go 78.24% <83.33%> (+1.90%) ⬆️
internal/gatewayapi/route.go 88.28% <98.48%> (+0.25%) ⬆️
internal/gatewayapi/filters.go 80.30% <100.00%> (+0.17%) ⬆️
internal/gatewayapi/helpers.go 80.38% <100.00%> (+0.15%) ⬆️
internal/logging/log.go 100.00% <100.00%> (ø)
... and 3 more

... and 2 files with indirect coverage changes

Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
@arkodg
Copy link
Contributor

arkodg commented Aug 21, 2023

ptal @qicz

@@ -55,7 +55,7 @@ func (l Logger) WithName(name string) Logger {
return Logger{
Logger: zapr.NewLogger(logger).WithName(name),
logging: l.logging,
sugaredLogger: l.sugaredLogger,
sugaredLogger: logger.Sugar(),
Copy link
Member

Choose a reason for hiding this comment

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

inited

func NewLogger(logging *v1alpha1.EnvoyGatewayLogging) Logger {
logger := initZapLogger(logging, logging.Level[v1alpha1.LogComponentGatewayDefault])
return Logger{
Logger: zapr.NewLogger(logger),
logging: logging,
sugaredLogger: logger.Sugar(),
}
}
func DefaultLogger(level v1alpha1.LogLevel) Logger {
logging := v1alpha1.DefaultEnvoyGatewayLogging()
logger := initZapLogger(logging, level)
return Logger{
Logger: zapr.NewLogger(logger),
logging: logging,
sugaredLogger: logger.Sugar(),
}
}

Copy link
Member Author

@zhaohuabing zhaohuabing Aug 22, 2023

Choose a reason for hiding this comment

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

@qicz The issue with the current implementation sugaredLogger: l.sugaredLogger is that the logging level of the returned SugaredLogger is incorrect. Please see the test TestLoggerWithName in this PR.

logger.Sugar().Debugf("debug message")
...
assert.Contains(t, capturedOutput, "debug message")

Copy link
Member

@qicz qicz Aug 22, 2023

Choose a reason for hiding this comment

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

misunderstand it, you are right.

qicz

This comment was marked as spam.

qicz

This comment was marked as resolved.

Copy link
Member

@qicz qicz left a comment

Choose a reason for hiding this comment

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

LGTM

@arkodg arkodg merged commit 67234a4 into envoyproxy:main Aug 22, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants