Skip to content

Commit

Permalink
fix(agent): Improves decoration function handling
Browse files Browse the repository at this point in the history
The decorating processor function was being created multiple times if
more than one handler was pushed.  This was due to the scoped name
of the function (in lowercase) was not being used in the search to see
if it had been previously created.  A new integration test was added
to check the use case where more than one handler is being used.

The handler is now checked to see if it supported the pushProcessor()
functionality and if it does not then a warning is generated to notify
the user messages sent to that handler will not be decorated.
  • Loading branch information
mfulb committed Sep 22, 2023
1 parent dfebf27 commit 13ebe04
Show file tree
Hide file tree
Showing 5 changed files with 571 additions and 2 deletions.
24 changes: 22 additions & 2 deletions agent/lib_monolog.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
/*
* Define name of log decorating processor function
*/
#define LOG_DECORATE_NAMESPACE "newrelic\\Monolog"
#define LOG_DECORATE_NAMESPACE "Newrelic\\Monolog"
#define LOG_DECORATE_NAMESPACE_LC "newrelic\\monolog"
#define LOG_DECORATE_PROC_FUNC_NAME \
"newrelic_phpagent_monolog_decorating_processor"

Expand Down Expand Up @@ -430,7 +431,7 @@ static int nr_monolog_create_decorate_processor_function(TSRMLS_D) {
zend_function* processor_func = NULL;

/* see if processor function exists and if not create */
processor_func = nr_php_find_function(LOG_DECORATE_PROC_FUNC_NAME);
processor_func = nr_php_find_function(LOG_DECORATE_NAMESPACE_LC "\\" LOG_DECORATE_PROC_FUNC_NAME);
if (NULL == processor_func) {
nrl_verbosedebug(NRL_INSTRUMENT,
"Creating Monolog decorating processor func");
Expand Down Expand Up @@ -463,6 +464,8 @@ static int nr_monolog_create_decorate_processor_function(TSRMLS_D) {
"%s: error creating Monolog decorating processor function!",
__func__);
}
} else {
nrl_verbosedebug(NRL_INSTRUMENT, "Using existing Monolog decorating processor func");
}

return retval;
Expand All @@ -477,6 +480,11 @@ NR_PHP_WRAPPER(nr_monolog_logger_pushhandler) {
goto end;
}

nrl_verbosedebug(
NRL_INSTRUMENT, "%s : Monolog handler = %*s", __func__,
NRSAFELEN(nr_php_class_entry_name_length(Z_OBJCE_P(handler))),
nr_php_class_entry_name(Z_OBJCE_P(handler)));

