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

Add log level and format configuration options for services #567

Merged
merged 3 commits into from
Dec 16, 2024

Conversation

zhuoyuan-liu
Copy link
Contributor

Introduce configuration options for log level and format in services, allowing customization of logging behavior. This change enhances the flexibility of logging by enabling different log levels and formats through command-line flags and environment variables.

The change was made for admin and api. For tls, I would like to discuss a better naming convention:

  • We need a configuration for server's logs
  • We need another logger configuration for exports logs and results from osquery

Also, the code are highly duplicated and I would like to remove the duplication.

For documentation, do you know where should update these configuration options?

@javuto javuto self-requested a review November 15, 2024 19:33
@javuto javuto added 🗄️ logging Logging related issues osctrl-api osctrl-api related changes osctrl-admin osctrl-admin related changes labels Nov 15, 2024
@javuto
Copy link
Collaborator

javuto commented Nov 16, 2024

This is great, thank you for working on it!

If you are thinking having different flags for osctrl-tls, it would be good to follow the same convention for the three services.

What do you mean with We need another logger configuration for exports logs and results from osquery?

To update the documentation, clone the repository https://github.com/jmpsec/osctrl-docs, update the markdown in the content folder, and just run make to generate new HTML docs. Once the PR is merged, the documentation at https://osctrl.net will be automatically updated.

@@ -1046,8 +1085,13 @@ func main() {
}
app.Action = cliAction
if err := app.Run(os.Args); err != nil {
log.Fatal().Msgf("app.Run error: %v", err)
Copy link
Collaborator

@javuto javuto Nov 22, 2024

Choose a reason for hiding this comment

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

It is preferable to use Fatal() than to print the error and call exit, because closing writers and flushing buffers. See the implementation of Fatal() in zerolog:

// Fatal starts a new message with fatal level. The os.Exit(1) function
// is called by the Msg method, which terminates the program immediately.
//
// You must call Msg on the returned event in order to send the event.
func (l *Logger) Fatal() *Event {
	return l.newEvent(FatalLevel, func(msg string) {
		if closer, ok := l.w.(io.Closer); ok {
			// Close the writer to flush any buffered message. Otherwise the message
			// will be lost as os.Exit() terminates the program immediately.
			closer.Close()
		}
		os.Exit(1)
	})
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we cannot use Fatal() because the zerolog is not initialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will initialize the log after reading all the parameters.

api/main.go Show resolved Hide resolved
api/main.go Show resolved Hide resolved
admin/main.go Show resolved Hide resolved
admin/main.go Outdated Show resolved Hide resolved
admin/main.go Outdated Show resolved Hide resolved
api/main.go Outdated Show resolved Hide resolved
api/main.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@javuto javuto left a comment

Choose a reason for hiding this comment

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

I think we can use the same parameters in osctrl-tls for consistency, but there are some changes needed, thank you!

Copy link
Contributor Author

@zhuoyuan-liu zhuoyuan-liu left a comment

Choose a reason for hiding this comment

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

@javuto I have updated the code according to your comments, could you please take another look.

@@ -1046,8 +1085,13 @@ func main() {
}
app.Action = cliAction
if err := app.Run(os.Args); err != nil {
log.Fatal().Msgf("app.Run error: %v", err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we cannot use Fatal() because the zerolog is not initialized.

@@ -1046,8 +1085,13 @@ func main() {
}
app.Action = cliAction
if err := app.Run(os.Args); err != nil {
log.Fatal().Msgf("app.Run error: %v", err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will initialize the log after reading all the parameters.

@javuto javuto merged commit 884d17d into jmpsec:main Dec 16, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄️ logging Logging related issues osctrl-admin osctrl-admin related changes osctrl-api osctrl-api related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants