newrelic-python-agent icon indicating copy to clipboard operation
newrelic-python-agent copied to clipboard

NewRelicContextFormatter update to support adding a stack trace

Open fritzdj opened this issue 1 year ago • 3 comments

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

https://github.com/newrelic/newrelic-python-agent/issues/1169

fritzdj avatar Jun 25 '24 13:06 fritzdj

CLA assistant check
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.

:white_check_mark: TimPansino
:white_check_mark: fritzdj
:x: mergify[bot]
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Jun 25 '24 13:06 CLAassistant

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.

TimPansino avatar Jun 27 '24 18:06 TimPansino

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

fritzdj avatar Jun 28 '24 03:06 fritzdj

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...

rafaelclp avatar Jul 17 '24 20:07 rafaelclp