improv: address nicolas feedback

Author: heitorlessa

python/README.md

@@ -108,7 +108,7 @@ def handler(event, context)
 
 # another_file.py
 from aws_lambda_powertools.tracing import Tracer
-tracer = Tracer.instance()
+tracer = Tracer(auto_patch=False) # new instance using existing configuration with auto patching overriden
 ```
 
 ### Logging
@@ -256,8 +256,14 @@ def middleware_before_after(handler, event, context):
     logic_after_handler_execution()
     return response
 
-@middleware_before_after
-@middleware_name
+
+# middleware_name will wrap Lambda handler 
+# and simply return the handler as we're not pre/post-processing anything
+# then middleware_before_after will wrap middleware_name
+# run some code before/after calling the handler returned by middleware_name
+# This way, lambda_handler is only actually called once (top-down)
+@middleware_before_after # This will run last
+@middleware_name # This will run first
 def lambda_handler(event, context):
     return True
 ```
@@ -273,8 +279,7 @@ def obfuscate_sensitive_data(handler, event, context, fields=None):
             field = event.get(field, "")
             event[field] = obfuscate_pii(field)
 
-    response = handler(event, context)
-    return response
+    return handler(event, context)
 
 @obfuscate_sensitive_data(fields=["email"])
 def lambda_handler(event, context):
@@ -305,7 +310,7 @@ from aws_lambda_powertools.tracing import Tracer
 
 @lambda_handler_decorator(trace_execution=True)
 def middleware_name(handler, event, context):
-    tracer = Tracer.instance() # Takes a copy of an existing tracer instance
+    tracer = Tracer() # Takes a copy of an existing tracer instance
     tracer.add_anotation...
     tracer.metadata...
     return handler(event, context)

python/aws_lambda_powertools/helper/register.py

@@ -2,10 +2,11 @@
 import inspect
 import logging
 import os
-from contextlib import contextmanager
 from distutils.util import strtobool
 from typing import Callable
 
+from ..tracing import Tracer
+
 logger = logging.getLogger(__name__)
 logger.setLevel(os.getenv("LOG_LEVEL", "INFO"))
 
@@ -117,8 +118,10 @@ def wrapper(event, context):
             try:
                 middleware = functools.partial(decorator, func, event, context, **kwargs)
                 if trace_execution:
-                    with _trace_middleware(middleware=decorator):
-                        response = middleware()
+                    tracer = Tracer(auto_patch=False)
+                    tracer.create_subsegment(name=f"## middleware {decorator.__qualname__}")
+                    response = middleware()
+                    tracer.end_subsegment()
                 else:
                     response = middleware()
                 return response
@@ -129,15 +132,3 @@ def wrapper(event, context):
         return wrapper
 
     return final_decorator
-
-
-@contextmanager
-def _trace_middleware(middleware):
-    try:
-        from ..tracing import Tracer
-
-        tracer = Tracer.instance()
-        tracer.create_subsegment(name=f"## middleware {middleware.__qualname__}")
-        yield
-    finally:
-        tracer.end_subsegment()

python/aws_lambda_powertools/middleware_factory/factory.py

@@ -1,3 +1,4 @@
+import copy
 import functools
 import logging
 import os
@@ -6,25 +7,25 @@
 
 from aws_xray_sdk.core import models, patch_all, xray_recorder
 
-from ..helper.register import RegisterMeta
-
 is_cold_start = True
 logger = logging.getLogger(__name__)
 logger.setLevel(os.getenv("LOG_LEVEL", "INFO"))
 
 
