Initial logger support #2230

mths1 · 2023-12-10T18:43:51Z

Proposed Changes

Work in Progress, but works. Currently only console and error log are supported. Investigating syslog and crash. Help is welcome...

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

Bugfix (non-breaking change which fixes issue #XXXX)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation (correction or otherwise)
Cosmetics (whitespace, styles...)
DevOps (Build scripts, pipelines...)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

I have read the CODE_OF_CONDUCT.md document
All tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if needed)
Any dependent changes have been merged and published in related repositories
I have updated changelog (At the bottom of the release version)
I have squashed all my commits into one before merging

Further Comments

If this is a relatively large or complex change, kick off the discussion by
explaining why you chose the solution you did and what alternatives you
considered, etc.

mths1 · 2023-12-10T18:50:54Z

Some decisions to make (among others I guess):

What about SASL? Currently it works with logger_sasl_compatible, but it would be possible to work without, either by let SASL write to the std-log (it writes as INFO, maybe we should move to NOTICE instead of info?), by keeping logger_sasl_compatible or having a seperate logger to a to-be-defined SASL file.

How should the output format look like? The default is:

2023-12-10T19:48:46.972961+01:00 info: Try to start vmq_swc: ok
2023-12-10T19:48:47.059590+01:00 info: Opening LevelDB database at "./data/msgstore/1"
2023-12-10T19:48:47.105198+01:00 info: Opening LevelDB database at "./data/msgstore/2"
2023-12-10T19:48:47.147255+01:00 info: Opening LevelDB database at "./data/msgstore/3"
2023-12-10T19:48:47.186576+01:00 info: Opening LevelDB database at "./data/msgstore/4"
2023-12-10T19:48:47.273619+01:00 info: Opening LevelDB database at "./data/msgstore/5"
2023-12-10T19:48:47.329384+01:00 info: Opening LevelDB database at "./data/msgstore/6"
2023-12-10T19:48:47.422189+01:00 info: Opening LevelDB database at "./data/msgstore/7"
2023-12-10T19:48:47.447329+01:00 info: Opening LevelDB database at "./data/msgstore/8"
2023-12-10T19:48:47.511926+01:00 info: Opening LevelDB database at "./data/msgstore/9"
2023-12-10T19:48:47.536903+01:00 info: Opening LevelDB database at "./data/msgstore/10"
2023-12-10T19:48:47.568671+01:00 info: Opening LevelDB database at "./data/msgstore/11"
2023-12-10T19:48:47.620563+01:00 info: Opening LevelDB database at "./data/msgstore/12"
2023-12-10T19:48:47.705046+01:00 info: Try to start vmq_generic_msg_store: ok
2023-12-10T19:48:48.028764+01:00 info: loaded 0 subscriptions into vmq_reg_trie
2023-12-10T19:48:48.042195+01:00 info: cluster event handler 'vmq_cluster' registered

What changes should the CLI support?

...

mths1 · 2023-12-10T18:51:44Z

