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

WIP feat: Support go std logger #294

Closed
wants to merge 4 commits into from
Closed

Conversation

gaocegege
Copy link
Contributor

@gaocegege gaocegege commented Nov 25, 2019

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:

NONE

@caicloud-bot caicloud-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 25, 2019
@gaocegege
Copy link
Contributor Author

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 {
Copy link
Contributor Author

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

Copy link
Collaborator

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?

Copy link
Collaborator

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()

Copy link
Contributor Author

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

Copy link
Contributor Author

@gaocegege gaocegege Dec 1, 2019

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()

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.

Copy link
Collaborator

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:

  1. A standard implementation for std log.
  2. 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.

Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM

Copy link
Contributor Author

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.

@caicloud-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: kdada

Assign the PR to them by writing /assign @kdada in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kdada
Copy link
Collaborator

kdada commented Dec 7, 2019

@gaocegege Could you fix the CI?

@gaocegege
Copy link
Contributor Author

gaocegege commented Dec 8, 2019

Sure. If the approach LGT you, I can fix it soon.

@gaocegege
Copy link
Contributor Author

@kdada PTAL

Should we implement the corresponding logic for log plugin in this PR?

@kdada
Copy link
Collaborator

kdada commented Dec 17, 2019

Should we implement the corresponding logic for log plugin in this PR?

Yes. It makes the logger easy to use.

@hezhizhen
Copy link
Contributor

Any updates? @kdada @gaocegege

@gaocegege
Copy link
Contributor Author

@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?

@ddysher
Copy link
Member

ddysher commented Jun 16, 2020

@iawia002 please chime in and move this forward (either check it in or close it).

@caicloud-bot caicloud-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2020
@caicloud-bot
Copy link
Contributor

@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.

@gaocegege
Copy link
Contributor Author

/close

Do not have the bandwidth to maintain it.

@gaocegege gaocegege closed this Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
caicloud-cla: yes Indicates the PR's author has not signed the Caicloud CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to write log to file? Log package: support logging to file
5 participants