Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 20 additions & 10 deletions src/lumigo_tracer/auto_instrument_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ def parse_handler():
try:
module_name, unit_name = os.environ[ORIGINAL_HANDLER_KEY].rsplit(".", 1)
except KeyError:
raise ValueError(
raise Exception(
"Could not find the original handler. Please contact Lumigo for more information."
)
except ValueError:
raise RuntimeError(
f"Invalid handler format: Bad handler '{os.environ[ORIGINAL_HANDLER_KEY]}': not enough values to unpack (expected 2, got 1)"
)
) from None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you adding from None? The user will lose the context of the exception:
image

Same regarding the other from None is this file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I tried to copied AWS behavior:
image

except ValueError as e:
raise ValueError(
f"Runtime.MalformedHandlerName: Bad handler '{os.environ[ORIGINAL_HANDLER_KEY]}': {str(e)}"
) from None
importable_name = module_name.replace("/", ".")
return importable_name, unit_name

Expand All @@ -26,11 +26,21 @@ def _handler(*args, **kwargs):
handler_module = ""
try:
handler_module, unit_name = parse_handler()
original_handler = getattr(importlib.import_module(handler_module), unit_name)
except (ImportError, AttributeError):
original_module = importlib.import_module(handler_module)
except ImportError as e:
raise ImportError(
f"Unable to import module '{handler_module}': No module named '{handler_module}'"
)
f"Runtime.ImportModuleError: Unable to import module '{handler_module}': {str(e)}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you even add of original exception in the text. Don't you prefer to do from e instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to copy AWS's bootstrap default behavior:
image

) from None
except SyntaxError as e:
raise SyntaxError(
f"Runtime.UserCodeSyntaxError: Syntax error in module '{handler_module}': {str(e)}"
) from None
try:
original_handler = getattr(original_module, unit_name)
except AttributeError:
raise Exception(
f"Runtime.HandlerNotFound: Handler '{unit_name}' missing on module '{handler_module}'"
) from None
return original_handler(*args, **kwargs)


Expand Down
59 changes: 49 additions & 10 deletions src/test/unit/test_auto_instrument_handler.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,33 +32,72 @@ def test_hierarchy_happy_flow(monkeypatch):
def test_import_error(monkeypatch):
monkeypatch.setenv(ORIGINAL_HANDLER_KEY, "blabla.not.exists")

with pytest.raises(ImportError):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not do it with pytest.raises?
Same issue regarding the other tests in this file as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great question!
I did use pytest.raises in the first place, but then I noticed that it removes the context of the message, and I want to validate that there was no context (to follow AWS behavior!) :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment explaining why don't we use it

try:
_handler({}, {})
except ImportError as e:
# Note: We're not using pytest.raises in order to get the exception context
assert "Runtime.ImportModuleError" in str(e)
assert "another exception occurred" not in traceback.format_exc()
else:
assert False


def test_no_env_handler_error(monkeypatch):
if os.environ.get(ORIGINAL_HANDLER_KEY):
monkeypatch.delenv(ORIGINAL_HANDLER_KEY)
monkeypatch.delenv(ORIGINAL_HANDLER_KEY, None)

with pytest.raises(ValueError):
with pytest.raises(Exception) as e:
_handler({}, {})
assert "Could not find the original handler" in str(e.value)


def test_error_in_original_handler_no_extra_exception_log(monkeypatch, context):
monkeypatch.setattr(importlib, "import_module", mock.Mock(side_effect=Exception))
monkeypatch.setattr(importlib, "import_module", mock.Mock(side_effect=ZeroDivisionError))
monkeypatch.setenv(ORIGINAL_HANDLER_KEY, "sys.exit")

try:
_handler({}, context)
except ZeroDivisionError:
# Note: We're not using pytest.raises in order to get the exception context
assert "another exception occurred" not in traceback.format_exc()
else:
assert False


def test_error_in_original_handler_syntax_error(monkeypatch, context):
monkeypatch.setattr(importlib, "import_module", mock.Mock(side_effect=SyntaxError))
monkeypatch.setenv(ORIGINAL_HANDLER_KEY, "sys.exit")
exception_occurred = False

try:
_handler({}, context)
except Exception:
exception_occurred = True
except SyntaxError as e:
# Note: We're not using pytest.raises in order to get the exception context
assert "Runtime.UserCodeSyntaxError" in str(e)
assert "another exception occurred" not in traceback.format_exc()
assert exception_occurred is True
else:
assert False


def test_handler_bad_format(monkeypatch):
monkeypatch.setenv(ORIGINAL_HANDLER_KEY, "no_method")

with pytest.raises(RuntimeError):
try:
_handler({}, {})
except ValueError as e:
# Note: We're not using pytest.raises in order to get the exception context
assert "Runtime.MalformedHandlerName" in str(e)
assert "another exception occurred" not in traceback.format_exc()
else:
assert False


def test_handler_not_found(monkeypatch):
monkeypatch.setenv(ORIGINAL_HANDLER_KEY, "sys.not_found")

try:
_handler({}, {})
except Exception as e:
# Note: We're not using pytest.raises in order to get the exception context
assert "Runtime.HandlerNotFound" in str(e)
assert "another exception occurred" not in traceback.format_exc()
else:
assert False