(the failing tests are currently kind of expected. Didn't work yet on get them running

mths1 · 2023-12-10T19:11:36Z

Structured Logs Yes/No (would mean more refactoring)
SysLog (Seems not to be possible out-of-the-box)

ioolkos · 2023-12-10T20:35:57Z

@mths1 awesome to have a go at this!

For interested parties, a couple of links:

https://www.erlang.org/doc/apps/kernel/logger_chapter
https://www.erlang.org/doc/apps/kernel/logger_cookbook
https://ferd.ca/erlang-otp-21-s-new-logger.html

mths1 · 2023-12-10T21:37:39Z

Latest version does not need advanced_config anymore. Works now for console (file and real console) and error logging. Still investigating crash log and syslog...

mths1 · 2023-12-11T19:00:10Z

My latest idea:
console => filters SASL progress, now supports rotation
error => includes now SASL crashes, no need for crash log anymore
crash => for backwards compatibility, maybe disabled by default?
sasl => for backwards compatibility, contains supervisor progress (and crash), maybe disabled by default?

We could also include sasl progress reports into console, but it might then be better to upgrade our log_info to log_notice, so that we could filter them away with log_level notice.

Regressions:

Different settings for log rotation
Deprecation of SASL
Deprecation of crash
error_logger always forwarded to logger
different logging format string
Custom plugins would need to upgrade as well

ioolkos · 2023-12-11T20:07:59Z

@mths1
+1 for disabling crash log and SASL, as a default. I'd keep progress reports out of the console which allows us to keep "info" level.

@codeadict do you think structured logging should be part of the deal right from the beginning here? or got at it incrementally?

mths1 · 2023-12-16T14:30:51Z

Latest version to test:

Supports console log (enabled), error log (enabled), sasl log(disable), crash log(disabled), syslog (disabled)
Supports log rotation for all loggers (different feature set than lager though. Regression)
Syslog is supported via https://github.com/garazdawi/syslogger which rabbid uses as well
CLI support (vmq-admin log)

Next step would be to decide on the formatting. I added some config for keeping the default or switch to something similar to the previous format.

ioolkos · 2023-12-16T15:02:18Z

Awesome!
One question I have: Doesn't Rabbit use https://github.com/schlagert/syslog, though? (looking at the Makefile). Would have the benefit of no additional Nifs (C sources). But I currently haven't looked into comparing both libraries (syslogger or syslog) in detail.

mths1 · 2023-12-18T10:11:35Z

@ioolkos : Yes, you are right. Seems i copied the wrong link from somewhere. Good news, also works with the other library.

I added a json formatter (which is based on the one from rabbitmq). I won't work much on this anymore for now. It is ready for testing. Unless, we want to move to structured logging I assume there shouldn't be too many open points left.

codeadict · 2023-12-19T15:18:12Z

https://github.com/schlagert/syslog is definitely better and well maintained and tested, structured logs could be a different PR, not necessary for V1

apps/vmq_bridge/rebar.config

apps/vmq_commons/src/vmq_log.erl

apps/vmq_commons/src/vmq_log_json_format.erl

apps/vmq_diversity/rebar.config

codeadict · 2023-12-19T15:21:52Z

apps/vmq_diversity/rebar.config

@@ -17,5 +17,5 @@
    %%Eonblast hasn't merged the Erlang 18 related PR from djustinek
    %%{emysql, {git, "git://github.com/Eonblast/Emysql.git", {tag, "v0.4.1"}}},
    {mongodb, {git, "https://github.com/comtihon/mongodb-erlang.git", {ref, "713e8bd"}}},
-    {mcd, {git, "https://github.com/EchoTeam/mcd.git", {ref, "b5b4a32"}}}
+    {mcd, {git, "https://github.com/mths1/mcd.git", {branch, "master"}}}


should this be in this branch?

I'd prefer to move mcd to vernemq repro (this is a mcd version without lager dependency)

apps/vmq_server/src/vmq_metadata.erl

codeadict · 2023-12-19T15:28:23Z

apps/vmq_server/src/vmq_metadata.erl

@@ -42,7 +43,7 @@ stop() ->
    Impl = application:get_env(vmq_server, metadata_impl, vmq_plumtree),
    _ = spawn(fun() ->
        Ret = vmq_plugin_mgr:disable_plugin(Impl),
-        lager:info("Try to stop ~p: ~p", [Impl, Ret])
+        ?LOG_INFO("Try to stop ~p: ~p", [Impl, Ret])


Suggested change

?LOG_INFO("Try to stop ~p: ~p", [Impl, Ret])

?LOG_INFO("Trying to stop ~p: ~p", [Impl, Ret])

apps/vmq_swc/rebar.config

apps/vmq_swc/test/vmq_swc_test_utils.erl

apps/vmq_webhooks/rebar.config

ioolkos · 2024-01-29T15:04:10Z

currently testing & reviewing. plan to merge this for the upcoming release.

Address code review Co-authored-by: Dairon M. <dairon.medina@gmail.com>

* Initial logger support * Apply suggestions from code review Address code review Co-authored-by: Dairon M. <dairon.medina@gmail.com> * Address review comments (log show -> log show config) --------- Co-authored-by: mths1 <mths1> Co-authored-by: Dairon M. <dairon.medina@gmail.com>

mths1 force-pushed the feature/logger branch from 700d2f7 to 6a855fb Compare December 10, 2023 18:57

mths1 force-pushed the feature/logger branch 2 times, most recently from f130386 to ed5c6be Compare December 10, 2023 19:57

mths1 force-pushed the feature/logger branch from ed5c6be to eec4be9 Compare December 10, 2023 21:30

mths1 force-pushed the feature/logger branch from eec4be9 to b12b1fd Compare December 11, 2023 15:35

mths1 force-pushed the feature/logger branch 6 times, most recently from 97f8c76 to 70fd0c0 Compare December 16, 2023 14:17

mths1 force-pushed the feature/logger branch 2 times, most recently from 5554958 to db07c50 Compare December 16, 2023 14:53

mths1 force-pushed the feature/logger branch 8 times, most recently from 341cd06 to c34c7aa Compare December 17, 2023 20:11

mths1 force-pushed the feature/logger branch 3 times, most recently from b95bc89 to da5f6b5 Compare December 18, 2023 08:12

codeadict reviewed Dec 19, 2023

View reviewed changes

mths1 marked this pull request as ready for review December 19, 2023 20:52

mths1 force-pushed the feature/logger branch from d0c66f8 to 938152b Compare December 19, 2023 21:26

mths1 force-pushed the feature/logger branch from 938152b to a5ba7fd Compare January 5, 2024 12:06

mths1 and others added 3 commits January 31, 2024 12:13

Initial logger support

c4125df

Apply suggestions from code review

be8f109

Address code review Co-authored-by: Dairon M. <dairon.medina@gmail.com>

Address review comments (log show -> log show config)

bc97838

ioolkos force-pushed the feature/logger branch from a5ba7fd to bc97838 Compare January 31, 2024 11:13

ioolkos approved these changes Jan 31, 2024

View reviewed changes

ioolkos merged commit 6182880 into vernemq:main Jan 31, 2024
9 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial logger support #2230

Initial logger support #2230

mths1 commented Dec 10, 2023 •

edited

mths1 commented Dec 10, 2023

mths1 commented Dec 10, 2023

mths1 commented Dec 10, 2023

ioolkos commented Dec 10, 2023

mths1 commented Dec 10, 2023

mths1 commented Dec 11, 2023

ioolkos commented Dec 11, 2023

mths1 commented Dec 16, 2023

ioolkos commented Dec 16, 2023

mths1 commented Dec 18, 2023

codeadict commented Dec 19, 2023

codeadict Dec 19, 2023

mths1 Dec 19, 2023

codeadict Dec 19, 2023

ioolkos commented Jan 29, 2024

	?LOG_INFO("Try to stop ~p: ~p", [Impl, Ret])
	?LOG_INFO("Trying to stop ~p: ~p", [Impl, Ret])

Initial logger support #2230

Initial logger support #2230

Conversation

mths1 commented Dec 10, 2023 • edited

Proposed Changes

Types of Changes

Checklist

Further Comments

mths1 commented Dec 10, 2023

mths1 commented Dec 10, 2023

mths1 commented Dec 10, 2023

ioolkos commented Dec 10, 2023

mths1 commented Dec 10, 2023

mths1 commented Dec 11, 2023

ioolkos commented Dec 11, 2023

mths1 commented Dec 16, 2023

ioolkos commented Dec 16, 2023

mths1 commented Dec 18, 2023

codeadict commented Dec 19, 2023

codeadict Dec 19, 2023

Choose a reason for hiding this comment

mths1 Dec 19, 2023

Choose a reason for hiding this comment

codeadict Dec 19, 2023

Choose a reason for hiding this comment

ioolkos commented Jan 29, 2024

mths1 commented Dec 10, 2023 •

edited