Notes for Andrew on lager review.

az475-bb-summary.png

lager_notes.md

Notes - Part 1

• Before publishing it would be nice to add a tag to get rid of the annoying and potentially confusing fatal: No names found, cannot describe anything. messages.

• README.org

  • Line 7 - I would emphasize Features in some way or at very least add a colon after it.
  • Usage section - After adding lager logging to some of our projects, I think it would be good to directly link to some examples of how to set it up and use it in this section in an attempt to avoid some of the inevitable questions and confusion from people seeking to use the library.

• rebar.config - Remove the deps tuple since there are no deps or at least remove the commented-out line for riak_err.

• Makefile

  • Need to add test as a PHONY target because there is a test subdirectory in the lager directory. Currently running make test yields make: `test' is up to date.
  • I would remove the targets that are not applicable to lager. e.g. all targets under release targets and developer targets
  • make docs fails with errors
  • orgs targets fails due to missing orgbatch.el
  • check_plt and build_plt targets fail because they are looking for files under deps subdirectory. dialyzer target will not work as a result of the plt targets not working.
  • I did not test the tarball targets since they require a tag.

• trunc_io_eqc.erl

  • This is merely aesthetic, but the comments for the -endif statements at the end of the file should be swapped.
  • Are the problems caused by use of real values in the tests something you want to try to resolve or just leaving them permantly commented-out and leave it to users to exercise them if they choose?

• lager_app.erl - On line 61, the module should be error_logger instead of error_handler

• lager_crash_log.erl - At line 100, there is no file:format_info function so I'm assuming it should be file:format_error

• lager_transform.erl

  • There are some commented-out io:format calls that I assume were there for debugging. Should remove those before publishing.

• lager_trunc_io.erl - In the format clause beginning at line 101, there are several instances of commented-out code. I am not sure if you want it to remain for reference, but if not I would clean that up.

Notes - Part 2

• README.org - line 25 s/which/wish

• Using the parse transform shortcut functions (e.g. lager:error) does not work in riak_client. Not sure why, but my guess would be that fact that it is a parameterized module. The error I get is this:

  src/riak_client.erl:none: internal error in lint_module; crash reason: {{badmatch,false}, [{erl_scan,set_attr,3}, {erl_lint,modify_line1,2}, {erl_lint,modify_line1,2}, {erl_lint,modify_line1,2}, {erl_lint,modify_line1,2}, {erl_lint,modify_line1,2}, {erl_lint,modify_line1,2}, {erl_lint,modify_line1,2}]} make: *** [compile] Error 1

• It seems that in regards to riak that {parse_transform, lager_transform} is needed in the rebar.config for each dependency that will use lager shortcut functions, but is not needed in the toplevel rebar.config.

Notes - Part 3

I ran some basho_bench tests for a duration of 1 hour to look for any significant performance or latency differences in the current master branch code versus the a475 branch code. I do not see any major differences in the results and while this is not completely conclusive, I am satisfied that using lager does not have any drastic detrimental effect compared to the current master branch. I was hoping to provide the results of running a 6 hour test for each branch, but somehow my test that was running overnight failed. I think it would be a good idea to run a set of extended benchmarking tests after you have completed the integration of lager and added any additional logging statements to the code that are needed, just to be sure that the net effect of those changes does not have a negative performance impact. I'll be happy to assist with that testing if needed. Nice work!

master-bb-summary.png