-
Notifications
You must be signed in to change notification settings - Fork 105
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
WIP feat: Support go std logger #294
Conversation
Signed-off-by: Ce Gao <[email protected]>
Here is an example func main() {
// Print nirvana banner.
fmt.Println(nirvana.Logo, nirvana.Banner)
// Create nirvana command.
cmd := config.NewNamedNirvanaCommand("server", config.NewDefaultOption())
// Create plugin options.
metricsOption := metrics.NewDefaultOption() // Metrics plugin.
reqlogOption := reqlog.NewDefaultOption() // Request log plugin.
versionOption := pversion.NewOption( // Version plugin.
"helloworld",
version.Version,
version.Commit,
version.Package,
)
// Enable plugins.
cmd.EnablePlugin(metricsOption, reqlogOption, versionOption)
// Create server config.
serverConfig := nirvana.NewConfig()
f, err := os.OpenFile("testlogfile", os.O_RDWR|os.O_CREATE|os.O_APPEND, 0666)
if err != nil {
fmt.Printf("Failed to open the file: %v", err)
os.Exit(1)
}
defer f.Close()
// Configure APIs. These configurations may be changed by plugins.
serverConfig.Configure(
nirvana.Logger(log.NewGoStandardLogger(0, f, "test", stdlog.LstdFlags)),
nirvana.Filter(filters.Filters()...),
nirvana.Modifier(modifiers.Modifiers()...),
nirvana.Descriptor(apis.Descriptor()),
)
// Set nirvana command hooks.
cmd.SetHook(&config.NirvanaCommandHookFunc{
PreServeFunc: func(config *nirvana.Config, server nirvana.Server) error {
// Output project information.
config.Logger().Infof("Package:%s Version:%s Commit:%s", version.Package, version.Version, version.Commit)
return nil
},
})
// Start with server config.
if err := cmd.ExecuteWithConfig(serverConfig); err != nil {
serverConfig.Logger().Fatal(err)
}
} |
log/golog.go
Outdated
} | ||
|
||
// NewGoStandardLogger creates a zap logger. | ||
func NewGoStandardLogger(level Level, out io.Writer, prefix string, flag int) Logger { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should provide a NewDefaultGoStandardLogger since we already have one using stdLogger.
/cc @kdada
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you wrap a standard log rather than output data to the out
directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flag stdlog.LstdFlags
is duplicated with the result of prefix()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, if there is a mature tool, prefer not to reinvent a new one. And std log supports log header line or some other features. WDYT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flag
stdlog.LstdFlags
is duplicated with the result ofprefix()
I think the prefix and flags serves for different purposes. But it also works for me if we remove the support for prefix or flags.
Or users can use Lmsgprefix
instead of LstdFlags
. The example above is to show the use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the behaviors of this logger should be splitted into two scenarios:
- A standard implementation for std log.
- A pure file logger for the log format of
prefix()
.
So I think you can remove the prefix()
from this PR and make it be the 1st implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my personal preference, I'd like the 2nd implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, can we output the log to file and to STDOUT? Maybe register two loggers.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@gaocegege Could you fix the CI? |
Sure. If the approach LGT you, I can fix it soon. |
Signed-off-by: Ce Gao <[email protected]>
Signed-off-by: Ce Gao <[email protected]>
Signed-off-by: Ce Gao <[email protected]>
@kdada PTAL Should we implement the corresponding logic for log plugin in this PR? |
Yes. It makes the logger easy to use. |
Any updates? @kdada @gaocegege |
@hezhizhen Recently working on Clever and do not have the bandwidth to deal with it. @kdada Can we merge it if we implement the log plugin in another PR? |
@iawia002 please chime in and move this forward (either check it in or close it). |
@gaocegege: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/close Do not have the bandwidth to maintain it. |
Signed-off-by: Ce Gao [email protected]
What this PR does / why we need it:
Add your description
Which issue(s) this PR is related to (optional, link to 3rd issue(s)):
Fixes #245
Fixes #291
Reference to #
Special notes for your reviewer:
/cc @your-reviewer
Release note: