[MM-19315] Replace old Logger with mlog.Logger (mattermost package) #130

willypuzzle · 2024-12-03T16:46:47Z

Summary

This PR replace the old Logger with the more modern mlog.Logger.

Ticket Link

Release note:

Added LogFormat option, possible values are plain or json (default value: plain)

Refactored the logging system in both Android and Apple notification servers. Replaced `logger.Infof` with `logger.Info` for a more structured logging approach, using `mlog.String`, `mlog.Int`, and `mlog.Err` to provide additional context. This change enhances readability and allows for better log analysis.

The logging statements in the server and notification files have been refactored to improve readability. The changes include replacing `logger.Errorf` with `logger.Error` and using structured logging fields instead of string formatting. This makes it easier to understand the log messages and their associated data.

Switched from using Panic to Fatal in the server's error handling mechanism. This change ensures that any serious errors will cause the program to exit immediately, rather than just panicking and potentially leaving the server in an unstable state.

The logger type has been updated from *Logger to *mlog.Logger in the AndroidNotificationServer, AppleNotificationServer, and Server structures. This change also affects the NewAndroidNotificationServer, NewAppleNotificationServer functions where the logger parameter type is updated accordingly.

Refactored the ConfigPushProxy struct. Deprecated LogFileLocation, EnableConsoleLog, and EnableFileLog for backward compatibility. Introduced LoggingCfgFile and LoggingCfgJSON as replacements. No changes to other existing fields.

Removed the automatic enabling of console log when both console and file logs are disabled. Also, removed the creation and handling of log files within the configuration loading function. This simplifies the function by focusing it solely on loading configurations.

The main function has been updated to include a more robust logger initialization process. This includes error handling for the creation of a new logger, as well as its configuration. If no logging is defined, a default config (console output) is used. A separate function has been added to provide this default logging configuration. The server now also ensures that the logger is properly shut down when it's no longer needed.

Upgraded the Go version from 1.21 to 1.22 and updated various dependencies to their latest versions. This includes updates to firebase, gorilla handlers, mux, prometheus client_golang, common, golang.org/x/net and oauth2 among others. Also added new dependencies such as github.com/mattermost/mattermost/server/public and github.com/francoispqt/gojay.

mattermost-build · 2024-12-03T16:46:51Z

Hello @willypuzzle,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

larkox

Looks great! Just a couple of comments to discuss.

server/config_push_proxy.go

main.go

The default logging configuration has been refactored to support both console and file logging. The logic for generating the default config has been moved from main.go to a new file, server/logging.go. This change allows the application to dynamically choose between console or file logging based on the 'EnableFileLog' flag in the configuration.

agnivade · 2024-12-04T04:06:11Z

Thank you @willypuzzle. I will be taking a look at this soon.

larkox

A couple more comments about tests.

main.go

server/logging.go

This reverts commit 8e38a57.

Upgraded the Go version from 1.21 to 1.22 and updated various dependencies to their latest versions. Also, added new dependencies required for the project's functionality.

The Docker images for Go and Golint have been updated to newer versions. The new version of the Go image includes various performance improvements and bug fixes. Similarly, the Golint image has also been updated to include the latest linting rules and enhancements.

The logger initialization in various test files has been updated. Instead of using the NewLogger function, we are now using mlog.NewLogger(). This change affects android_notification_test.go, metrics_test.go, and server_test.go.

- Added error checks after initializing new loggers in various test functions to ensure no silent failures. - Simplified the condition check for enabling file logs by directly using the boolean value.

Introduced a new set of unit tests for the server's logging functionality. These tests cover various aspects such as escaping double quotes, default console log configuration, file log configuration and overall logging configuration. Also added a helper function to generate random strings for testing purposes.

The logging settings have been simplified and restructured. The previous console and file log enablement flags have been removed. Instead, a new approach has been introduced where the logging configuration is now handled through a dedicated config file or directly via JSON.

A new JSON configuration file for logging has been added. This includes settings for console type output, plain formatting, and color enabling. It also specifies minimum lengths for level and message, as well as the inclusion of caller information. Different log levels from debug to panic have been defined with corresponding IDs and optional color or stacktrace attributes.

The logger initialization and configuration process has been refactored for better code organization. The logic previously in the main function is now encapsulated within a new function, NewMlogLogger, in the server package. This change also includes updates to logging tests to accommodate the new structure.

This update includes several significant changes: - The release is compatible with all versions of Mattermost server and mobile. - Notably, Google Cloud Messaging (GCM) service has been deprecated as of April 10, 2018, and will be removed by April 11, 2019. Migration to Firebase Cloud Messaging (FCM) is necessary. - The old Logger system has been replaced with a new one. - Deprecated types EnableConsoleLog, EnableFileLog, and LogFileLocation from the config have been replaced. - Lastly, we've switched from go version 1.21 to go version 1.22 for improved performance and stability.

The README file has been updated to include a reference to the CHANGELOG. This will provide users with more detailed information about the changes made in each release.

willypuzzle · 2024-12-04T20:24:35Z

@larkox @agnivade I made all the requested changes and improvements.

larkox

Great work! Just two minor comments.

server/logging.go

server/logging_test.go

agnivade · 2024-12-23T15:46:28Z

/update-branch

mattermost-build · 2024-12-23T15:46:31Z

Error trying to update the PR.
Please do it manually.

agnivade · 2024-12-23T15:48:15Z

@toninis - Any idea why CI is not running for this PR?

willypuzzle · 2024-12-23T18:11:36Z

Thank you for this @willypuzzle ! There were some more changes needed. But I went ahead and did those myself since you have already spent so much time on this.

@agnivade no prob! ask the same, I'm happy to have practice with go!

mattermost-build · 2025-01-03T01:06:11Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

mattermost-build · 2025-01-14T01:05:43Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

agnivade · 2025-01-14T04:33:19Z

/update-branch

mattermost-build · 2025-01-14T04:33:23Z

Error trying to update the PR.
Please do it manually.

agnivade · 2025-01-14T04:34:30Z

@willypuzzle - could you please update this branch with latest master and push again? Sorry for the delay. The person I pinged is on leave.

willypuzzle · 2025-01-14T09:38:33Z

@willypuzzle - could you please update this branch with latest master and push again? Sorry for the delay. The person I pinged is on leave.

@agnivade the PR's branch is just updated

agnivade · 2025-01-16T09:17:35Z

Can you push an empty commit? I want to re-trigger the CI flow for a fresh commit.

agnivade · 2025-01-17T09:26:42Z

I meant just an empty commit. You can do that by git commit --allow-empty -m 'Trigger CI'. Please revert the earlier commit, and push a new one.

This reverts commit e35b0d6.

willypuzzle · 2025-01-17T09:38:08Z

I meant just an empty commit. You can do that by git commit --allow-empty -m 'Trigger CI'. Please revert the earlier commit, and push a new one.

sorry, I didn't know it.

agnivade · 2025-01-17T14:41:13Z

Thanks, I will take it from here.

agnivade · 2025-01-17T16:46:33Z

@willypuzzle - Could you please merge with latest master and push again? This commit 93d8c3b should take care of the problem.

agnivade · 2025-01-20T03:56:44Z

Thanks for your patience @willypuzzle !

willypuzzle · 2025-01-20T10:04:46Z

Thanks for your patience @willypuzzle !

@agnivade Thanks to you! I always learn a lot from you all!

willypuzzle added 8 commits November 27, 2024 13:58

Updated server error handling

3c4e226

Switched from using Panic to Fatal in the server's error handling mechanism. This change ensures that any serious errors will cause the program to exit immediately, rather than just panicking and potentially leaving the server in an unstable state.

Updated push proxy configuration

4edb970

Refactored the ConfigPushProxy struct. Deprecated LogFileLocation, EnableConsoleLog, and EnableFileLog for backward compatibility. Introduced LoggingCfgFile and LoggingCfgJSON as replacements. No changes to other existing fields.

mattermost-build added the Contributor label Dec 3, 2024

enahum requested review from agnivade and larkox December 3, 2024 18:38

enahum added the 1: Dev Review Requires review by a core commiter label Dec 3, 2024

larkox reviewed Dec 3, 2024

View reviewed changes

server/config_push_proxy.go Outdated Show resolved Hide resolved

server/config_push_proxy.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

larkox reviewed Dec 4, 2024

View reviewed changes

main.go Outdated Show resolved Hide resolved

server/logging.go Outdated Show resolved Hide resolved

willypuzzle added 11 commits December 4, 2024 14:51

Revert "Updated Go version and dependencies"

1cdae1e

This reverts commit 8e38a57.

Updated Go version and dependencies

e5d88d7

Upgraded the Go version from 1.21 to 1.22 and updated various dependencies to their latest versions. Also, added new dependencies required for the project's functionality.

