feat(agent): Adds label forwarding to log events #1027

mfulb · 2025-02-20T21:25:20Z

This PR adds the ability for labels to be forwarded with any log messages forwarded by the PHP agent.

newrelic-php-agent-bot · 2025-02-20T21:25:46Z

Test Suite	Status	Result
Multiverse	❌	0/8 passing
SOAK	❌	74/79 passing

codecov-commenter · 2025-02-24T22:02:50Z

Codecov Report

Attention: Patch coverage is 95.94595% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.98%. Comparing base (f02c410) to head (65f9ba2).

Files with missing lines	Patch %	Lines
agent/php_txn.c	96.36%	2 Missing ⚠️
axiom/cmd_txndata_transmit.c	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1027      +/-   ##
==========================================
+ Coverage   77.93%   77.98%   +0.04%     
==========================================
  Files         198      198              
  Lines       27713    27785      +72     
==========================================
+ Hits        21599    21669      +70     
- Misses       6114     6116       +2

Flag	Coverage Δ
agent-for-php-7.2	`78.14% <95.94%> (+0.05%)`	⬆️
agent-for-php-7.3	`78.16% <95.94%> (+0.05%)`	⬆️
agent-for-php-7.4	`77.87% <95.94%> (+0.05%)`	⬆️
agent-for-php-8.0	`77.26% <95.94%> (+0.05%)`	⬆️
agent-for-php-8.1	`77.76% <95.94%> (+0.05%)`	⬆️
agent-for-php-8.2	`?`
agent-for-php-8.3	`77.37% <95.94%> (+0.05%)`	⬆️
agent-for-php-8.4	`77.39% <95.94%> (+0.05%)`	⬆️

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.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

agent/php_txn.c

agent/scripts/newrelic.ini.template

lavarou · 2025-02-26T14:37:44Z

tests/integration/logging/monolog2/test_monolog_labels_forwarding_exclude_05.php

+      "common": {
+        "attributes": { }
+      },


For other reviewers - sending "common" object with empty "attributes" is an existing functionality.

axiom/tests/.gitignore

agent/php_txn.c

zsistla · 2025-02-27T14:53:56Z

Thanks for adding the new tests_monolog_label* tests.
I did a first look through and overall they look good.
A few questions/requests:

Could you add additional tests where:

Labels list is not set, but exclude log labels are
Log forwarding disabled, but exclude log labels are set (I.e., not default)
Exclude label list includes a label that matches multiple case sensitive labels; expected behavior is they are all removed.
Exclude label list has same entry more than once

Did you run the new integration/multiverse label tests with valgrind?
What is expected behavior if a label has a comma? Can you add a test case, so we can document the expected behavior that way?

The function nr_txn_begin() now accepts a nrobj_t *log_labels parameter which contains the logging labels to be associated with log events.

Co-authored-by: Michal Nowacki <[email protected]>

agent/php_txn.c

zsistla · 2025-02-27T23:27:49Z

agent/php_txn.c

+    if (!exclude) {
+      nro_set_hash_string(log_labels, key, value);
+    } else {
+      nrl_verbosedebug(NRL_TXN, "%s: Excluding label %s", __FUNCTION__, key);


Key was not checked for NULL, this could generate a segfault. Check for NULL earlier, and/or wrap in NRSAFESTR.

I've added the NRSAFESTR macro just in case although with the change in c13d6c0 the value of key should not be NULL here.
See a238c67

agent/php_txn.c

zsistla · 2025-02-27T23:46:41Z

agent/php_txn.c

+                     __FUNCTION__);
+    return NULL;
+  }
+


A check if log_forwarding_labels_exclude is an empty string (most common case) would allow an early exit from the function.

If the exclude rule is empty we need to copy all the labels into the log labels structure so I don't see how we could exit early.

Why do we need to copy all the log labels if there are no excludes?
We know that we want to include everything, so if the exclude list is the an empty string, couldn't we just could exit early here and return labels?

daemon/internal/newrelic/log_events_test.go

protocol/flatbuffers/protocol.fbs

lavarou · 2025-03-06T16:16:19Z

agent/php_txn.c

+  log_labels = nro_new(NR_OBJECT_HASH);
+  for (int i = 0; i < nro_getsize(label_keys); i++) {
+    const char* key = nro_get_array_string(label_keys, i + 1, NULL);
+    const char* value = nro_get_hash_string(labels, key, NULL);


The value is not needed until it is certain the label has not been filtered out. If it is filtered out, there's no need to retrieve it.

Thank you for this observation - I've incorporated this into 90955f0

lavarou · 2025-03-06T16:22:10Z

axiom/nr_txn.h

+                             const nr_attribute_config_t* attribute_config,
+                             const nrobj_t* log_forwarding_labels);


