Refactor: Allow Metrics instance injection in MetricsTool

tools/metrics_tool.py

@@ -1,13 +1,22 @@
 # tools/metrics_tool.py
-
 from .base_tool import BaseTool
-from .metrics import metrics
+from .metrics import metrics as global_metrics_instance # For default and measuring execute
+from .metrics import Metrics # For type hinting and potentially creating a new one if needed
+import logging
 
 class MetricsTool(BaseTool):
-    def __init__(self):
+    def __init__(self, metrics_provider: Metrics | None = None, logger: logging.Logger | None = None):
-        self.metrics = metrics
+        self.metrics_provider = metrics_provider if metrics_provider is not None else global_metrics_instance
+        self.logger = logger if logger else logging.getLogger(__name__)
+        if not self.logger.handlers:
+            self.logger.addHandler(logging.NullHandler())
+        self.logger.debug(f"MetricsTool initialized. Using metrics provider: {self.metrics_provider}")
 
     def clear(self):
+        # This tool itself doesn't hold state that needs clearing beyond what its metrics_provider might do.
+        # If this tool were responsible for clearing the metrics it reports on, it would call:
+        # self.metrics_provider.clear_metrics()
+        self.logger.debug("MetricsTool clear method called. No local state to clear.")
         pass
 
     def get_functions(self):
@@ -60,25 +69,60 @@ class MetricsTool(BaseTool):
             }
         ]
 
-    @metrics.measure
+    @global_metrics_instance.measure # The execute method can be measured by the global instance
     def execute(self, function_name, **kwargs):
+        self.logger.info(f"Executing MetricsTool function: {function_name} with args: {kwargs}")
         if function_name == "get_function_metrics":
             return self._get_function_metrics()
         elif function_name == "get_specific_function_metrics":
-            return self._get_specific_function_metrics(kwargs.get("function_name"))
+            func_name_arg = kwargs.get("function_name")
+            if func_name_arg is None: # Check if None, as empty string could be a valid (though unlikely) func name
+                self.logger.warning("'function_name' argument is missing for get_specific_function_metrics.")
+                return "Error: Missing required argument 'function_name'."
+            return self._get_specific_function_metrics(str(func_name_arg)) # Ensure string
         elif function_name == "get_top_n_functions":
-            return self._get_top_n_functions(kwargs.get("n"))
+            n_arg = kwargs.get("n")
+            if n_arg is None:
+                self.logger.warning("'n' argument is missing for get_top_n_functions.")
+                return "Error: Missing required argument 'n'."
+            try:
+                n_val = int(n_arg)
+                if n_val <= 0:
+                    self.logger.warning(f"'n' argument must be a positive integer, got {n_val}.")
+                    return "Error: Argument 'n' must be a positive integer."
+                return self._get_top_n_functions(n_val)
+            except ValueError:
+                self.logger.warning(f"'n' argument must be an integer, got '{n_arg}'.")
+                return "Error: Argument 'n' must be an integer."
         else:
-            return f"Unknown function: {function_name}"
+            error_message = f"Unknown function: {function_name}"
+            self.logger.error(error_message)
+            return error_message
 
     def _get_function_metrics(self):
-        return self.metrics.get_metrics()
+        self.logger.debug("Calling metrics_provider.get_metrics() for all functions.")
+        return self.metrics_provider.get_metrics()
 
-    def _get_specific_function_metrics(self, function_name):
+    def _get_specific_function_metrics(self, function_to_get):
-        all_metrics = self.metrics.get_metrics()
+        self.logger.debug(f"Getting metrics for specific function: {function_to_get}")
-        return all_metrics.get(function_name, f"No metrics found for function: {function_name}")
+        all_metrics = self.metrics_provider.get_metrics()
+        return all_metrics.get(function_to_get, f"No metrics found for function: {function_to_get}")
 
     def _get_top_n_functions(self, n):
-        all_metrics = self.metrics.get_metrics()
+        self.logger.debug(f"Getting top {n} functions by total execution time.")
-        sorted_metrics = sorted(all_metrics.items(), key=lambda x: x[1]['total_time'], reverse=True)
+        all_metrics = self.metrics_provider.get_metrics()
-        return dict(sorted_metrics[:n])
+        # Ensure that the items are actual metric dicts before trying to access 'total_time'
+        valid_metrics_items = []
+        for name, metric_values in all_metrics.items():
+            if isinstance(metric_values, dict) and 'total_time' in metric_values:
+                valid_metrics_items.append((name, metric_values))
+            else:
+                self.logger.warning(f"Metric item for '{name}' is not in expected format: {metric_values}")
+
+        # Sort items by total_time. items() gives list of (func_name, metrics_dict)
+        try:
+            sorted_metrics = sorted(valid_metrics_items, key=lambda item: item[1]['total_time'], reverse=True)
+            return dict(sorted_metrics[:n])
+        except TypeError as e:
+            self.logger.error(f"Error sorting metrics, possibly due to unexpected data types: {e}", exc_info=True)
+            return "Error: Could not sort metrics due to unexpected data."