feat(agent): add additional arguments as error attributes for newrelic_notice_error #942

hahuja2 · 2024-07-18T21:35:21Z

This PR does the following:

Create new function called nr_txn_record_error_with_additional_attributes, which takes in the additional parameters that are passed in newrelic_notice_error and adds them as error attributes.
Add unit tests to test new functions
Add integration tests to verify additional parameters are showing up as error attributes

codecov-commenter · 2024-07-18T21:59:34Z

Codecov Report

Attention: Patch coverage is 94.56522% with 5 lines in your changes missing coverage. Please review.

Project coverage is 78.17%. Comparing base (06824b7) to head (fa99aeb).
Report is 4 commits behind head on dev.

Files	Patch %	Lines
axiom/nr_errors.c	85.29%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #942      +/-   ##
==========================================
- Coverage   78.56%   78.17%   -0.40%     
==========================================
  Files         193      194       +1     
  Lines       27294    26911     -383     
==========================================
- Hits        21444    21038     -406     
- Misses       5850     5873      +23

Flag	Coverage Δ
agent-for-php-7.0	`?`
agent-for-php-7.1	`?`
agent-for-php-7.2	`78.18% <94.56%> (+0.01%)`	⬆️
agent-for-php-7.3	`78.20% <94.56%> (+0.01%)`	⬆️
agent-for-php-7.4	`77.90% <94.56%> (+0.01%)`	⬆️
agent-for-php-8.0	`77.92% <94.50%> (+0.01%)`	⬆️
agent-for-php-8.1	`77.91% <94.50%> (+0.01%)`	⬆️
agent-for-php-8.2	`77.51% <94.50%> (+0.01%)`	⬆️
agent-for-php-8.3	`77.51% <94.50%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ZNeumann · 2024-07-22T19:39:44Z

agent/php_api.c

+    char* buf1 = nr_strndup(errormsgstr, errormsglen);
+    char* buf2 = nr_strndup(error_file, error_file_len);
+    char* buf3 = nr_strndup(error_context, error_context_len);
+    char* stack_json = nr_php_backtrace_to_json(NULL TSRMLS_CC);
+
+    nr_txn_record_error_with_additional_attributes(
+        NRPRG(txn), priority, true, buf1, errclass, buf2,
+        error_line, buf3, error_number, stack_json);
+
+    nr_free(buf1);
+    nr_free(buf2);
+    nr_free(buf3);
+    nr_free(stack_json);


What is the purpose of copying these strings only to immediately free them? Is there a reason why we cannot just directly pass in errormsgstr, error_file, and error_context?

Good point, fixed: b8e6402

ZNeumann · 2024-07-22T19:43:40Z

axiom/nr_errors.c

+  nr_free(error->message);
+  nr_free(error->klass);
+  nr_free(error->error_file);
+  nr_free(error->error_context);
+  nr_free(error->span_id);
+  nr_free(error->stacktrace_json);
+  nr_realfree((void**)error_ptr);


Based on the name of this function, I would expect it to only destroy the additional params, and thus still require the calling of nr_error_destroy afterward to fully free the error.

To reduce code duplication, I have removed this function and added the additional logic in nr_error_destroy inside of a if statement: f917ced

ZNeumann · 2024-07-22T19:49:53Z

axiom/nr_errors_private.h

+  char* error_file;      /* Error file */
+  int error_line;        /* Error line */
+  char* error_context;   /* Error context */
+  int error_no;          /* Error number */
+  int option;            /* Error option */


As far as I can tell, these are specifically user-provided through the API. It would be useful to specify that in the name of the variables, or at least their comment.

Good point, I have updated the comments: 9f3e7e6

ZNeumann · 2024-07-22T19:53:48Z

axiom/nr_errors_private.h

+  int error_line;        /* Error line */
+  char* error_context;   /* Error context */
+  int error_no;          /* Error number */
+  int option;            /* Error option */


So am I correct in that the option variable indicates whether the other user-provided variables are populated? Could we not just check, for example, error_file for null behind an API like nr_error_has_user_attributes rather than having this confusing variable? As far as I can tell, these attributes are either all-or-nothing.

Agree, the option variable isn't necessary and I have removed it: 9f3e7e6

ZNeumann · 2024-07-22T19:58:23Z

axiom/nr_txn.c

@@ -1638,6 +1641,109 @@ void nr_txn_record_error(nrtxn_t* txn,
  nr_free(span_id);
 }

+void nr_txn_record_error_with_additional_attributes(


There is a large amount of code duplication in this API. It would be nice to create a more unified API such that we don't have multiple copies of large functions.

Good point, I have added helper functions for nr_txn_record_error, nr_segment_set_error, and nr_error_create to reduce code duplication: 23823f9 and f917ced

Could you remove nr_txn_record_error_helper and then just have nr_txn_record_error call nr_txn_record_error_with_additional_attributes with the unused fields filled with nothing values?

Good idea, I have removed all of the helper functions: e177f40, ed191c0, 788c5a0

newrelic-php-agent-bot · 2024-07-24T21:14:42Z

Test Suite	Status	Result
Multiverse	✅	7/7 passing
SOAK	❌	55/56 passing

mfulb · 2024-07-25T20:19:26Z

agent/php_api.c

-                                      ZEND_NUM_ARGS() TSRMLS_CC, "lsslz!",
-                                      &ignore1, &errormsgstr, &errormsglen,
-                                      &ignore2, &ignore3, &ignore4, &ignore5)) {
+                                      ZEND_NUM_ARGS() TSRMLS_CC, "lssls!",


The format for the parameters changes so the last value is a 's' instead of 'z' - what is the rationale behind this change?

This has been changed to a 's' because in the docs API, it mentions that error_context is a string which is the 5th parameter for this function.

mfulb · 2024-07-25T20:21:57Z

agent/php_api.c

@@ -168,13 +172,23 @@ PHP_FUNCTION(newrelic_notice_error) {
    }
  }

-  {
-    char* buf = nr_strndup(errormsgstr, errormsglen);
+  if (!determine) {


This code seems to be able to be reduced as there are common elements to each possible path - also use an else instead of having two separate if statements testing logically opposite conditions.

Fixed: 66a069d

mfulb · 2024-07-25T20:23:34Z

axiom/nr_errors.c

                            const char* stacktrace_json,
                            const char* span_id,
                            nrtime_t when) {
  nr_error_t* error;

-  if (0 == message) {


Why were these guards removed?

These were removed because these guards are being checked before this helper function is called. An example of this is in nr_error_create.

mfulb · 2024-07-25T20:24:53Z

axiom/nr_errors.c

  error = (nr_error_t*)nr_zalloc(sizeof(nr_error_t));
  error->priority = priority;
  error->when = when;
  error->message = nr_strdup(message);
  error->klass = nr_strdup(klass);
  error->stacktrace_json = nr_strdup(stacktrace_json);
+  if (NULL != error_file && NULL != error_context) {


If only one of these two values is defined do we still want to create the associated field?

Is there a value for the error_line and error_no that indicates it is not defined that needs to be checked?

After discussion, it has been decided that if one attribute is passed in as NULL, the rest of the attributes should still show up. I will create a commit for this.

I have added an if statement for each separate value to ensure all values that are defined will be reported: 788c5a0

mfulb · 2024-07-25T20:27:50Z

axiom/nr_errors.c

+                                              const char* stacktrace_json,
+                                              const char* span_id,
+                                              nrtime_t when) {
+  if (0 == message || 0 == klass || 0 == error_file || 0 == error_context


Please change usages in this file of 0 to NULL if used to test a pointer value. The usage of 0 is from old code and we do not use that form now.

Fixed: ace780f

mfulb · 2024-07-25T20:29:01Z

axiom/nr_errors.c

@@ -94,6 +153,10 @@ void nr_error_destroy(nr_error_t** error_ptr) {
  nr_free(error->klass);
  nr_free(error->span_id);
  nr_free(error->stacktrace_json);
+  if ((NULL != error->error_file) && (NULL != error->error_context)) {


nr_free should handle this case.

Fixed: c058df8

mfulb · 2024-07-25T20:44:14Z

axiom/nr_errors.h

+ *           8. String containing stack trace in JSON format.
+ *           9. Span ID
+ *           10. When the error occurred.
+ *


Would be good to explain if all are required or if there are certain combinations allowed.

Added: 9fad7fa

mfulb · 2024-07-25T20:44:45Z

axiom/nr_errors.h

+/*
+ * Purpose : Get the error file of an error.
+ *
+ * Returns : The error file of the error or NULL on failure.


Is it a 'failure' or simply it is not defined?

Good point, it should instead say that it was not defined, fixed: 9fad7fa

mfulb · 2024-07-25T20:47:35Z

axiom/nr_segment.c

  if (NULL == segment->error) {
    segment->error = nr_zalloc(sizeof(nr_segment_error_t));
  }

  nr_free(segment->error->error_message);
  nr_free(segment->error->error_class);
+  if (NULL != error_file && NULL != error_context) {


Couldn't this code be pulled into the conditional below which has the same check?

I have removed the if statement, so that nr_free() is still called: ed191c0

mfulb · 2024-07-26T13:30:32Z

axiom/nr_segment.c


  segment->error->error_message
      = error_message ? nr_strdup(error_message) : NULL;
  segment->error->error_class = error_class ? nr_strdup(error_class) : NULL;
+  if (NULL != error_file && NULL != error_context) {


I'm not clear on the behavior of this function if the segment had all fields defined previously and then this function is called and error_file and error_context are NULL. It seems the previous values would be retained. That could be confusing as it seems there is no way to clear those fields once set.

After discussion, it has been decided that if NULL is passed in, the attribute should not show up. I will modify the code and create a commit for this.

I have modified this function where all strings are freed now, so the previous values will not be retained. New values will only be added if they are not NULL: ed191c0, bc0a82e

mfulb · 2024-07-26T13:32:24Z

axiom/nr_segment.c

-    return;
-  }
-
+static void nr_segment_set_error_helper(nr_segment_t* segment,


I would recommend putting a comment on each of these static helper functions throughout this file to make it clear no NULL checking is done. It is unlikely any new code would call these directly but I think it is a good safety guard to make sure a coder is aware of this.

Do you think we need unit tests for these helper functions throughout this file?

I have modified the code and removed all of the helper functions: e177f40, ed191c0, 788c5a0

mfulb · 2024-07-26T13:32:47Z

axiom/nr_segment.h

@@ -117,6 +117,10 @@ typedef struct _nr_segment_metric_t {
 typedef struct _nr_segment_error_t {
  char* error_message; /* The error message that will appear on a span event. */
  char* error_class;   /* The error class that will appear on a span event. */
+  char* error_file;


Can you add a comment for each of these fields like the existing ones have?

Added: eb64129

mfulb · 2024-07-26T14:07:08Z

axiom/nr_txn.c

@@ -2479,6 +2536,13 @@ nr_analytics_event_t* nr_error_to_event(const nrtxn_t* txn) {
  nro_set_hash_string(params, "error.class", nr_error_get_klass(txn->error));
  nro_set_hash_string(params, "error.message",
                      nr_error_get_message(txn->error));
+  if (nr_error_get_file(txn->error) && nr_error_get_context(txn->error)) {


Is it ever possible we'd only one to write a subset of these 4 or do they either all exist or not exist?

After discussion, it has been decided that the values that are defined should still be reported. I have modified the code to check each value individually in separate if statements: e177f40

mfulb · 2024-07-26T14:08:08Z

axiom/nr_txn.h

@@ -471,6 +471,17 @@ extern nr_status_t nr_txn_record_error_worthy(const nrtxn_t* txn, int priority);
 #define NR_TXN_ALLOW_RAW_EXCEPTION_MESSAGE \
  "Message removed by New Relic security settings"

+extern void nr_txn_record_error_with_additional_attributes(nrtxn_t* txn,


Could you document the record error functions (both forms)?

Added: c5a40cc

mfulb · 2024-07-26T14:13:17Z

axiom/tests/test_segment.c

@@ -2868,6 +2874,12 @@ static void test_segment_set_error_attributes(void) {
  tlib_pass_if_str_equal("error.class", "error.class 1",
                         segment.error->error_class);

+  nr_segment_set_error_with_additional_params(&segment, "error.message", "error.class", "error.file", 125, "rand3", 100);


Some tests exercising the additional new params which are pointers with NULL values would be good. Because there are short circuiting conditions when checking for NULL I would recommend exercising each param individually with a NULL instead of a single test setting them all to NULL.

Good point, added: 06430a2

mfulb · 2024-07-26T14:14:27Z

axiom/tests/test_segment.c

@@ -2868,6 +2874,12 @@ static void test_segment_set_error_attributes(void) {
  tlib_pass_if_str_equal("error.class", "error.class 1",
                         segment.error->error_class);

+  nr_segment_set_error_with_additional_params(&segment, "error.message", "error.class", "error.file", 125, "rand3", 100);


What is the expected behavior if any of the strings are ""?

If the user passes in an empty string, then the attribute will still show up. The attribute will not show up if NULL is passed in.

Co-authored-by: Michael Fulbright <[email protected]>

mfulb · 2024-09-11T18:48:58Z

PR #960 supersedes the work in this PR. The conclusion was that the agent spec does not support these current parameters currently as attributes, and much of the information is available from the stack trace or by adding the desired information to the error message passed to the API.

hahuja2 marked this pull request as ready for review July 18, 2024 22:28

hahuja2 changed the title ~~feat(agent): handle additional arguments correctly for newrelic_notice_error~~ feat(agent): add additional arguments as error attributes for newrelic_notice_error Jul 18, 2024

hahuja2 requested review from ZNeumann, zsistla, mfulb, lavarou and bduranleau-nr July 18, 2024 22:38

ZNeumann reviewed Jul 22, 2024

View reviewed changes

mfulb reviewed Jul 25, 2024

View reviewed changes

mfulb reviewed Jul 26, 2024

View reviewed changes

hahuja2 and others added 27 commits September 6, 2024 16:41

feat(agent): handle additional args for newrelic_notice_error

e4e0491

chore(axiom): add unit tests

addcdec

chore(axiom): add integration tests

b7fe442

chore(axiom): format function

09648a4

chore(agent): remove unnecessary use of nr_strdup

449c525

chore(agent): add helper function for nr_error_create

a232371

chore(axiom): reduce code duplication

382c5d9

chore(axiom): update description and remove option variable

20b568b

chore(axiom): remove unnecessary function

3481c07

Update axiom/tests/test_txn.c

93d9f8b

Co-authored-by: Michael Fulbright <[email protected]>

chore(axiom): add additional parameters as user attributes

08c4261

chore(agent): simplify if statement

2da5096

chore(axiom): remove helper function and add if statements

524754d

chore(axiom): change usage of 0 to NULL

2017e8e

chore(axiom): remove if check

28b6f33

chore(axiom): update comments

92b0d2d

chore(axiom): remove helper function and add additional if statements

1ec5164

chore(axiom): fix duplication

cbe1fa2

chore(axiom): add comments

604762c

chore(axiom): allow NULL to be passed in as a parameter

a7a519c

chore(axiom): remove helper function and split up if statements

9f44c72

chore(axiom): add description

2c72637

chore(axiom): update unit tests

8099059

chore(axiom): update unit tests

62afe8a

chore(axiom): modify unit tests

785e83c

chore(tests): add integration tests

a47bf1d

chore(axiom): create nr_user_error_t struct

8b6a0e3

mfulb force-pushed the ha/update-notice-error branch from a2ad43b to 8b6a0e3 Compare September 6, 2024 20:41

mfulb closed this Sep 11, 2024

feat(agent): add additional arguments as error attributes for newrelic_notice_error #942

feat(agent): add additional arguments as error attributes for newrelic_notice_error #942

Uh oh!

Conversation

hahuja2 commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newrelic-php-agent-bot commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfulb Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfulb Jul 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hahuja2 commented Jul 18, 2024 •

edited

Loading

codecov-commenter commented Jul 18, 2024 •

edited

Loading

newrelic-php-agent-bot commented Jul 24, 2024 •

edited

Loading

mfulb Jul 25, 2024 •

edited

Loading

mfulb Jul 25, 2024 •

edited

Loading

hahuja2 Aug 26, 2024 •

edited

Loading