-class Tracer(metaclass=RegisterMeta):
+class Tracer:
     """Tracer using AWS-XRay to provide decorators with known defaults for Lambda functions
 
-    When running locally, it honours `POWERTOOLS_TRACE_DISABLED` environment variable
-    so end user code doesn't have to be modified to run it locally
-    instead Tracer returns dummy segments/subsegments.
-
-    Tracing is automatically disabled when running locally via via SAM CLI.
+    When running locally, it detects whether it's running via SAM CLI,
+    and if it is it returns dummy segments/subsegments instead.
 
-    By default, it patches all available libraries supported by X-Ray SDK. \n
+    By default, it patches all available libraries supported by X-Ray SDK. Patching is
+    automatically disabled when running locally via SAM CLI or by any other means. \n
     Ref: https://docs.aws.amazon.com/xray-sdk-for-python/latest/reference/thirdparty.html
 
+    Tracer keeps a copy of its configuration as it can be instantiated more than once. This
+    is useful when you are using your own middlewares and want to utilize an existing Tracer.
+    Make sure to set `auto_patch=False` in subsequent Tracer instances to avoid double patching.
+
     Environment variables
     ---------------------
     POWERTOOLS_TRACE_DISABLED : str
@@ -39,7 +40,7 @@ class Tracer(metaclass=RegisterMeta):
     auto_patch: bool
         Patch existing imported modules during initialization, by default True
     disabled: bool
-        Flag to explicitly disable tracing, useful when running locally.
+        Flag to explicitly disable tracing, useful when running/testing locally.
         `Env POWERTOOLS_TRACE_DISABLED="true"`
 
     Example
@@ -105,7 +106,7 @@ def handler(event: dict, context: Any) -> Dict:
 
         # utils.py
         from aws_lambda_powertools.tracing import Tracer
-        tracer = Tracer.instance()
+        tracer = Tracer()
         ...
 
     Returns
@@ -119,22 +120,22 @@ def handler(event: dict, context: Any) -> Dict:
 
     """
 
+    _default_config = {"service": "service_undefined", "disabled": False, "provider": xray_recorder, "auto_patch": True}
+    _config = copy.copy(_default_config)
+
     def __init__(
-        self,
-        service: str = "service_undefined",
-        disabled: bool = False,
-        provider: xray_recorder = xray_recorder,
-        auto_patch: bool = True,
+        self, service: str = None, disabled: bool = None, provider: xray_recorder = None, auto_patch: bool = None
     ):
-        self.provider = provider
-        self.disabled = self.__is_trace_disabled() or disabled
-        self.service = os.getenv("POWERTOOLS_SERVICE_NAME") or service
-        self.auto_patch = auto_patch
+        self.__build_config(service=service, disabled=disabled, provider=provider, auto_patch=auto_patch)
+        self.provider = self._config["provider"]
+        self.disabled = self._config["disabled"]
+        self.service = self._config["service"]
+        self.auto_patch = self._config["auto_patch"]
 
         if self.disabled:
             self.__disable_tracing_provider()
 
-        if auto_patch:
+        if self.auto_patch:
             self.patch()
 
     def capture_lambda_handler(self, lambda_handler: Callable[[Dict, Any], Any] = None):
@@ -340,18 +341,17 @@ def end_subsegment(self):
         self.provider.end_subsegment()
 
     def patch(self):
-        """Patch modules for instrumentation
-        """
+        """Patch modules for instrumentation"""
         logger.debug("Patching modules...")
 
-        is_lambda_emulator = os.getenv("AWS_SAM_LOCAL")
-        is_lambda_env = os.getenv("LAMBDA_TASK_ROOT")
+        is_lambda_emulator = os.getenv("AWS_SAM_LOCAL", False)
+        is_lambda_env = os.getenv("LAMBDA_TASK_ROOT", False)
 
         if self.disabled:
             logger.debug("Tracing has been disabled, aborting patch")
             return
 
-        if is_lambda_emulator or not is_lambda_env:
+        if is_lambda_emulator or is_lambda_env:
             logger.debug("Running under SAM CLI env or not in Lambda; aborting patch")
             return
 
@@ -390,3 +390,19 @@ def __is_trace_disabled(self) -> bool:
             return is_lambda_emulator
 
         return False
+
+    def __build_config(
+        self, service: str = None, disabled: bool = None, provider: xray_recorder = None, auto_patch: bool = None
+    ):
+        """ Populates Tracer config for new and existing initializations """
+        is_disabled = disabled if disabled is not None else self.__is_trace_disabled()
+        is_service = service if service is not None else os.getenv("POWERTOOLS_SERVICE_NAME")
+
+        self._config["provider"] = provider if provider is not None else self._config["provider"]
+        self._config["auto_patch"] = auto_patch if auto_patch is not None else self._config["auto_patch"]
+        self._config["service"] = is_service if is_service else self._config["service"]
+        self._config["disabled"] = is_disabled if is_disabled else self._config["disabled"]
+
+    @classmethod
+    def _reset_config(cls):
+        cls._config = copy.copy(cls._default_config)