Updated logger initialization in tests

3c9f4b3

The logger initialization in various test files has been updated. Instead of using the NewLogger function, we are now using mlog.NewLogger(). This change affects android_notification_test.go, metrics_test.go, and server_test.go.

Improved error handling and logging configuration

01577ed

- Added error checks after initializing new loggers in various test functions to ensure no silent failures. - Simplified the condition check for enabling file logs by directly using the boolean value.

Updated README with CHANGELOG reference

02a6b10

The README file has been updated to include a reference to the CHANGELOG. This will provide users with more detailed information about the changes made in each release.

larkox requested changes Dec 5, 2024

View reviewed changes

server/logging.go Outdated Show resolved Hide resolved

server/logging_test.go Outdated Show resolved Hide resolved

agnivade added 2: Reviews Complete All reviewers have approved the pull request Do Not Merge Should not be merged until this label is removed and removed 1: Dev Review Requires review by a core commiter labels Dec 23, 2024

mattermost-build added the Lifecycle/1:stale label Jan 3, 2025

hanzei removed the Lifecycle/1:stale label Jan 3, 2025

mattermost-build added the Lifecycle/1:stale label Jan 14, 2025

Empty commit

e35b0d6

willypuzzle added 2 commits January 17, 2025 10:35

Revert "Empty commit"

2812127

This reverts commit e35b0d6.

Trigger CI

badc96e

Merge branch 'master' into MM-19315-add-mlog-logger-as-logger

3ba8817

agnivade removed the Do Not Merge Should not be merged until this label is removed label Jan 20, 2025

agnivade merged commit 7d33b02 into mattermost:master Jan 20, 2025
4 of 5 checks passed

willypuzzle deleted the MM-19315-add-mlog-logger-as-logger branch January 20, 2025 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-19315] Replace old Logger with mlog.Logger (mattermost package) #130

[MM-19315] Replace old Logger with mlog.Logger (mattermost package) #130

willypuzzle commented Dec 3, 2024 •

edited

Loading

mattermost-build commented Dec 3, 2024

larkox left a comment

agnivade commented Dec 4, 2024

larkox left a comment

willypuzzle commented Dec 4, 2024

larkox left a comment

agnivade commented Dec 23, 2024

mattermost-build commented Dec 23, 2024

agnivade commented Dec 23, 2024

willypuzzle commented Dec 23, 2024

mattermost-build commented Jan 3, 2025

mattermost-build commented Jan 14, 2025

agnivade commented Jan 14, 2025

mattermost-build commented Jan 14, 2025

agnivade commented Jan 14, 2025

willypuzzle commented Jan 14, 2025 •

edited

Loading

agnivade commented Jan 16, 2025

agnivade commented Jan 17, 2025

willypuzzle commented Jan 17, 2025

agnivade commented Jan 17, 2025

agnivade commented Jan 17, 2025

agnivade commented Jan 20, 2025

willypuzzle commented Jan 20, 2025

[MM-19315] Replace old Logger with mlog.Logger (mattermost package) #130

[MM-19315] Replace old Logger with mlog.Logger (mattermost package) #130

Conversation

willypuzzle commented Dec 3, 2024 • edited Loading

Summary

Ticket Link

Release note:

mattermost-build commented Dec 3, 2024

larkox left a comment

Choose a reason for hiding this comment

agnivade commented Dec 4, 2024

larkox left a comment

Choose a reason for hiding this comment

willypuzzle commented Dec 4, 2024

larkox left a comment

Choose a reason for hiding this comment

agnivade commented Dec 23, 2024

mattermost-build commented Dec 23, 2024

agnivade commented Dec 23, 2024

willypuzzle commented Dec 23, 2024

mattermost-build commented Jan 3, 2025

mattermost-build commented Jan 14, 2025

agnivade commented Jan 14, 2025

mattermost-build commented Jan 14, 2025

agnivade commented Jan 14, 2025

willypuzzle commented Jan 14, 2025 • edited Loading

agnivade commented Jan 16, 2025

agnivade commented Jan 17, 2025

willypuzzle commented Jan 17, 2025

agnivade commented Jan 17, 2025

agnivade commented Jan 17, 2025

agnivade commented Jan 20, 2025

willypuzzle commented Jan 20, 2025

willypuzzle commented Dec 3, 2024 •

edited

Loading

willypuzzle commented Jan 14, 2025 •

edited

Loading