NewRelicContextFormatter update to support adding a stack trace #1168

fritzdj · 2024-06-25T13:15:04Z

Overview

Currently the NewRelicContextFormatter implementation doesn't support including the stack trace. It would be convenient if there was an option because it would allow stack frames to be included as an attribute for an log in NR (instead of them being broken up by frame, which is the default if structured logs are not used). This PR is adding opt-in support for including stack traces.

Related Issue

#1169

CLAassistant · 2024-06-25T13:15:11Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ TimPansino
✅ fritzdj
❌ mergify[bot]
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

fritzdj · 2024-06-25T13:16:05Z

newrelic/api/log.py

-    def __init__(self, *args, **kwargs):
-        super(NewRelicContextFormatter, self).__init__()
+    def __init__(self, *args, stack_trace_limit: Optional[int] = 0, **kwargs):
+        """
+        :param Optional[int] stack_trace_limit:
+            Specifies the maximum number of frames to include for stack traces.
+            Defaults to `0` to suppress stack traces.
+            Setting this to `None` will make it so all available frames are included.
+        """
+        super(NewRelicContextFormatter, self).__init__(*args, **kwargs)


Reviewer note: *args and **kwargs were not being passed through to logging.Formatter init. This might have been a bug.

That's likely true.

TimPansino · 2024-06-27T17:18:22Z

newrelic/api/log.py

+            formatted["error.expected"] = expected
+
+        if self._stack_trace_limit is None or self._stack_trace_limit > 0:
+            stack_trace: Optional[str]


Suggested change

stack_trace: Optional[str]

We still have Python 2 wheels at the moment, so this line is a syntax issue. Should be removed.

TimPansino · 2024-06-27T17:23:25Z

tests/agent_features/test_logs_in_context.py

+    assert filename.endswith("/test_logs_in_context.py")
+    assert isinstance(line_number, int)
+    assert isinstance(stack_trace, str)
+    assert stack_trace == expected_stack_trace


Suggested change

assert stack_trace == expected_stack_trace

assert stack_trace and stack_trace == expected_stack_trace

Same as above

TimPansino · 2024-06-27T17:24:08Z

tests/agent_features/test_logs_in_context.py

+    assert filename.endswith("/test_logs_in_context.py")
+    assert isinstance(line_number, int)
+    assert isinstance(stack_trace, str)
+    assert stack_trace == expected_stack_trace


Suggested change

assert stack_trace == expected_stack_trace

assert stack_trace and stack_trace == expected_stack_trace

I like this strategy for checking stack traces, but would like to make sure they're not None or empty just in case.

TimPansino · 2024-06-27T17:27:25Z

newrelic/api/log.py

-    def __init__(self, *args, **kwargs):
-        super(NewRelicContextFormatter, self).__init__()
+    def __init__(self, *args, stack_trace_limit: Optional[int] = 0, **kwargs):
+        """
+        :param Optional[int] stack_trace_limit:
+            Specifies the maximum number of frames to include for stack traces.
+            Defaults to `0` to suppress stack traces.
+            Setting this to `None` will make it so all available frames are included.
+        """
+        super(NewRelicContextFormatter, self).__init__(*args, **kwargs)


That's likely true.

TimPansino · 2024-06-27T18:01:08Z

newrelic/api/log.py


-    @classmethod
-    def log_record_to_dict(cls, record):
+        expected = is_expected_error(exc_info)


An issue here, we're taking log_record_to_dict() which is a classmethod and changing it to an instance method. This would be a breaking change to the public API.

The same goes for format_exc_info() changing from a top level function to an instance method.

We definitely have customers who use these functions, or at least log_record_to_dict() without instantiating a class for convenience.

I'll push a commit with my proposal, but I'd like to convert these both into class functions, and export the names to the top level of the module so they can be used like functions. I think passing in the stack trace limit as an argument to the functions should preserve the most backwards compatibility for other users.

TimPansino · 2024-06-27T18:02:21Z

newrelic/api/log.py

@@ -69,11 +55,46 @@ def safe_json_encode(obj, ignore_string_types=False, **kwargs):
 class NewRelicContextFormatter(logging.Formatter):
    DEFAULT_LOG_RECORD_KEYS = frozenset(set(vars(logging.LogRecord("", 0, "", 0, "", (), None))) | {"message"})

-    def __init__(self, *args, **kwargs):
-        super(NewRelicContextFormatter, self).__init__()
+    def __init__(self, *args, stack_trace_limit: Optional[int] = 0, **kwargs):


Suggested change

def __init__(self, *args, stack_trace_limit: Optional[int] = 0, **kwargs):

def __init__(self, *args, stack_trace_limit=0, **kwargs):

We still have Python 2 wheels at the moment, so this line is a syntax issue. Should be removed.

Actually having a default keyword argument after *args isn't supported in Python 2 at all, I'll remove it for now like we do for our trace APIs and just attempt to pop it off the kwargs list if it exists.

TimPansino · 2024-06-27T18:38:03Z