These two arguments are not documented by doc string. I don't know if this is something we care about or not.

Good point - I addressed this omission with 7a02270

lavarou

Looks good considering all limitations of agent<->daemon/integration_runner design.

Takes advantage of return value from nrp_get_hash_boolean() being -1 if the key lookup did not suceed to simplify this section of code. Also is smarter about retrieving data until it is known it is needed.

Checks if log forwarding is disabled then log forwarding labels is also disabled, even if the log forwarding labels config is set to true.

mfulb · 2025-03-06T20:37:46Z

Thanks for adding the new tests_monolog_label* tests. I did a first look through and overall they look good. A few questions/requests:

Could you add additional tests where:

Labels list is not set, but exclude log labels are

Log forwarding disabled, but exclude log labels are set (I.e., not default)

Exclude label list includes a label that matches multiple case sensitive labels; expected behavior is they are all removed.

Exclude label list has same entry more than once

Did you run the new integration/multiverse label tests with valgrind?

What is expected behavior if a label has a comma? Can you add a test case, so we can document the expected behavior that way?

I think commit 65f9ba2 will address your points 1 and 3 above.

I did run the integration tests (no multiverse tests for log labels (yet)) with valgrind. A couple of CLM related tests passed valgrind but failed because an additional span ("name": "Custom/Monolog\Logger::pushHandler") was detected. I'm suspecting this is because it ran slower with valgrind than w/o so it was included whereas it is not when valgrind is not used?

zsistla · 2025-03-07T12:28:59Z

65f9ba2

Awesome, thank you!

mfulb changed the base branch from main to dev February 20, 2025 21:25

mfulb requested review from bduranleau-nr and lavarou February 25, 2025 15:29

mfulb marked this pull request as ready for review February 25, 2025 15:29

zsistla reviewed Feb 25, 2025

View reviewed changes

agent/php_txn.c Show resolved Hide resolved

mfulb force-pushed the feat/apm_logging_labels branch from bc18e12 to df096d7 Compare February 25, 2025 18:06

lavarou reviewed Feb 26, 2025

View reviewed changes

agent/scripts/newrelic.ini.template Outdated Show resolved Hide resolved

lavarou reviewed Feb 26, 2025

View reviewed changes

agent/scripts/newrelic.ini.template Outdated Show resolved Hide resolved

lavarou reviewed Feb 26, 2025

View reviewed changes

axiom/tests/.gitignore Outdated Show resolved Hide resolved

zsistla reviewed Feb 27, 2025

View reviewed changes

agent/php_txn.c Show resolved Hide resolved

mfulb added 16 commits February 27, 2025 14:23

feat(agent): Adds INI values for labels for forwarded logs

012793d

feat(agent): Adds processing of log label from labels

6c84bb9

feat(agent): Label forwarding for APM logs

f38315d

feat(agent): Adds log forwarding labels supportability metric

78c8388

tests(logging): Adds Monolog3 tests for log forwarding labels

520169e

chore(tests): Removes unneeded spaces

9a3cd24

chore(daemon): Remove debugging statement

5983bc0

tests(daemon): Add simple tests for LogEvents

b8c415a

chore(daemon): Removes unneeded import

ed8cada

chore(daemon): Logs unmarshall error

6c0b65f

tests(daemon): Adds log forwarded label to test

0672ac0

fix(agent): Fixes log forwarded labels definition

5b23982

fix(agent): Fixes encoding of forwarded log labels to an Event

8186043

tests(agent): Adds test for encoding forwarded log labels

993403b

fix(agent): Updates function call for nr_txn_begin()

b5616d0

The function nr_txn_begin() now accepts a nrobj_t *log_labels parameter which contains the logging labels to be associated with log events.

fix(agent): Fixes decoding of forwarded log labels as an Event

bea9a98

mfulb added 2 commits February 27, 2025 14:23

tests(agent): Adds another test for log forwarded labels using defaults

ee16060

fix(agent): Restores gitignore line for test_segment_message

c9bea37

mfulb force-pushed the feat/apm_logging_labels branch from df096d7 to c9bea37 Compare February 27, 2025 19:26

