From 9418fb079a02f1f7a693714d6d46f521ab25cd19 Mon Sep 17 00:00:00 2001
From: Nelson Vides <nelson.vides@erlang-solutions.com>
Date: Sat, 21 Sep 2024 13:11:12 +0200
Subject: [PATCH] Migrate error_logger to logger

It has been a very long time since OTP made `logger` available and
deprecated `error_logger`, so it's about time we migrate to it.

Note too that we're choosing to report structured logging, that is,
maps, as `worker_pool` is a library, it should allow application code to
choose how to filter and format log messages.
---
 src/wpool.erl                   |  6 +++---
 src/wpool_pool.erl              | 14 +++++++++----
 src/wpool_process_callbacks.erl |  7 ++++++-
 src/wpool_process_sup.erl       |  8 ++++++--
 src/wpool_sup.erl               |  7 ++++++-
 src/wpool_utils.erl             |  2 +-
 src/wpool_worker.erl            | 35 ++++++++++++++++++++++-----------
 test/wpool_SUITE.erl            |  2 +-
 8 files changed, 57 insertions(+), 24 deletions(-)

diff --git a/src/wpool.erl b/src/wpool.erl
index e3bac4e..7803a76 100644
--- a/src/wpool.erl
+++ b/src/wpool.erl
@@ -89,7 +89,7 @@
 -type overrun_handler() :: {Module :: module(), Fun :: atom()}.
 %% The module and function to call when a task is <i>overrun</i>
 %%
-%% The default value for this setting is `{error_logger, warning_report}'. The function must be of
+%% The default value for this setting is `{logger, warning}'. The function must be of
 %% arity 1, and it will be called as`Module:Fun(Args)' where `Args' is a proplist with the following
 %% reported values:
 %% <ul>
@@ -173,7 +173,7 @@
     {worker_opt, [worker_opt()]} |
     {strategy, supervisor_strategy()} |
     {worker_shutdown, worker_shutdown()} |
-    {overrun_handler, overrun_handler()} |
+    {overrun_handler, overrun_handler() | [overrun_handler()]} |
     {overrun_warning, overrun_warning()} |
     {max_overrun_warnings, max_overrun_warnings()} |
     {pool_sup_intensity, pool_sup_intensity()} |
@@ -192,7 +192,7 @@
                      worker_opt => [worker_opt()],
                      strategy => supervisor_strategy(),
                      worker_shutdown => worker_shutdown(),
-                     overrun_handler => overrun_handler(),
+                     overrun_handler => overrun_handler() | [overrun_handler()],
                      overrun_warning => overrun_warning(),
                      max_overrun_warnings => max_overrun_warnings(),
                      pool_sup_intensity => pool_sup_intensity(),
diff --git a/src/wpool_pool.erl b/src/wpool_pool.erl
index 5a12aa9..4f64998 100644
--- a/src/wpool_pool.erl
+++ b/src/wpool_pool.erl
@@ -21,6 +21,8 @@
 %%% `t:wpool:pool_sup_intensity()' options respectively.
 -module(wpool_pool).
 
+-include_lib("kernel/include/logger.hrl").
+
 -behaviour(supervisor).
 
 %% API
@@ -329,7 +331,7 @@ time_checker_name(Name) ->
 init({Name, Options}) ->
     Size = maps:get(workers, Options, 100),
     QueueType = maps:get(queue_type, Options),
-    OverrunHandler = maps:get(overrun_handler, Options, {error_logger, warning_report}),
+    OverrunHandler = maps:get(overrun_handler, Options, {logger, warning}),
     SupShutdown = maps:get(pool_sup_shutdown, Options, brutal_kill),
     TimeCheckerName = time_checker_name(Name),
     QueueManagerName = queue_manager_name(Name),
@@ -522,15 +524,19 @@ find_wpool(Name) ->
 %% @doc We use this function not to report an error if for some reason we've
 %% lost the record on the persistent_term table. This SHOULDN'T be called too much.
 build_wpool(Name) ->
-    error_logger:warning_msg("Building a #wpool record for ~p. Something must have failed.",
-                             [Name]),
+    logger:warning(#{what => "Building a #wpool record. Something must have failed.",
+                     pool => Name},
+                   ?LOCATION),
     try supervisor:count_children(process_sup_name(Name)) of
         Children ->
             Size = proplists:get_value(active, Children, 0),
             store_wpool(Name, Size, #{})
     catch
         _:Error ->
-            error_logger:warning_msg("Wpool ~p not found: ~p", [Name, Error]),
+            logger:warning(#{what => "Wpool not found",
+                             pool => Name,
+                             reason => Error},
+                           ?LOCATION),
             undefined
     end.
 
diff --git a/src/wpool_process_callbacks.erl b/src/wpool_process_callbacks.erl
index cd1901b..c08fff0 100644
--- a/src/wpool_process_callbacks.erl
+++ b/src/wpool_process_callbacks.erl
@@ -1,5 +1,7 @@
 -module(wpool_process_callbacks).
 
+-include_lib("kernel/include/logger.hrl").
+
 -behaviour(gen_event).
 
 %% The callbacks are called in an extremely dynamic from call/3.
@@ -71,7 +73,10 @@ call(Module, Event, Args) ->
         end
     catch
         E:R ->
-            error_logger:warning_msg("Could not call callback module, error:~p, reason:~p", [E, R])
+            logger:warning(#{what => "Could not call callback module",
+                             error => E,
+                             reason => R},
+                           ?LOCATION)
     end.
 
 ensure_loaded(Module) ->
diff --git a/src/wpool_process_sup.erl b/src/wpool_process_sup.erl
index deb49dd..4917d83 100644
--- a/src/wpool_process_sup.erl
+++ b/src/wpool_process_sup.erl
@@ -14,6 +14,8 @@
 %%% @doc This is the supervisor that supervises the `gen_server' workers specifically.
 -module(wpool_process_sup).
 