I've several pushed commits for both @fritzdj and our team to review. Here's the summary of my changes.

Python 2 issues in the code and tests fixed with transparent changes.
The public APIs for log_record_to_dict() and format_exc_info were originally changed in a backwards incompatible manner.
- These have been changed to be class methods of the NewRelicContextFormatter to maintain compatibility, and exported as top level names in the newrelic.api.log module for the same reason.
- As part of this change, stack_trace_limit was added as a function argument to both of these APIs and will be supplied automatically by calls to format() if using the Formatter as an object, or can be supplied otherwise.

I think this will achieve the most flexible compatibility possible with existing customer uses, while making this feature possible. It's worth noting that in all cases the default behavior will remain unchanged, where no stack traces are attached.

fritzdj · 2024-06-28T03:06:51Z

Thanks @TimPansino, these changes look good to me! Good catches on the breaking change to the public API and Python 2 compatibility issues.

rafaelclp · 2024-07-17T20:20:18Z

We extend the NewRelicContextFormatter to add more data to the log record dict:

class OurNewRelicContextFormatter(NewRelicContextFormatter):
    @classmethod
    def log_record_to_dict(cls, record) -> dict[str, Any]:
        log_dict = super().log_record_to_dict(record)
        # ... add more stuff to log_dict here ...
        return log_dict

When we updated to newrelic==9.12, we started getting errors because log_record_to_dict "takes 2 positional arguments but 3 were given".

We already fixed this from our side, by taking in *args, **kwargs and redirecting them to the super call, but wondering if we are the only ones affected...

fritzdj added 2 commits June 25, 2024 08:47

NewRelicContextFormatter update to support adding a stack trace

7cc3ef3

Adding test coverage

03b7c20

fritzdj requested a review from a team June 25, 2024 13:15

fritzdj commented Jun 25, 2024

View reviewed changes

fritzdj mentioned this pull request Jun 25, 2024

Update NewRelicContextFormatter to support including stack traces #1169

Closed

TimPansino added 4 commits June 27, 2024 11:19

Remove typing for Python 2 compatibility

5aa8883

Refactor instance methods to class methods and export

f4a84a5

Add truthy safeguard to log tests

2faa80f

Fix Python 2 issues in tests

4d594e5

TimPansino reviewed Jun 27, 2024

View reviewed changes

mergify bot added tests-failing and removed tests-failing labels Jun 27, 2024

mergify bot added 2 commits June 27, 2024 21:01

Merge branch 'main' into main

a3aa8d8

Merge branch 'main' into main

f03c1bc

Merge branch 'main' into main

96e745c

mergify bot added the tests-failing label Jul 3, 2024

fritzdj and others added 2 commits July 8, 2024 14:11

lint:fix

817e97a

Merge branch 'main' into main

d6cd56f

mergify bot removed the tests-failing label Jul 10, 2024

mergify bot added 2 commits July 10, 2024 21:53

Merge branch 'main' into main

35185d6

Merge branch 'main' into main

8a49b1b

lrafeei approved these changes Jul 11, 2024

View reviewed changes

mergify bot added the tests-failing label Jul 11, 2024

lrafeei merged commit 47581e9 into newrelic:main Jul 11, 2024
45 of 49 checks passed

mergify bot removed the tests-failing label Jul 11, 2024

hmstepanek added this to the v9.12.0 milestone Jul 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NewRelicContextFormatter update to support adding a stack trace #1168

NewRelicContextFormatter update to support adding a stack trace #1168

fritzdj commented Jun 25, 2024 •

edited

Loading

CLAassistant commented Jun 25, 2024 •

edited

Loading

fritzdj Jun 25, 2024

TimPansino Jun 27, 2024

TimPansino Jun 27, 2024

TimPansino Jun 27, 2024

TimPansino Jun 27, 2024

TimPansino Jun 27, 2024

TimPansino Jun 27, 2024

TimPansino Jun 27, 2024

TimPansino Jun 27, 2024

TimPansino commented Jun 27, 2024

fritzdj commented Jun 28, 2024

rafaelclp commented Jul 17, 2024 •

edited

Loading

	assert stack_trace == expected_stack_trace
	assert stack_trace and stack_trace == expected_stack_trace

	def __init__(self, args, stack_trace_limit: Optional[int] = 0, *kwargs):
	def __init__(self, args, stack_trace_limit=0, *kwargs):

NewRelicContextFormatter update to support adding a stack trace #1168

NewRelicContextFormatter update to support adding a stack trace #1168

Conversation

fritzdj commented Jun 25, 2024 • edited Loading

Overview

Related Issue

CLAassistant commented Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimPansino commented Jun 27, 2024

fritzdj commented Jun 28, 2024

rafaelclp commented Jul 17, 2024 • edited Loading

fritzdj commented Jun 25, 2024 •

edited

Loading

CLAassistant commented Jun 25, 2024 •

edited

Loading

rafaelclp commented Jul 17, 2024 •

edited

Loading