mfulb and others added 2 commits February 27, 2025 14:32

Update agent/scripts/newrelic.ini.template

192e04e

Co-authored-by: Michal Nowacki <[email protected]>

chore(agent): Improves INI description for log labels exclude directive

277d034

zsistla reviewed Feb 27, 2025

View reviewed changes

agent/php_txn.c Outdated Show resolved Hide resolved

zsistla reviewed Feb 27, 2025

View reviewed changes

agent/php_txn.c Outdated Show resolved Hide resolved

zsistla reviewed Feb 27, 2025

View reviewed changes

agent/php_txn.c Outdated Show resolved Hide resolved

zsistla reviewed Feb 27, 2025

View reviewed changes

zsistla reviewed Feb 28, 2025

View reviewed changes

daemon/internal/newrelic/log_events_test.go Outdated Show resolved Hide resolved

zsistla reviewed Feb 28, 2025

View reviewed changes

protocol/flatbuffers/protocol.fbs Outdated Show resolved Hide resolved

mfulb commented Mar 6, 2025

View reviewed changes

protocol/flatbuffers/protocol.fbs Outdated Show resolved Hide resolved

Update protocol/flatbuffers/protocol.fbs

467c8a2

lavarou reviewed Mar 6, 2025

View reviewed changes

lavarou previously approved these changes Mar 6, 2025

View reviewed changes

mfulb added 2 commits March 6, 2025 12:23

tests(daemon): Improves log labels ingest unit tests

ee9a5a4

fix(daemon): Verify log labels are correct format

8d46a55

mfulb dismissed lavarou’s stale review via 8d46a55 March 6, 2025 17:24

mfulb added 8 commits March 6, 2025 12:35

chore(docs): Improves api doc for nr_txn_begin()

7a02270

fix(agent): Do not all log labels with a NULL key

c13d6c0

fix(agent): Protects against possiblity key is NULL

a238c67

chore(agent): Refactors log label exclusion logic

90955f0

Takes advantage of return value from nrp_get_hash_boolean() being -1 if the key lookup did not suceed to simplify this section of code. Also is smarter about retrieving data until it is known it is needed.

tests(agent): Adds check for log forwarding labels config

67c5348

Checks if log forwarding is disabled then log forwarding labels is also disabled, even if the log forwarding labels config is set to true.

tests(agent): Adds unit tests for log label creation

a3aaad0

chore: Fix formatting

47df6f2

tests(agent): Adds some more test cases for log labels

65f9ba2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(agent): Adds label forwarding to log events #1027

feat(agent): Adds label forwarding to log events #1027

mfulb commented Feb 20, 2025

newrelic-php-agent-bot commented Feb 20, 2025 •

edited

Loading

codecov-commenter commented Feb 24, 2025 •

edited

Loading

lavarou Feb 26, 2025

zsistla commented Feb 27, 2025 •

edited

Loading

zsistla Feb 27, 2025 •

edited

Loading

mfulb Mar 6, 2025

zsistla Feb 27, 2025

mfulb Mar 6, 2025

zsistla Mar 7, 2025 •

edited

Loading

lavarou Mar 6, 2025

mfulb Mar 6, 2025

lavarou Mar 6, 2025

mfulb Mar 6, 2025

lavarou left a comment

mfulb commented Mar 6, 2025 •

edited

Loading

zsistla commented Mar 7, 2025

		const nr_attribute_config_t* attribute_config,
		const nrobj_t* log_forwarding_labels);

feat(agent): Adds label forwarding to log events #1027

Are you sure you want to change the base?

feat(agent): Adds label forwarding to log events #1027

Conversation

mfulb commented Feb 20, 2025

newrelic-php-agent-bot commented Feb 20, 2025 • edited Loading

codecov-commenter commented Feb 24, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

zsistla commented Feb 27, 2025 • edited Loading

zsistla Feb 27, 2025 • 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

zsistla Mar 7, 2025 • 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

lavarou left a comment

Choose a reason for hiding this comment

mfulb commented Mar 6, 2025 • edited Loading

zsistla commented Mar 7, 2025

newrelic-php-agent-bot commented Feb 20, 2025 •

edited

Loading

codecov-commenter commented Feb 24, 2025 •

edited

Loading

zsistla commented Feb 27, 2025 •

edited

Loading

zsistla Feb 27, 2025 •

edited

Loading

zsistla Mar 7, 2025 •

edited

Loading

mfulb commented Mar 6, 2025 •

edited

Loading