Skip to content

Fix notice_error logic for non-iterable exceptions. #1564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 3, 2025
Merged

Fix notice_error logic for non-iterable exceptions. #1564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
46f6aac
Fix notice_error logic for non-iterable exceptions.
umaannamalai Oct 31, 2025
71703bc
Merge branch 'main' into fix-notice-error-bug
umaannamalai Nov 3, 2025
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
13 changes: 10 additions & 3 deletions newrelic/api/time_trace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,16 +360,23 @@ def _observe_exception(self, exc_info=None, ignore=None, expected=None, status_c
return fullname, message, message_raw, tb, is_expected

def notice_error(self, error=None, attributes=None, expected=None, ignore=None, status_code=None):
def error_is_iterable(error):
return hasattr(error, "__iter__") and not isinstance(error, (str, bytes))

def none_in_error(error):
return error_is_iterable(error) and None in error

attributes = attributes if attributes is not None else {}

# If no exception details provided, use current exception.

# Pull from sys.exc_info if no exception is passed
if not error or None in error:
# Pull from sys.exc_info() if no exception is passed
# Check that the error exists and that it is a fully populated iterable
if not error or none_in_error(error) or (error and not error_is_iterable(error)):
Copy link
Contributor

@TimPansino TimPansino Nov 3, 2025

Choose a reason for hiding this comment

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

If the intent of this change is to make it possible to pass in an instance of BaseException to be able to report it then this change is overcomplicated and unfortunately looks to be introducing a bug.

Take for example this code:

try:
    raise ValueError()
except ValueError as exc1:
    pass

try:
    raise TypeError()
except TypeError as exc2:
    notice_error(exc1)

Previously the exception instance was not allowed as an input, and would have caused an error that would crash.

With this PR's changes instead, it silently reports the wrong exception. The user is trying to pass in an exception to report, but as written this will trigger a pull from sys.exc_info() which will yield back the tuple for exc2, instead of exc1 which was passed in.

There's also a lot of complexity in these if statements now that I don't care for, as well as a bunch of new function calls that add overhead. Instead, I propose we construct a tuple in the form we expect from the exception instance if and only if the error passed in is an instance of BaseException and it has a non None __traceback__ attribute.

error = sys.exc_info()

# If no exception to report, exit
if not error or None in error:
if not error or none_in_error(error) or (error and not error_is_iterable(error)):
Copy link
Contributor

@TimPansino TimPansino Nov 3, 2025

Choose a reason for hiding this comment

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

This line should only ever deal with the output of sys.exc_info, which is guaranteed to be a tuple of (None, None, None) or (exc, val, tb). This line is adding a bunch of unnecessary complexity when the original code here should have been fine.

return

exc, value, tb = error
Expand Down
12 changes: 9 additions & 3 deletions newrelic/core/stats_engine.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,9 +676,14 @@ def record_time_metrics(self, metrics):
self.record_time_metric(metric)

def notice_error(self, error=None, attributes=None, expected=None, ignore=None, status_code=None):
def error_is_iterable(error):
return hasattr(error, "__iter__") and not isinstance(error, (str, bytes))

def none_in_error(error):
return error_is_iterable(error) and None in error

attributes = attributes if attributes is not None else {}
settings = self.__settings

if not settings:
return

Expand All @@ -691,11 +696,12 @@ def notice_error(self, error=None, attributes=None, expected=None, ignore=None,
return

# Pull from sys.exc_info if no exception is passed
if not error or None in error:
# Check that the error exists and that it is a fully populated iterable
if not error or none_in_error(error) or (error and not error_is_iterable(error)):
error = sys.exc_info()

# If no exception to report, exit
if not error or None in error:
if not error or none_in_error(error) or (error and not error_is_iterable(error)):
return

exc, value, tb = error
Expand Down
19 changes: 19 additions & 0 deletions tests/agent_features/test_notice_error.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,25 @@

# =============== Test errors during a transaction ===============

_key_error_name = callable_name(KeyError)

_test_notice_error_exception_object = [(_key_error_name, "'f'")]


@validate_transaction_errors(errors=_test_notice_error_exception_object)
@background_task()
def test_notice_error_non_iterable_object():
"""Test that notice_error works when passed an exception object directly"""
try:
test_dict = {"a": 4, "b": 5}
# Force a KeyError
test_dict["f"]
except KeyError as e:
# The caught exception here is a non-iterable, singular KeyError object with no associated traceback
# This will exercise logic to pull from sys.exc_info() instead of using the exception directly
notice_error(e)


_test_notice_error_sys_exc_info = [(_runtime_error_name, "one")]


Expand Down
Loading