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

Change lager to logger #1898

Merged
merged 11 commits into from Nov 10, 2018

Conversation

@terry-xiaoyu
Copy link
Contributor

terry-xiaoyu commented Oct 16, 2018

I'd like to optimize the Log section in our configuration file, mainly for readability.

@terry-xiaoyu terry-xiaoyu force-pushed the switch_to_logger branch 2 times, most recently from 6a2aa82 to cbd9967 Oct 16, 2018
Copy link
Contributor

spring2maz left a comment

will this work properly without corresponding changes in cuttlefish schema file ?
i.e. priv/emqx.schema

etc/emqx.conf Outdated Show resolved Hide resolved
etc/emqx.conf Outdated Show resolved Hide resolved
@terry-xiaoyu

This comment has been minimized.

Copy link
Contributor Author

terry-xiaoyu commented Oct 17, 2018

Other changes will come soon. I created the PR early to discuss the config template with you.

@terry-xiaoyu terry-xiaoyu requested a review from zhengyupan Oct 17, 2018
etc/emqx.conf Outdated Show resolved Hide resolved
etc/emqx.conf Outdated Show resolved Hide resolved
etc/emqx.conf Outdated Show resolved Hide resolved
@emqplus emqplus self-assigned this Oct 18, 2018
@terry-xiaoyu terry-xiaoyu changed the title Optimize config for log section Change lager to logger Nov 2, 2018
@terry-xiaoyu terry-xiaoyu force-pushed the switch_to_logger branch from 2b264e9 to 2a5f3e9 Nov 2, 2018
@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 2, 2018

Pull Request Test Coverage Report for Build 3726

  • 21 of 197 (10.66%) changed or added relevant lines in 12 files are covered.
  • 25 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-2.0%) to 61.519%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/emqx_broker.erl 0 1 0.0%
src/emqx_listeners.erl 3 4 75.0%
src/emqx_sys_mon.erl 0 1 0.0%
src/emqx_bridge.erl 0 2 0.0%
src/emqx_ctl.erl 0 2 0.0%
src/emqx_logger.erl 8 18 44.44%
src/emqx_tracer.erl 4 26 15.38%
src/emqx_logger_formatter.erl 0 137 0.0%
Files with Coverage Reduction New Missed Lines %
src/emqx_ctl.erl 1 33.33%
src/emqx_tracer.erl 1 13.95%
src/emqx_session.erl 11 72.06%
src/emqx_local_bridge.erl 12 0.0%
Totals Coverage Status
Change from base Build 3682: -2.0%
Covered Lines: 2737
Relevant Lines: 4449

💛 - Coveralls
AdditionalLogFiles =
if LogTo =:= file orelse LogTo =:= both ->
lists:filter(fun({K, V}) ->
cuttlefish_variable:is_fuzzy_match(K, string:tokens("log.$level.file", "."))

This comment has been minimized.

Copy link
@spring2maz

spring2maz Nov 3, 2018

Contributor

isn't string:tokens("log.$level.file", ".") ["log", "$level", "file"] ?

This comment has been minimized.

Copy link
@terry-xiaoyu

terry-xiaoyu Nov 5, 2018

Author Contributor

Yes, I'll fix this.

config => FileConf(Filename),
formatter => Formatter,
filesync_repeat_interval => 1000}}
|| {[_, Level, _], Filename} <- AdditionalLogFiles],

This comment has been minimized.

Copy link
@spring2maz

spring2maz Nov 3, 2018

Contributor

if we change the lists:filter at line 474 to a list:foldl and return {Level, Filename} there, it would be more readable here.

This comment has been minimized.

Copy link
@terry-xiaoyu

terry-xiaoyu Nov 5, 2018

Author Contributor

I'll fix this.

filesync_repeat_interval => 1000}}
|| {[_, Level, _], Filename} <- AdditionalLogFiles],

_AllHandlers = DefaultHandler ++ FileHandler ++ AdditionalHandlers

This comment has been minimized.

Copy link
@spring2maz

spring2maz Nov 3, 2018

Contributor

No need to have _AllHandlers because the ++ ops are obvious enough ?

This comment has been minimized.

Copy link
@terry-xiaoyu

terry-xiaoyu Nov 5, 2018

Author Contributor

OK.

#{level => TopLogLevel,
config => FileConf(cuttlefish:conf_get("log.file", Conf)),
formatter => Formatter,
filesync_repeat_interval => 1000}}];

This comment has been minimized.

Copy link
@spring2maz

spring2maz Nov 3, 2018

Contributor

why 1000 ? repeating fsync every one second seems to be a bit too aggressive to me.

This comment has been minimized.

Copy link
@terry-xiaoyu

terry-xiaoyu Nov 5, 2018

Author Contributor

I copied this from the examples of erlang-doc... I think the value no_repeat is a good choice.

filesync_repeat_interval

This value, in milliseconds, specifies how often the handler does a disk_log sync operation to write buffered data to disk. The handler attempts the operation repeatedly, but only performs a new sync if something has actually been logged.

Defaults to 5000 milliseconds.

If no_repeat is set as value, the repeated sync operation is disabled. The user can also call the filesync/1 function to perform a disk_log sync.

## Format: log.$level.file = $filename,
## where "$level" can be one of: debug, info, notice, warning,
## error, critical, alert, emergency
## Note: Log files for a specific log level will contain all the logs

This comment has been minimized.

Copy link
@spring2maz

spring2maz Nov 3, 2018

Contributor

I'm not quite sure why we need this extra log config if we already have log.file.
Especially when the severity level filter logic (higher than or equal to) is exactly the same.

This comment has been minimized.

Copy link
@terry-xiaoyu

terry-xiaoyu Nov 5, 2018

Author Contributor

For me, it is convenient to set an extra log file that contains logs with severity level higher than 'error', as it helps me to find all the errors quickly, when the main log level is set to 'debug'.

I commented this config out so there will no extra log files by default.

case logger:add_handler(handler_id(Who), logger_disk_log_h,
#{level => Level,
formatter => ?FORMAT,
filesync_repeat_interval => 1000,

This comment has been minimized.

Copy link
@spring2maz

spring2maz Nov 3, 2018

Contributor

same here fsync interval 1000 is too aggressive IMO

Config;
Size0 ->
Size =
case Size0 - string:length([B,A]) of

This comment has been minimized.

Copy link
@spring2maz

spring2maz Nov 3, 2018

Contributor

string:length([B,A]) is always 2 no matter what B and A are.

1> string:len([a,b]).
2

This comment has been minimized.

Copy link
@terry-xiaoyu

terry-xiaoyu Nov 5, 2018

Author Contributor

string:length(["B1","A"]) returns 3, I think this code is correct.

B = do_format(Level,Meta,BT,Config),
A = do_format(Level,Meta,AT,Config),
MsgStr =
if DoMsg ->

This comment has been minimized.

Copy link
@spring2maz

spring2maz Nov 3, 2018

Contributor

this whole block from this line to line 80 worth an abstraction to a help function.

This comment has been minimized.

Copy link
@terry-xiaoyu

terry-xiaoyu Nov 5, 2018

Author Contributor

Agree. But this whole file is copied from OTP lib/kernel/src/logger_formatter.erl maily for new date format, and I'd like to keep most of the file untouched.

@turtleDeng turtleDeng merged commit 6e26f5e into emqx30 Nov 10, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 61.507%
Details
@turtleDeng turtleDeng deleted the switch_to_logger branch Nov 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.