if (nr_txn_log_forwarding_enabled(NRPRG(txn))
&& nr_php_object_instanceof_class(
handler, "NewRelic\\Monolog\\Enricher\\Handler" TSRMLS_CC)) {
Expand All @@ -489,6 +497,18 @@ NR_PHP_WRAPPER(nr_monolog_logger_pushhandler) {
zval* callback_name = NULL;
zval* ph_retval = NULL;

/* Verify the handler implements pushProcessor () */
if (!nr_php_object_has_method(handler, "pushProcessor" TSRMLS_CC)) {
nrl_warning(
NRL_INSTRUMENT,
"Monolog handler %*s does not implement the pushProcessor() "
"method so log decoration will not occur!",
NRSAFELEN(nr_php_class_entry_name_length(Z_OBJCE_P(handler))),
nr_php_class_entry_name(Z_OBJCE_P(handler)));

goto end;
}

/* Create function used to decorate Monolog log records */
nr_monolog_create_decorate_processor_function();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,13 @@
ok - entity.name correct
*/

/* The "Supportability/api/get_linking_metadata" metric has a count of 16 because it is
* called once inside the processor function which adds the linking metadata per log
* message (8 total messages in this test).
* Then it is also called once per log message in the custom formatter this
* test adds to check the values inserted by the processor function which adds 8 more.
*/

/*EXPECT_METRICS
[
"?? agent run id",
Expand Down Expand Up @@ -150,6 +157,9 @@ public function format(array $record) {
}
tap_equal($name, $matches[6], "entity.name correct");
}

/* have to return a non-null value which is the output string */
return "";
}

public function formatBatch(array $records) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,264 @@
<?php
/*
* Copyright 2022 New Relic Corporation. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*/

/*DESCRIPTION
Test that Monolog2 instrumentation adds linking metadata for log decoration
*/

/*SKIPIF
<?php
require('skipif.inc');
*/

/*INI
newrelic.loglevel = verbosedebug
newrelic.application_logging.enabled = true
newrelic.application_logging.forwarding.enabled = false
newrelic.application_logging.local_decorating.enabled = true
newrelic.application_logging.metrics.enabled = true
newrelic.application_logging.forwarding.max_samples_stored = 10
newrelic.application_logging.forwarding.log_level = INFO
*/

/*EXPECT
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
ok - All NR-LINKING elements present
ok - NR-LINKING present
ok - entity.guid correct
ok - entity.guid correct
ok - trace.id correct
ok - span.id is non-zero length and alphanumeric
ok - entity.name correct
*/

/*EXPECT_METRICS
[
"?? agent run id",
"?? timeframe start",
"?? timeframe stop",
[
[{"name": "DurationByCaller/Unknown/Unknown/Unknown/Unknown/all"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "DurationByCaller/Unknown/Unknown/Unknown/Unknown/allOther"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "Logging/lines"}, [8, "??", "??", "??", "??", "??"]],
[{"name": "Logging/lines/ALERT"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "Logging/lines/CRITICAL"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "Logging/lines/DEBUG"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "Logging/lines/EMERGENCY"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "Logging/lines/ERROR"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "Logging/lines/INFO"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "Logging/lines/NOTICE"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "Logging/lines/WARNING"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "OtherTransaction/all"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "OtherTransactionTotalTime"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "OtherTransactionTotalTime/php__FILE__"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "Supportability/api/get_linking_metadata"}, [32, "??", "??", "??", "??", "??"]],
[{"name": "Supportability/Logging/PHP/Monolog/enabled"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "Supportability/library/Monolog/detected"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "Supportability/Logging/LocalDecorating/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "Supportability/Logging/Forwarding/PHP/disabled"}, [1, "??", "??", "??", "??", "??"]],
[{"name": "Supportability/Logging/Metrics/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]]
]
]
*/


/*EXPECT_LOG_EVENTS
null
*/

require_once(realpath(dirname(__FILE__)) . '/../../../include/config.php');
require_once(realpath (dirname ( __FILE__ )) . '/../../../include/tap.php');
require_once(realpath(dirname(__FILE__)) . '/../../../include/monolog.php');
require_monolog(2);

use Monolog\Handler\NoopHandler;
use Monolog\Handler\ProcessHandler;
use Monolog\Logger;
use Monolog\Handler\StreamHandler;

/* create formatter class that echos the interesting data that the log
decoration should have added. */
class CheckDecorateFormatter implements Monolog\Formatter\FormatterInterface {
public function __construct(?string $dateFormat = null) {
}
public function format(array $record) {
$nrlinking = $record['extra']['NR-LINKING'] ?? 'NR-LINKING DATA MISSING!!!';
$result = preg_match("/(NR\-LINKING)\|([\w\d]+)\|([\w\d]+)\|([\w\d]+)\|([\w\d]+)\|([\w\d\%]+\.php)\|/", $nrlinking, $matches);
$linkmeta = newrelic_get_linking_metadata();

tap_equal(7, count($matches), "All NR-LINKING elements present");
if (7 == count($matches)) {
tap_equal("NR-LINKING", $matches[1], "NR-LINKING present");
tap_equal($linkmeta['entity.guid'] ?? '<missing entity.guid>', $matches[2], "entity.guid correct");
tap_equal($linkmeta['hostname'] ?? '<missing hostname>', $matches[3], "entity.guid correct");
tap_equal($linkmeta['trace.id'] ?? '<missing trace.id>', $matches[4], "trace.id correct");
tap_equal(true, strlen($matches[5]) > 0 && preg_match("/[\w\d]+/",$matches[5]), "span.id is non-zero length and alphanumeric");
if (isset($linkmeta['entity.name'])) {
$name = urlencode($linkmeta['entity.name']);
} else {
$name = '<missing entity.name>';
}
tap_equal($name, $matches[6], "entity.name correct");
}

/* have to return a non-null value which is the output string */
return "";
}

public function formatBatch(array $records) {
foreach ($records as $key => $record) {
$records[$key] = $this->format($record);
}

return $records;
}
}


function test_logging() {
$logger = new Logger('monolog2');

$formatter = new CheckDecorateFormatter();

$stdoutHandler = new StreamHandler('php://stdout', Logger::DEBUG);
$stdoutHandler->setFormatter($formatter);
$noopHandler = new NoopHandler();
$processHandler = new ProcessHandler('cat > /dev/null', logger::DEBUG);
$processHandler->setFormatter($formatter);

$logger->pushHandler($stdoutHandler);
$logger->pushHandler($noopHandler);
$logger->pushHandler($processHandler);


// insert delays between log messages to allow priority sampling
// to resolve that later messages have higher precedence
// since timestamps are only millisecond resolution
// without delays sometimes order in output will reflect
// all having the same timestamp.
$logger->debug("debug");
usleep(10000);
$logger->info("info");
usleep(10000);
$logger->notice("notice");
usleep(10000);
$logger->warning("warning");
usleep(10000);
$logger->error("error");
usleep(10000);
$logger->critical("critical");
usleep(10000);
$logger->alert("alert");
usleep(10000);
$logger->emergency("emergency");
}

test_logging();
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,13 @@
newrelic.application_logging.forwarding.log_level = INFO
*/

/* The "Supportability/api/get_linking_metadata" metric has a count of 16 because it is
* called once inside the processor function which adds the linking metadata per log
* message (8 total messages in this test).
* Then it is also called once per log message in the custom formatter this
* test adds to check the values inserted by the processor function which adds 8 more.
*/

/*EXPECT
ok - All NR-LINKING elements present
ok - NR-LINKING present
Expand Down Expand Up @@ -151,6 +158,9 @@ public function format(LogRecord $record) {
}
tap_equal($name, $matches[6], "entity.name correct");
}

/* have to return a non-null value which is the output string */
return "";
}

public function formatBatch(array $records) {
Expand Down
Loading

0 comments on commit 13ebe04

Please sign in to comment.