python/aws_lambda_powertools/tracing/tracer.py

@@ -9,9 +9,9 @@ def dummy_response():
 
 
 @pytest.fixture(scope="function", autouse=True)
-def reset_singleton():
+def reset_tracing_config():
+    Tracer._reset_config()
     yield
-    Tracer.clear_instance()
 
 
 def test_capture_lambda_handler(dummy_response):
@@ -53,6 +53,7 @@ def handler(event, context):
         return dummy_response
 
     handler({}, {})
+    monkeypatch.delenv("AWS_SAM_LOCAL")
 
 
 def test_tracer_metadata_disabled(dummy_response):
@@ -114,29 +115,12 @@ def greeting(name, message):
 
 
 def test_tracer_reuse():
-    # GIVEN tracer A, B and C were initialized
-    # WHEN tracer B explicitly reuses A instance
+    # GIVEN tracer A, B were initialized
+    # WHEN tracer B explicitly reuses A config
     # THEN tracer B attributes should be equal to tracer A
-    # and tracer C should use have defaults instead
     service_name = "booking"
     tracer_a = Tracer(disabled=True, service=service_name)
-    tracer_b = Tracer.instance()
-    tracer_c = Tracer()
+    tracer_b = Tracer()
 
-    assert id(tracer_a) == id(tracer_b)
-    assert id(tracer_a) != id(tracer_c)
-    assert id(tracer_b) != id(tracer_c)
-
-
-def test_tracer_reuse_no_instance():
-    # GIVEN tracer A and B instance were not registered
-    # WHEN instance is method is explicitly called
-    # THEN we initialize a new instance and register it
-    # so that tracer B receives a copy of the instance
-    # and only one __init__ is executed
-    tracer_a = Tracer.instance()
-    tracer_b = Tracer.instance()
-
-    assert id(tracer_a) == id(tracer_b)
-
-    Tracer.clear_instance()
+    assert id(tracer_a) != id(tracer_b)
+    assert tracer_a.__dict__.items() == tracer_b.__dict__.items()

python/tests/functional/test_tracing.py

@@ -40,6 +40,12 @@ def end_subsegment(self, *args, **kwargs):
     return XRayStub
 
 
+@pytest.fixture(scope="function", autouse=True)
+def reset_tracing_config():
+    Tracer._reset_config()
+    yield
+
+
 def test_tracer_lambda_handler(mocker, dummy_response, xray_stub):
     put_metadata_mock = mocker.MagicMock()
     begin_subsegment_mock = mocker.MagicMock()
@@ -130,26 +136,20 @@ def handler(event, context):
     assert put_annotation_mock.call_count == 1
     assert put_annotation_mock.call_args == mocker.call(key=annotation_key, value=annotation_value)
 
-@mock.patch('aws_lambda_powertools.tracing.Tracer.patch')
+
+@mock.patch("aws_lambda_powertools.tracing.Tracer.patch")
 def test_tracer_autopatch(patch_mock):
     # GIVEN tracer is instantiated
     # WHEN default options were used, or patch() was called
     # THEN tracer should patch all modules
-    tracer_a = Tracer(disabled=True)    
-    
+    Tracer(disabled=True)
     assert patch_mock.call_count == 1
 
-    tracer_b = Tracer(disabled=True, auto_patch=False)
-    tracer_b.patch()
 
-    assert patch_mock.call_count == 2
-
-@mock.patch('aws_lambda_powertools.tracing.Tracer.patch')
+@mock.patch("aws_lambda_powertools.tracing.Tracer.patch")
 def test_tracer_no_autopatch(patch_mock):
     # GIVEN tracer is instantiated
     # WHEN auto_patch is disabled
     # THEN tracer should not patch any module
-    tracer_a = Tracer(disabled=True, auto_patch=False)
-    tracer_b = tracer_a.instance()
-
+    Tracer(disabled=True, auto_patch=False)
     assert patch_mock.call_count == 0