+-include_lib("kernel/include/logger.hrl").
+
 -behaviour(supervisor).
 
 %% API
@@ -62,6 +64,8 @@ add_initial_callback(EventManager, Module) ->
         ok ->
             ok;
         Other ->
-            error_logger:warning_msg("The callback module:~p could not be loaded, reason:~p",
-                                     [Module, Other])
+            logger:warning(#{what => "The callback module could not be loaded",
+                             module => Module,
+                             reason => Other},
+                           ?LOCATION)
     end.
diff --git a/src/wpool_sup.erl b/src/wpool_sup.erl
index 68e1044..9bd07dc 100644
--- a/src/wpool_sup.erl
+++ b/src/wpool_sup.erl
@@ -14,6 +14,8 @@
 %%% @private
 -module(wpool_sup).
 
+-include_lib("kernel/include/logger.hrl").
+
 -behaviour(supervisor).
 
 -export([start_link/0, init/1]).
@@ -37,7 +39,10 @@ start_pool(Name, Options) ->
 stop_pool(Name) ->
     case erlang:whereis(Name) of
         undefined ->
-            error_logger:warning_msg("Couldn't stop ~p. It was not running", [Name]),
+            logger:warning(#{what => "Could not stop pool",
+                             reason => "It was not running",
+                             pool => Name},
+                           ?LOCATION),
             ok;
         Pid ->
             ok = supervisor:terminate_child(?MODULE, Pid)
diff --git a/src/wpool_utils.erl b/src/wpool_utils.erl
index 1b5eb48..743884e 100644
--- a/src/wpool_utils.erl
+++ b/src/wpool_utils.erl
@@ -56,7 +56,7 @@ add_defaults(Opts) when is_list(Opts) ->
 
 defaults() ->
     #{max_overrun_warnings => infinity,
-      overrun_handler => {error_logger, warning_report},
+      overrun_handler => {logger, warning},
       overrun_warning => infinity,
       queue_type => fifo,
       worker_opt => [],
diff --git a/src/wpool_worker.erl b/src/wpool_worker.erl
index 3bc9176..3c6485d 100644
--- a/src/wpool_worker.erl
+++ b/src/wpool_worker.erl
@@ -16,6 +16,8 @@
 %%% It is a module that implements a very simple RPC-like interface.
 -module(wpool_worker).
 
+-include_lib("kernel/include/logger.hrl").
+
 -behaviour(gen_server).
 
 %% api
@@ -70,12 +72,15 @@ handle_cast({M, F, A}, State) ->
         _ ->
             {noreply, State, hibernate}
     catch
-        _:Error:Stacktrace ->
-            log_error(M, F, A, Error, Stacktrace),
+        Class:Reason:Stacktrace ->
+            log_error(M, F, A, Class, Reason, Stacktrace),
             {noreply, State, hibernate}
     end;
 handle_cast(Cast, State) ->
-    error_logger:error_msg("Invalid cast:~p", [Cast]),
+    logger:error(#{what => "Invalid cast",
+                   cast => Cast,
+                   worker => self()},
+                 ?LOCATION),
     {noreply, State, hibernate}.
 
 %% @private
@@ -86,17 +91,25 @@ handle_call({M, F, A}, _From, State) ->
         R ->
             {reply, {ok, R}, State, hibernate}
     catch
-        _:Error:Stacktrace ->
-            log_error(M, F, A, Error, Stacktrace),
-            {reply, {error, Error}, State, hibernate}
+        Class:Reason:Stacktrace ->
+            log_error(M, F, A, Class, Reason, Stacktrace),
+            {reply, {error, Reason}, State, hibernate}
     end;
-handle_call(Call, _From, State) ->
-    error_logger:error_msg("Invalid call:~p", [Call]),
+handle_call(Call, From, State) ->
+    logger:error(#{what => "Invalid call",
+                   call => Call,
+                   from => From,
+                   worker => self()},
+                 ?LOCATION),
     {reply, {error, invalid_request}, State, hibernate}.
 
 %%%===================================================================
 %%% not exported functions
 %%%===================================================================
-log_error(M, F, A, Error, Stacktrace) ->
-    error_logger:error_msg("Error on ~p:~p~p >> ~p Backtrace ~p",
-                           [M, F, A, Error, Stacktrace]).
+log_error(M, F, A, Class, Reason, Stacktrace) ->
+    logger:error(#{what => "Reason on ~p:~p~p >> ~p Backtrace ~p",
+                   mfa => {M, F, A},
+                   class => Class,
+                   reason => Reason,
+                   stacktrace => Stacktrace},
+                 ?LOCATION).
diff --git a/test/wpool_SUITE.erl b/test/wpool_SUITE.erl
index ee5ba94..07a1ae1 100644
--- a/test/wpool_SUITE.erl
+++ b/test/wpool_SUITE.erl
@@ -292,7 +292,7 @@ stats(_Config) ->
     PoolPid = Get(supervisor, InitStats),
     Options = Get(options, InitStats),
     infinity = Get(overrun_warning, Options),
-    {error_logger, warning_report} = Get(overrun_handler, Options),
+    {logger, warning} = Get(overrun_handler, Options),
     10 = Get(workers, Options),
     10 = Get(size, InitStats),
     1 = Get(next_worker, InitStats),