From 42e401d07ddae84f393add9ea6e8c383137fa8dd Mon Sep 17 00:00:00 2001 From: Michael Fulbright <89205663+mfulb@users.noreply.github.com> Date: Tue, 14 Nov 2023 12:02:17 -0500 Subject: [PATCH 01/17] chore(agent): Bump to 10.15.0 (#764) This bumps the dev branch to the next release version/code name. --- VERSION | 2 +- axiom/nr_version.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/VERSION b/VERSION index 0ca1348de..f9fb144f9 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -10.14.0 +10.15.0 diff --git a/axiom/nr_version.c b/axiom/nr_version.c index 30fc06d36..703447ba3 100644 --- a/axiom/nr_version.c +++ b/axiom/nr_version.c @@ -41,8 +41,9 @@ * narcissus 20Jun2023 (10.11) * orchid 20Sep2023 (10.12) * poinsettia 03Oct2023 (10.13) + * quince 13Nov2023 (10.14) */ -#define NR_CODENAME "quince" +#define NR_CODENAME "rose" const char* nr_version(void) { return NR_STR2(NR_VERSION); From 78c55dcfffa0e74f927a371a835baa8ba288afef Mon Sep 17 00:00:00 2001 From: Michal Nowacki Date: Wed, 15 Nov 2023 16:42:22 -0500 Subject: [PATCH 02/17] tests: fix guzzle integration tests (#772) Guzzle integration tests are flaky - they expect a span for a call to `new GuzzleHttp\Client();`. However, when `transaction_tracer.detail` is set to 0, the agent sometimes drops it. On the other hand when `transaction_tracer.detail` is set to 1, the agent generates many more spans and adding them to `EXPECT_SPAN_EVENTS` seems tedious. Therefore `EXPECT_SPAN_EVENTS_LIKE` is used in test expectations to check if the required spans - root and external - are present. --- .../external/guzzle5/test_spans_external.php | 31 +------------------ .../guzzle5/test_spans_external.php5.php | 26 +--------------- .../test_spans_are_created_correctly.php | 31 +------------------ .../test_spans_are_created_correctly.php5.php | 26 +--------------- .../test_spans_are_created_correctly.php | 31 +------------------ 5 files changed, 5 insertions(+), 140 deletions(-) diff --git a/tests/integration/external/guzzle5/test_spans_external.php b/tests/integration/external/guzzle5/test_spans_external.php index c32e4ec34..c03af59cf 100644 --- a/tests/integration/external/guzzle5/test_spans_external.php +++ b/tests/integration/external/guzzle5/test_spans_external.php @@ -26,14 +26,8 @@ /*EXPECT_RESPONSE_HEADERS */ -/*EXPECT_SPAN_EVENTS +/*EXPECT_SPAN_EVENTS_LIKE [ - "?? agent run id", - { - "reservoir_size": 10000, - "events_seen": 4 - }, - [ [ { "traceId": "??", @@ -52,28 +46,6 @@ {}, {} ], - [ - { - "traceId": "??", - "duration": "??", - "transactionId": "??", - "name": "Custom\/GuzzleHttp\\Client::__construct", - "guid": "??", - "type": "Span", - "category": "generic", - "priority": "??", - "sampled": true, - "parentId": "??", - "timestamp": "??" - }, - {}, - { - "code.lineno": "??", - "code.namespace": "GuzzleHttp\\Client", - "code.filepath": "??", - "code.function": "__construct" - } - ], [ { "traceId": "??", @@ -120,7 +92,6 @@ "http.statusCode": 200 } ] - ] ] */ diff --git a/tests/integration/external/guzzle5/test_spans_external.php5.php b/tests/integration/external/guzzle5/test_spans_external.php5.php index 6d8f0b1fa..509e96964 100644 --- a/tests/integration/external/guzzle5/test_spans_external.php5.php +++ b/tests/integration/external/guzzle5/test_spans_external.php5.php @@ -23,14 +23,8 @@ /*EXPECT_RESPONSE_HEADERS */ -/*EXPECT_SPAN_EVENTS +/*EXPECT_SPAN_EVENTS_LIKE [ - "?? agent run id", - { - "reservoir_size": 10000, - "events_seen": 4 - }, - [ [ { "traceId": "??", @@ -49,23 +43,6 @@ {}, {} ], - [ - { - "traceId": "??", - "duration": "??", - "transactionId": "??", - "name": "Custom\/GuzzleHttp\\Client::__construct", - "guid": "??", - "type": "Span", - "category": "generic", - "priority": "??", - "sampled": true, - "parentId": "??", - "timestamp": "??" - }, - {}, - {} - ], [ { "traceId": "??", @@ -112,7 +89,6 @@ "http.statusCode": 200 } ] - ] ] */ diff --git a/tests/integration/external/guzzle6/test_spans_are_created_correctly.php b/tests/integration/external/guzzle6/test_spans_are_created_correctly.php index 2692781a4..0c9d0994f 100644 --- a/tests/integration/external/guzzle6/test_spans_are_created_correctly.php +++ b/tests/integration/external/guzzle6/test_spans_are_created_correctly.php @@ -24,14 +24,8 @@ */ -/*EXPECT_SPAN_EVENTS +/*EXPECT_SPAN_EVENTS_LIKE [ - "?? agent run id", - { - "reservoir_size": 10000, - "events_seen": 3 - }, - [ [ { "traceId": "??", @@ -50,28 +44,6 @@ {}, {} ], - [ - { - "traceId": "??", - "duration": "??", - "transactionId": "??", - "name": "Custom\/GuzzleHttp\\Client::__construct", - "guid": "??", - "type": "Span", - "category": "generic", - "priority": "??", - "sampled": true, - "parentId": "??", - "timestamp": "??" - }, - {}, - { - "code.lineno": "??", - "code.namespace": "GuzzleHttp\\Client", - "code.filepath": "??", - "code.function": "__construct" - } - ], [ { "traceId": "??", @@ -95,7 +67,6 @@ "http.statusCode": 200 } ] - ] ] */ diff --git a/tests/integration/external/guzzle6/test_spans_are_created_correctly.php5.php b/tests/integration/external/guzzle6/test_spans_are_created_correctly.php5.php index 1520144f6..82b9b9911 100644 --- a/tests/integration/external/guzzle6/test_spans_are_created_correctly.php5.php +++ b/tests/integration/external/guzzle6/test_spans_are_created_correctly.php5.php @@ -21,14 +21,8 @@ */ -/*EXPECT_SPAN_EVENTS +/*EXPECT_SPAN_EVENTS_LIKE [ - "?? agent run id", - { - "reservoir_size": 10000, - "events_seen": 3 - }, - [ [ { "traceId": "??", @@ -47,23 +41,6 @@ {}, {} ], - [ - { - "traceId": "??", - "duration": "??", - "transactionId": "??", - "name": "Custom\/GuzzleHttp\\Client::__construct", - "guid": "??", - "type": "Span", - "category": "generic", - "priority": "??", - "sampled": true, - "parentId": "??", - "timestamp": "??" - }, - {}, - {} - ], [ { "traceId": "??", @@ -87,7 +64,6 @@ "http.statusCode": 200 } ] - ] ] */ diff --git a/tests/integration/external/guzzle7/test_spans_are_created_correctly.php b/tests/integration/external/guzzle7/test_spans_are_created_correctly.php index 58d3825e0..87388da64 100644 --- a/tests/integration/external/guzzle7/test_spans_are_created_correctly.php +++ b/tests/integration/external/guzzle7/test_spans_are_created_correctly.php @@ -20,14 +20,8 @@ */ -/*EXPECT_SPAN_EVENTS +/*EXPECT_SPAN_EVENTS_LIKE [ - "?? agent run id", - { - "reservoir_size": 10000, - "events_seen": 3 - }, - [ [ { "traceId": "??", @@ -46,28 +40,6 @@ {}, {} ], - [ - { - "traceId": "??", - "duration": "??", - "transactionId": "??", - "name": "Custom\/GuzzleHttp\\Client::__construct", - "guid": "??", - "type": "Span", - "category": "generic", - "priority": "??", - "sampled": true, - "parentId": "??", - "timestamp": "??" - }, - {}, - { - "code.lineno": "??", - "code.namespace": "GuzzleHttp\\Client", - "code.filepath": "??", - "code.function": "__construct" - } - ], [ { "traceId": "??", @@ -91,7 +63,6 @@ "http.statusCode": 200 } ] - ] ] */ From 9d3708265379869534a1521a9ddea630ca109c09 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Thu, 16 Nov 2023 06:29:48 -0700 Subject: [PATCH 03/17] chore: Update repolinter related items (#762) * Update README.md community plus header with the latest snippet * Update README.md link to point directly to the forums * Update repolinter.yml to elevate permissions needed to create/modify/close issues. Resolves https://github.com/newrelic/newrelic-php-agent/issues/573 --------- Co-authored-by: Michael Fulbright <89205663+mfulb@users.noreply.github.com> --- .github/workflows/repolinter.yml | 3 +++ README.md | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/repolinter.yml b/.github/workflows/repolinter.yml index acb921f30..e1e3c1cc1 100644 --- a/.github/workflows/repolinter.yml +++ b/.github/workflows/repolinter.yml @@ -8,6 +8,9 @@ name: Repolinter Action # filtered in the "Test Default Branch" step. on: [push, workflow_dispatch] +permissions: + issues: write + jobs: repolint: name: Run Repolinter diff --git a/README.md b/README.md index 6a67babc9..e0bef27e9 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -[![Community Plus header](https://github.com/newrelic/opensource-website/raw/master/src/images/categories/Community_Plus.png)](https://opensource.newrelic.com/oss-category/#community-plus) +New Relic Open Source community plus project banner. # New Relic PHP agent [![agent-build status](https://github.com/newrelic/newrelic-php-agent/workflows/agent-build/badge.svg)](https://github.com/newrelic/newrelic-php-agent/actions) @@ -35,7 +35,7 @@ If the issue is confirmed as a bug or is a feature request, please file a GitHub **Support Channels** * [New Relic Documentation](https://docs.newrelic.com/docs/agents/php-agent/getting-started/introduction-new-relic-php): Comprehensive guidance for using our platform -* [New Relic Community](https://discuss.newrelic.com/tags/phpagent): The best place to engage in troubleshooting questions +* [New Relic Community](https://forum.newrelic.com/): The best place to engage in troubleshooting questions * [New Relic Developer](https://developer.newrelic.com/): Resources for building a custom observability applications * [New Relic University](https://learn.newrelic.com/): A range of online training for New Relic users of every level * [New Relic Technical Support](https://support.newrelic.com/) 24/7/365 ticketed support. Read more about our [Technical Support Offerings](https://docs.newrelic.com/docs/licenses/license-information/general-usage-licenses/global-technical-support-offerings). From 7b3a076340edc65e50e5914133db7db1b579b3a8 Mon Sep 17 00:00:00 2001 From: Michal Nowacki Date: Fri, 17 Nov 2023 16:12:40 -0500 Subject: [PATCH 04/17] tests: add `EXPECT_METRICS_EXIST` to integration tests (#771) Integration tests sometimes don't care about the exact set of metrics that are being sent (particularly around things like datastore metrics); what is really interesting is that the framework is detected, the transaction is named correctly, and that there aren't errors. This adds a new `EXPECT_METRICS_EXIST` section which is simply a newline separated list of metric names that will be searched for: if any don't exist, the test will fail. For example: ```php ``` This asserts that the test will generate `Supportability/framework/WordPress/detected` metric, but doesn't assert anything else about the metric table. --------- Co-authored-by: Adam Harvey --- src/newrelic/integration/parse.go | 5 ++++ src/newrelic/integration/test.go | 10 ++++++++ src/newrelic/metrics.go | 7 ++++++ src/newrelic/metrics_test.go | 18 +++++++++++++++ .../analog/test_supportability_metric.php | 22 +++--------------- .../test_supportability_metric.php | 23 +++---------------- .../test_supportability_metric.php | 23 +++---------------- .../test_supportability_metric.php | 23 +++---------------- 8 files changed, 52 insertions(+), 79 deletions(-) diff --git a/src/newrelic/integration/parse.go b/src/newrelic/integration/parse.go index 554972556..f44c8d22a 100644 --- a/src/newrelic/integration/parse.go +++ b/src/newrelic/integration/parse.go @@ -32,6 +32,7 @@ var ( "EXPECT_SPAN_EVENTS_LIKE": parseSpanEventsLike, "EXPECT_LOG_EVENTS": parseLogEvents, "EXPECT_METRICS": parseMetrics, + "EXPECT_METRICS_EXIST": parseMetricsExist, "EXPECT": parseExpect, "EXPECT_REGEX": parseExpectRegex, "EXPECT_SCRUBBED": parseExpectScrubbed, @@ -224,6 +225,10 @@ func parseMetrics(test *Test, content []byte) error { test.metrics = content return nil } +func parseMetricsExist(test *Test, content []byte) error { + test.metricsExist = content + return nil +} func parseSlowSQLs(test *Test, content []byte) error { test.slowSQLs = content return nil diff --git a/src/newrelic/integration/test.go b/src/newrelic/integration/test.go index 4ce126a33..25945964d 100644 --- a/src/newrelic/integration/test.go +++ b/src/newrelic/integration/test.go @@ -36,6 +36,7 @@ type Test struct { spanEventsLike []byte logEvents []byte metrics []byte + metricsExist []byte slowSQLs []byte tracedErrors []byte txnTraces []byte @@ -557,6 +558,15 @@ func (t *Test) Compare(harvest *newrelic.Harvest) { return } + if nil != t.metricsExist { + for _, name := range strings.Split(strings.TrimSpace(string(t.metricsExist)), "\n") { + name = strings.TrimSpace(name) + if !harvest.Metrics.Has(name) { + t.Fail(fmt.Errorf("metric does not exist: %s\n\nactual metric table: %s", name, harvest.Metrics.DebugJSON())) + } + } + } + // if we expect a harvest and these is not, then we run our tests as per normal t.compareResponseHeaders() diff --git a/src/newrelic/metrics.go b/src/newrelic/metrics.go index 3a5e7cee0..28a78620c 100644 --- a/src/newrelic/metrics.go +++ b/src/newrelic/metrics.go @@ -340,6 +340,13 @@ func (mt *MetricTable) Empty() bool { return 0 == mt.count } +// Has returns true if the given metric exists in the metric table (regardless +// of scope). +func (mt *MetricTable) Has(name string) bool { + _, ok := mt.metrics[name] + return ok +} + // Data marshals the collection to JSON according to the schema expected // by the collector. func (mt *MetricTable) Data(id AgentRunID, harvestStart time.Time) ([]byte, diff --git a/src/newrelic/metrics_test.go b/src/newrelic/metrics_test.go index 1aee056dc..1c29e6eaa 100644 --- a/src/newrelic/metrics_test.go +++ b/src/newrelic/metrics_test.go @@ -301,3 +301,21 @@ func BenchmarkMetricTableCollectorJSON(b *testing.B) { mt.CollectorJSON(AgentRunID("12345"), now) } } + +func TestMetricTableHas(t *testing.T) { + mt := NewMetricTable(20, start) + mt.AddCount("foo", "", 1, 0) + mt.AddCount("bar", "quux", 1, 0) + + if mt.Has("quux") { + t.Fatal("non-existent metric is reported as existing") + } + + if !mt.Has("foo") { + t.Fatal("unscoped metric is reported as missing") + } + + if !mt.Has("bar") { + t.Fatal("scoped metric is reported as missing") + } +} diff --git a/tests/integration/logging/analog/test_supportability_metric.php b/tests/integration/logging/analog/test_supportability_metric.php index c0c5ce644..f3cfb4477 100644 --- a/tests/integration/logging/analog/test_supportability_metric.php +++ b/tests/integration/logging/analog/test_supportability_metric.php @@ -16,25 +16,9 @@ newrelic.application_logging.enabled = true */ -/*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":"OtherTransaction/all"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"OtherTransactionTotalTime"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"OtherTransactionTotalTime/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/PHP/Analog/disabled"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/library/Analog/detected"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/Forwarding/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/Metrics/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/LocalDecorating/PHP/disabled"}, [1, "??", "??", "??", "??", "??"]] - ] -] +/*EXPECT_METRICS_EXIST +Supportability/library/Analog/detected +Supportability/Logging/PHP/Analog/disabled */ diff --git a/tests/integration/logging/cakephp-log/test_supportability_metric.php b/tests/integration/logging/cakephp-log/test_supportability_metric.php index 777dae825..93bc9e01e 100644 --- a/tests/integration/logging/cakephp-log/test_supportability_metric.php +++ b/tests/integration/logging/cakephp-log/test_supportability_metric.php @@ -16,26 +16,9 @@ newrelic.application_logging.enabled = true */ -/*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":"OtherTransaction/all"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"OtherTransactionTotalTime"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"OtherTransactionTotalTime/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/PHP/cakephp-log/disabled"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/library/cakephp-log/detected"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/Forwarding/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/Metrics/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/LocalDecorating/PHP/disabled"}, [1, "??", "??", "??", "??", "??"]] - ] -] +/*EXPECT_METRICS_EXIST +Supportability/library/cakephp-log/detected +Supportability/Logging/PHP/cakephp-log/disabled */ - require_once(realpath(dirname(__FILE__)) . '/vendor/cakephp/log/Log.php'); diff --git a/tests/integration/logging/consolidation-log/test_supportability_metric.php b/tests/integration/logging/consolidation-log/test_supportability_metric.php index e41529604..0eac6dfe2 100644 --- a/tests/integration/logging/consolidation-log/test_supportability_metric.php +++ b/tests/integration/logging/consolidation-log/test_supportability_metric.php @@ -16,26 +16,9 @@ newrelic.application_logging.enabled = true */ -/*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":"OtherTransaction/all"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"OtherTransactionTotalTime"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"OtherTransactionTotalTime/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/PHP/Consolidation/Log/disabled"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/library/Consolidation/Log/detected"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/Forwarding/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/Metrics/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/LocalDecorating/PHP/disabled"}, [1, "??", "??", "??", "??", "??"]] - ] -] +/*EXPECT_METRICS_EXIST +Supportability/library/Consolidation/Log/detected +Supportability/Logging/PHP/Consolidation/Log/disabled */ - require_once(realpath(dirname(__FILE__)) . '/vendor/consolidation/log/src/Logger.php'); diff --git a/tests/integration/logging/laminas-log/test_supportability_metric.php b/tests/integration/logging/laminas-log/test_supportability_metric.php index 104f5d274..ddf7d276a 100644 --- a/tests/integration/logging/laminas-log/test_supportability_metric.php +++ b/tests/integration/logging/laminas-log/test_supportability_metric.php @@ -16,26 +16,9 @@ newrelic.application_logging.enabled = true */ -/*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":"OtherTransaction/all"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"OtherTransactionTotalTime"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"OtherTransactionTotalTime/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/PHP/laminas-log/disabled"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/library/laminas-log/detected"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/Forwarding/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/Metrics/PHP/enabled"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Supportability/Logging/LocalDecorating/PHP/disabled"}, [1, "??", "??", "??", "??", "??"]] - ] -] +/*EXPECT_METRICS_EXIST +Supportability/library/laminas-log/detected +Supportability/Logging/PHP/laminas-log/disabled */ - require_once(realpath(dirname(__FILE__)) . '/vendor/laminas/laminas-log/src/Logger.php'); From 4bb62ba8f1e5e276e544ce7a3f31195b0fa20c7a Mon Sep 17 00:00:00 2001 From: Michal Nowacki Date: Mon, 20 Nov 2023 15:09:11 -0500 Subject: [PATCH 05/17] tests: make tests deterministic (#781) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Force non-zero duration of helper user function When tests execute fast enough, the helper function's segment start time and end time are equal, and this causes the segment to be dropped. However, some integration tests have inconsistent test expectation in expected transaction traces. On one hand, wildcard match is used for trace details but on the other hand segment names are expected to match exactly. On faster hardware this discrepancy will cause such tests to fail. To address this test instability, non-zero duration is forced on test helper user function `force_transaction_trace`. 2. Adjust forced duration to ensure expected sampling `tests/integration/ini/test_transaction_tracer_max_segments_with_datastore.php` expects datastore segment, created with `newrelic_record_datastore_segment`, to have longer duration than the at least one execution of custom instrumented user function `my_function`, and therefore appear in the transaction trace. There is, however, absolutely no apparent reason for that to happen. `tests/integration/ini/test_transaction_tracer_max_segments_with_datastore.php` test limits the number of recorded segments to 3 with this ini setting: `newrelic.transaction_tracer.max_segments_cli=3`. This has direct impact on the size of the heap of segments kept. The test creates more than 3 segments - 3 for custom traced `my_function` and 1 datastore segment. However, neither of the segments have a well defined, deterministic duration. Duration is used to determine which segment gets kept and which one gets dropped at the time the segment gets ended. Note when the heap of segments kept heap is created, `nr_segment_wrapped_span_priority_comparator` is set to determine min and max values of elements in the heap. However, in this particular test, both segments have the same, default priority of 0. They’re neither root segments, neither segment’s id is used in distributed trace, nor any of the segments has logs or user attributes associated with it. This causes `nr_segment_wrapped_span_priority_comparator` to fall back to `nr_segment_wrapped_duration_comparator`, which leaves things to being more or less random as sometimes the execution of `my_function` is shorter than execution of `newrelic_record_datastore_segment` but other times it is the other way around. --- tests/include/helpers.php | 4 ++-- .../integration/ini/test_transaction_tracer_max_segments.php | 2 +- .../ini/test_transaction_tracer_max_segments_nested.php | 2 +- .../ini/test_transaction_tracer_max_segments_nested.php5.php | 2 +- .../ini/test_transaction_tracer_max_segments_no_cap.php | 2 +- .../test_transaction_tracer_max_segments_with_datastore.php | 4 ++-- ...st_transaction_tracer_max_segments_with_datastore.php5.php | 4 ++-- 7 files changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/include/helpers.php b/tests/include/helpers.php index d528ececb..92e28a0f4 100644 --- a/tests/include/helpers.php +++ b/tests/include/helpers.php @@ -31,8 +31,8 @@ function force_error() { * A user function has to be called to force a transaction trace. PHP 7.1 * includes dead code elimination, which means that we have to actually do * something that can't be eliminated: re-setting the error reporting level to - * what it already is will do the job. + * what it already is will do the job. Additionally force non-zero duration for the segment not to be dropped. */ function force_transaction_trace() { - error_reporting(error_reporting()); + error_reporting(error_reporting()); time_nanosleep(0, 50000); // 50usec should be enough } diff --git a/tests/integration/ini/test_transaction_tracer_max_segments.php b/tests/integration/ini/test_transaction_tracer_max_segments.php index 6b33d1f92..758b570e1 100644 --- a/tests/integration/ini/test_transaction_tracer_max_segments.php +++ b/tests/integration/ini/test_transaction_tracer_max_segments.php @@ -82,7 +82,7 @@ function my_function(){ - printf(''); + time_nanosleep(0, 50000); // force non-zero duration for the segment not to be dropped, 50usec should be enough } newrelic_add_custom_tracer("my_function"); diff --git a/tests/integration/ini/test_transaction_tracer_max_segments_nested.php b/tests/integration/ini/test_transaction_tracer_max_segments_nested.php index 5495defc7..7a97a6448 100644 --- a/tests/integration/ini/test_transaction_tracer_max_segments_nested.php +++ b/tests/integration/ini/test_transaction_tracer_max_segments_nested.php @@ -177,7 +177,7 @@ function my_function() { - printf(''); + time_nanosleep(0, 50000); // force non-zero duration for the segment not to be dropped, 50usec should be enough } function grandmother(){ for ($i = 0; $i < 1000; $i++) { diff --git a/tests/integration/ini/test_transaction_tracer_max_segments_nested.php5.php b/tests/integration/ini/test_transaction_tracer_max_segments_nested.php5.php index 3b55503b2..f5fb391c9 100644 --- a/tests/integration/ini/test_transaction_tracer_max_segments_nested.php5.php +++ b/tests/integration/ini/test_transaction_tracer_max_segments_nested.php5.php @@ -126,7 +126,7 @@ function my_function() { - printf(''); + time_nanosleep(0, 50000); // force non-zero duration for the segment not to be dropped, 50usec should be enough } function grandmother(){ for ($i = 0; $i < 1000; $i++) { diff --git a/tests/integration/ini/test_transaction_tracer_max_segments_no_cap.php b/tests/integration/ini/test_transaction_tracer_max_segments_no_cap.php index 4da948d22..fe3f05356 100644 --- a/tests/integration/ini/test_transaction_tracer_max_segments_no_cap.php +++ b/tests/integration/ini/test_transaction_tracer_max_segments_no_cap.php @@ -82,7 +82,7 @@ function my_function(){ - printf(''); + time_nanosleep(0, 50000); // force non-zero duration for the segment not to be dropped, 50usec should be enough } newrelic_add_custom_tracer("my_function"); diff --git a/tests/integration/ini/test_transaction_tracer_max_segments_with_datastore.php b/tests/integration/ini/test_transaction_tracer_max_segments_with_datastore.php index a0e8ac8b1..f6e2b0171 100644 --- a/tests/integration/ini/test_transaction_tracer_max_segments_with_datastore.php +++ b/tests/integration/ini/test_transaction_tracer_max_segments_with_datastore.php @@ -123,7 +123,7 @@ function my_function(){ - printf(''); + time_nanosleep(0, 50000); // force non-zero duration for the segment not to be dropped; duration needs to be shorter than datastore segment } newrelic_add_custom_tracer("my_function"); @@ -133,7 +133,7 @@ function my_function(){ my_function(); newrelic_record_datastore_segment(function () { - return 42; + time_nanosleep(0, 70000); return 42; // force non-zero duration for the segment not to be dropped; duration needs to be longer than user func segment }, array( 'product' => 'mysql', 'collection' => 'table', diff --git a/tests/integration/ini/test_transaction_tracer_max_segments_with_datastore.php5.php b/tests/integration/ini/test_transaction_tracer_max_segments_with_datastore.php5.php index fb7b1e706..c27e5d3be 100644 --- a/tests/integration/ini/test_transaction_tracer_max_segments_with_datastore.php5.php +++ b/tests/integration/ini/test_transaction_tracer_max_segments_with_datastore.php5.php @@ -112,7 +112,7 @@ function my_function(){ - printf(''); + time_nanosleep(0, 50000); // force non-zero duration for the segment not to be dropped; duration needs to be shorter than datastore segment } newrelic_add_custom_tracer("my_function"); @@ -122,7 +122,7 @@ function my_function(){ my_function(); newrelic_record_datastore_segment(function () { - return 42; + time_nanosleep(0, 70000); return 42; // force non-zero duration for the segment not to be dropped; duration needs to be longer than user func segment }, array( 'product' => 'mysql', 'collection' => 'table', From 8aae82ad7d1ef7b38a6e62c0d302c67db65f0081 Mon Sep 17 00:00:00 2001 From: bduranleau-nr <106178551+bduranleau-nr@users.noreply.github.com> Date: Mon, 27 Nov 2023 11:39:46 -0600 Subject: [PATCH 06/17] fix: remove warning message for `NULL` Docker ID (#783) Agent should not emit a warning message if the Docker ID was not detected in `/proc/self/mountinfo`. Non-dockerized and Non-docker V2 applications will not have this value. --- agent/php_environment.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/agent/php_environment.c b/agent/php_environment.c index 60f2a2c74..24f390b1d 100644 --- a/agent/php_environment.c +++ b/agent/php_environment.c @@ -608,9 +608,6 @@ void nr_php_gather_v2_docker_id() { if (NULL != dockerId) { NR_PHP_PROCESS_GLOBALS(docker_id) = dockerId; nrl_verbosedebug(NRL_AGENT, "%s: Docker v2 ID: %s", __func__, dockerId); - } else { - nrl_warning(NRL_AGENT, "%s: Unable to read docker v2 container id", - __func__); } } From 24c1c656540a742d9d0466b79d33808ca2507ed7 Mon Sep 17 00:00:00 2001 From: Michal Nowacki Date: Tue, 28 Nov 2023 11:35:44 -0500 Subject: [PATCH 07/17] refactor(agent): improve magic file recognition performance (#766) Speed up speed library/framework detection by performing a suffix match on the 'magic' file pattern with case insensitive string comparison instead of a substring search within a lowercased filename. This is possible because all of the 'magic' file search patterns patterns are right anchored. --------- Co-authored-by: Mike LaSpina --- agent/php_agent.h | 5 + agent/php_execute.c | 225 ++++++++++++++++++++----------------- axiom/tests/test_strings.c | 30 +++++ axiom/util_strings.h | 33 ++++++ 4 files changed, 192 insertions(+), 101 deletions(-) diff --git a/agent/php_agent.h b/agent/php_agent.h index 005daebbc..7877325d3 100644 --- a/agent/php_agent.h +++ b/agent/php_agent.h @@ -671,6 +671,11 @@ nr_php_op_array_file_name(const zend_op_array* op_array) { #endif } +static inline nr_string_len_t NRPURE +nr_php_op_array_file_name_len(const zend_op_array* op_array) { + return op_array->filename ? op_array->filename->len : 0; +} + static inline const char* NRPURE nr_php_op_array_function_name(const zend_op_array* op_array) { #if ZEND_MODULE_API_NO >= ZEND_7_0_X_API_NO /* PHP 7.0+ */ diff --git a/agent/php_execute.c b/agent/php_execute.c index a6bb53ed5..9cd08ee6e 100644 --- a/agent/php_execute.c +++ b/agent/php_execute.c @@ -310,6 +310,7 @@ typedef struct _nr_framework_table_t { const char* framework_name; const char* config_name; const char* file_to_check; + size_t file_to_check_len; nr_framework_special_fn_t special; nr_framework_enable_fn_t enable; nrframework_t detected; @@ -322,15 +323,16 @@ typedef struct _nr_framework_table_t { * * Note that all paths should be in lowercase. */ +// clang-format: off static const nr_framework_table_t all_frameworks[] = { /* * Watch out: * cake1.2 and cake1.3 use a subdirectory named 'cake' (lower case) * cake2.0 and on use a subdirectory named 'Cake' (upper case file name) */ - {"CakePHP", "cakephp", "cake/libs/object.php", nr_cakephp_special_1, + {"CakePHP", "cakephp", NR_PSTR("cake/libs/object.php"), nr_cakephp_special_1, nr_cakephp_enable_1, NR_FW_CAKEPHP}, - {"CakePHP", "cakephp", "cake/core/app.php", nr_cakephp_special_2, + {"CakePHP", "cakephp", NR_PSTR("cake/core/app.php"), nr_cakephp_special_2, nr_cakephp_enable_2, NR_FW_CAKEPHP}, /* @@ -340,87 +342,88 @@ static const nr_framework_table_t all_frameworks[] = { * specifically a problem for Expression Engine (look for expression_engine, * below.) */ - {"CodeIgniter", "codeigniter", "codeigniter.php", 0, nr_codeigniter_enable, + {"CodeIgniter", "codeigniter", NR_PSTR("codeigniter.php"), 0, nr_codeigniter_enable, NR_FW_CODEIGNITER}, - {"Drupal8", "drupal8", "core/includes/bootstrap.inc", 0, nr_drupal8_enable, + {"Drupal8", "drupal8", NR_PSTR("core/includes/bootstrap.inc"), 0, nr_drupal8_enable, NR_FW_DRUPAL8}, - {"Drupal", "drupal", "includes/common.inc", 0, nr_drupal_enable, + {"Drupal", "drupal", NR_PSTR("includes/common.inc"), 0, nr_drupal_enable, NR_FW_DRUPAL}, - {"Joomla", "joomla", "joomla/import.php", 0, nr_joomla_enable, + {"Joomla", "joomla", NR_PSTR("joomla/import.php"), 0, nr_joomla_enable, NR_FW_JOOMLA}, /* <= Joomla 1.5 */ - {"Joomla", "joomla", "libraries/joomla/factory.php", 0, nr_joomla_enable, + {"Joomla", "joomla", NR_PSTR("libraries/joomla/factory.php"), 0, nr_joomla_enable, NR_FW_JOOMLA}, /* >= Joomla 1.6, including 2.5 and 3.2 */ - {"Kohana", "kohana", "kohana/core.php", 0, nr_kohana_enable, NR_FW_KOHANA}, - {"Kohana", "kohana", "kohana/core.php", 0, nr_kohana_enable, NR_FW_KOHANA}, + {"Kohana", "kohana", NR_PSTR("kohana/core.php"), 0, nr_kohana_enable, NR_FW_KOHANA}, + {"Kohana", "kohana", NR_PSTR("kohana/core.php"), 0, nr_kohana_enable, NR_FW_KOHANA}, /* See below: Zend, the legacy project of Laminas, which shares much of the instrumentation implementation with Laminas */ - {"Laminas3", "laminas3", "laminas/mvc/application.php", 0, + {"Laminas3", "laminas3", NR_PSTR("laminas/mvc/application.php"), 0, nr_laminas3_enable, NR_FW_LAMINAS3}, - {"Laminas3", "laminas3", "laminas-mvc/src/application.php", 0, + {"Laminas3", "laminas3", NR_PSTR("laminas-mvc/src/application.php"), 0, nr_laminas3_enable, NR_FW_LAMINAS3}, - {"Laravel", "laravel", "illuminate/foundation/application.php", 0, + {"Laravel", "laravel", NR_PSTR("illuminate/foundation/application.php"), 0, nr_laravel_enable, NR_FW_LARAVEL}, - {"Laravel", "laravel", "bootstrap/compiled.php", 0, nr_laravel_enable, + {"Laravel", "laravel", NR_PSTR("bootstrap/compiled.php"), 0, nr_laravel_enable, NR_FW_LARAVEL}, /* 4.x */ - {"Laravel", "laravel", "storage/framework/compiled.php", 0, + {"Laravel", "laravel", NR_PSTR("storage/framework/compiled.php"), 0, nr_laravel_enable, NR_FW_LARAVEL}, /* 5.0.0-14 */ - {"Laravel", "laravel", "vendor/compiled.php", 0, nr_laravel_enable, + {"Laravel", "laravel", NR_PSTR("vendor/compiled.php"), 0, nr_laravel_enable, NR_FW_LARAVEL}, /* 5.0.15-5.0.x */ - {"Laravel", "laravel", "bootstrap/cache/compiled.php", 0, nr_laravel_enable, + {"Laravel", "laravel", NR_PSTR("bootstrap/cache/compiled.php"), 0, nr_laravel_enable, NR_FW_LARAVEL}, /* 5.1.0-x */ - {"Laravel", "laravel", "bootstrap/app.php", 0, nr_laravel_enable, + {"Laravel", "laravel", NR_PSTR("bootstrap/app.php"), 0, nr_laravel_enable, NR_FW_LARAVEL}, /* 8+ */ - {"Lumen", "lumen", "lumen-framework/src/helpers.php", 0, nr_lumen_enable, + {"Lumen", "lumen", NR_PSTR("lumen-framework/src/helpers.php"), 0, nr_lumen_enable, NR_FW_LUMEN}, - {"Magento", "magento", "app/mage.php", 0, nr_magento1_enable, + {"Magento", "magento", NR_PSTR("app/mage.php"), 0, nr_magento1_enable, NR_FW_MAGENTO1}, - {"Magento2", "magento2", "magento/framework/app/bootstrap.php", 0, + {"Magento2", "magento2", NR_PSTR("magento/framework/app/bootstrap.php"), 0, nr_magento2_enable, NR_FW_MAGENTO2}, - {"MediaWiki", "mediawiki", "includes/webstart.php", 0, nr_mediawiki_enable, + {"MediaWiki", "mediawiki", NR_PSTR("includes/webstart.php"), 0, nr_mediawiki_enable, NR_FW_MEDIAWIKI}, - {"Silex", "silex", "silex/application.php", 0, nr_silex_enable, + {"Silex", "silex", NR_PSTR("silex/application.php"), 0, nr_silex_enable, NR_FW_SILEX}, - {"Slim", "slim", "slim/slim/app.php", 0, nr_slim_enable, + {"Slim", "slim", NR_PSTR("slim/slim/app.php"), 0, nr_slim_enable, NR_FW_SLIM}, /* 3.x */ - {"Slim", "slim", "slim/slim/slim.php", 0, nr_slim_enable, + {"Slim", "slim", NR_PSTR("slim/slim/slim.php"), 0, nr_slim_enable, NR_FW_SLIM}, /* 2.x */ - {"Symfony", "symfony1", "sfcontext.class.php", 0, nr_symfony1_enable, + {"Symfony", "symfony1", NR_PSTR("sfcontext.class.php"), 0, nr_symfony1_enable, NR_FW_SYMFONY1}, - {"Symfony", "symfony1", "sfconfig.class.php", 0, nr_symfony1_enable, + {"Symfony", "symfony1", NR_PSTR("sfconfig.class.php"), 0, nr_symfony1_enable, NR_FW_SYMFONY1}, - {"Symfony2", "symfony2", "bootstrap.php.cache", 0, nr_symfony2_enable, + {"Symfony2", "symfony2", NR_PSTR("bootstrap.php.cache"), 0, nr_symfony2_enable, NR_FW_SYMFONY2}, /* also Symfony 3 */ {"Symfony2", "symfony2", - "symfony/bundle/frameworkbundle/frameworkbundle.php", 0, + NR_PSTR("symfony/bundle/frameworkbundle/frameworkbundle.php"), 0, nr_symfony2_enable, NR_FW_SYMFONY2}, /* also Symfony 3 */ - {"Symfony4", "symfony4", "http-kernel/httpkernel.php", 0, + {"Symfony4", "symfony4", NR_PSTR("http-kernel/httpkernel.php"), 0, nr_symfony4_enable, NR_FW_SYMFONY4}, /* also Symfony 5 */ - {"WordPress", "wordpress", "wp-config.php", 0, nr_wordpress_enable, + {"WordPress", "wordpress", NR_PSTR("wp-config.php"), 0, nr_wordpress_enable, NR_FW_WORDPRESS}, - {"Yii", "yii", "framework/yii.php", 0, nr_yii_enable, NR_FW_YII}, - {"Yii", "yii", "framework/yiilite.php", 0, nr_yii_enable, NR_FW_YII}, + {"Yii", "yii", NR_PSTR("framework/yii.php"), 0, nr_yii_enable, NR_FW_YII}, + {"Yii", "yii", NR_PSTR("framework/yiilite.php"), 0, nr_yii_enable, NR_FW_YII}, /* See above: Laminas, the successor to Zend, which shares much of the instrumentation implementation with Zend */ - {"Zend", "zend", "zend/loader.php", 0, nr_zend_enable, NR_FW_ZEND}, - {"Zend2", "zend2", "zend/mvc/application.php", 0, nr_fw_zend2_enable, + {"Zend", "zend", NR_PSTR("zend/loader.php"), 0, nr_zend_enable, NR_FW_ZEND}, + {"Zend2", "zend2", NR_PSTR("zend/mvc/application.php"), 0, nr_fw_zend2_enable, NR_FW_ZEND2}, - {"Zend2", "zend2", "zend-mvc/src/application.php", 0, nr_fw_zend2_enable, + {"Zend2", "zend2", NR_PSTR("zend-mvc/src/application.php"), 0, nr_fw_zend2_enable, NR_FW_ZEND2}, }; +// clang-format: on static const int num_all_frameworks = sizeof(all_frameworks) / sizeof(nr_framework_table_t); @@ -467,30 +470,32 @@ nrframework_t nr_php_framework_from_config(const char* config_name) { typedef struct _nr_library_table_t { const char* library_name; const char* file_to_check; + size_t file_to_check_len; nr_library_enable_fn_t enable; } nr_library_table_t; /* * Note that all paths should be in lowercase. */ +// clang-format: off static nr_library_table_t libraries[] = { - {"Doctrine 2", "doctrine/orm/query.php", nr_doctrine2_enable}, - {"Guzzle 3", "guzzle/http/client.php", nr_guzzle3_enable}, - {"Guzzle 4-5", "hasemitterinterface.php", nr_guzzle4_enable}, - {"Guzzle 6", "guzzle/src/functions_include.php", nr_guzzle6_enable}, + {"Doctrine 2", NR_PSTR("doctrine/orm/query.php"), nr_doctrine2_enable}, + {"Guzzle 3", NR_PSTR("guzzle/http/client.php"), nr_guzzle3_enable}, + {"Guzzle 4-5", NR_PSTR("hasemitterinterface.php"), nr_guzzle4_enable}, + {"Guzzle 6", NR_PSTR("guzzle/src/functions_include.php"), nr_guzzle6_enable}, - {"MongoDB", "mongodb/src/client.php", nr_mongodb_enable}, + {"MongoDB", NR_PSTR("mongodb/src/client.php"), nr_mongodb_enable}, /* * The first path is for Composer installs, the second is for * /usr/local/bin. While BaseTestRunner isn't the very first file to load, * it contains the test status constants and loads before tests can run. */ - {"PHPUnit", "phpunit/src/runner/basetestrunner.php", nr_phpunit_enable}, - {"PHPUnit", "phpunit/runner/basetestrunner.php", nr_phpunit_enable}, + {"PHPUnit", NR_PSTR("phpunit/src/runner/basetestrunner.php"), nr_phpunit_enable}, + {"PHPUnit", NR_PSTR("phpunit/runner/basetestrunner.php"), nr_phpunit_enable}, - {"Predis", "predis/src/client.php", nr_predis_enable}, - {"Predis", "predis/client.php", nr_predis_enable}, + {"Predis", NR_PSTR("predis/src/client.php"), nr_predis_enable}, + {"Predis", NR_PSTR("predis/client.php"), nr_predis_enable}, /* * Allow Zend Framework 1.x to be detected as a library as well as a @@ -498,14 +503,14 @@ static nr_library_table_t libraries[] = { * with other frameworks or even without a framework at all. This is * necessary for Magento in particular, which is built on ZF1. */ - {"Zend_Http", "zend/http/client.php", nr_zend_http_enable}, + {"Zend_Http", NR_PSTR("zend/http/client.php"), nr_zend_http_enable}, /* * Allow Laminas Framework 3.x to be detected as a library as well as a * framework. This allows Laminas_Http_Client to be instrumented when used * with other frameworks or even without a framework at all. */ - {"Laminas_Http", "laminas-http/src/client.php", nr_laminas_http_enable}, + {"Laminas_Http", NR_PSTR("laminas-http/src/client.php"), nr_laminas_http_enable}, /* * Other frameworks, detected only, but not specifically @@ -513,70 +518,73 @@ static nr_library_table_t libraries[] = { * detection of a supported framework or library later (since a transaction * can only have one framework). */ - {"Aura1", "aura/framework/system.php", NULL}, - {"Aura2", "aura/di/src/containerinterface.php", NULL}, - {"Aura3", "aura/di/src/containerconfiginterface.php", NULL}, - {"CakePHP3", "cakephp/src/core/functions.php", NULL}, - {"Fuel", "fuel/core/classes/fuel.php", NULL}, - {"Lithium", "lithium/core/libraries.php", NULL}, - {"Phpbb", "phpbb/request/request.php", NULL}, - {"Phpixie2", "phpixie/core/classes/phpixie/pixie.php", NULL}, - {"Phpixie3", "phpixie/framework.php", NULL}, - {"React", "react/event-loop/src/loopinterface.php", NULL}, - {"SilverStripe", "injector/silverstripeinjectioncreator.php", NULL}, - {"SilverStripe4", "silverstripeserviceconfigurationlocator.php", NULL}, - {"Typo3", "classes/typo3/flow/core/bootstrap.php", NULL}, - {"Typo3", "typo3/sysext/core/classes/core/bootstrap.php", NULL}, - {"Yii2", "yii2/baseyii.php", NULL}, + {"Aura1", NR_PSTR("aura/framework/system.php"), NULL}, + {"Aura2", NR_PSTR("aura/di/src/containerinterface.php"), NULL}, + {"Aura3", NR_PSTR("aura/di/src/containerconfiginterface.php"), NULL}, + {"CakePHP3", NR_PSTR("cakephp/src/core/functions.php"), NULL}, + {"Fuel", NR_PSTR("fuel/core/classes/fuel.php"), NULL}, + {"Lithium", NR_PSTR("lithium/core/libraries.php"), NULL}, + {"Phpbb", NR_PSTR("phpbb/request/request.php"), NULL}, + {"Phpixie2", NR_PSTR("phpixie/core/classes/phpixie/pixie.php"), NULL}, + {"Phpixie3", NR_PSTR("phpixie/framework.php"), NULL}, + {"React", NR_PSTR("react/event-loop/src/loopinterface.php"), NULL}, + {"SilverStripe", NR_PSTR("injector/silverstripeinjectioncreator.php"), NULL}, + {"SilverStripe4", NR_PSTR("silverstripeserviceconfigurationlocator.php"), NULL}, + {"Typo3", NR_PSTR("classes/typo3/flow/core/bootstrap.php"), NULL}, + {"Typo3", NR_PSTR("typo3/sysext/core/classes/core/bootstrap.php"), NULL}, + {"Yii2", NR_PSTR("yii2/baseyii.php"), NULL}, /* * Other CMS (content management systems), detected only, but * not specifically instrumented. */ - {"Moodle", "moodlelib.php", NULL}, + {"Moodle", NR_PSTR("moodlelib.php"), NULL}, /* * It is likely that this will never be found, since the CodeIgniter.php * will get loaded first, and as such mark this transaction as belonging to * CodeIgniter, and not Expession Engine. */ - {"ExpressionEngine", "system/expressionengine/config/config.php", NULL}, + {"ExpressionEngine", NR_PSTR("system/expressionengine/config/config.php"), NULL}, /* * ExpressionEngine 5, however, has a very obvious file we can look for. */ - {"ExpressionEngine5", "expressionengine/boot/boot.php", NULL}, + {"ExpressionEngine5", NR_PSTR("expressionengine/boot/boot.php"), NULL}, /* * DokuWiki uses doku.php as an entry point, but has other files that are * loaded directly that this won't pick up. That's probably OK for * supportability metrics, but we'll add the most common name for the * configuration file as well just in case. */ - {"DokuWiki", "doku.php", NULL}, - {"DokuWiki", "conf/dokuwiki.php", NULL}, + {"DokuWiki", NR_PSTR("doku.php"), NULL}, + {"DokuWiki", NR_PSTR("conf/dokuwiki.php"), NULL}, /* * SugarCRM no longer has a community edition, so this likely only works * with older versions. */ - {"SugarCRM", "sugarobjects/sugarconfig.php", NULL}, + {"SugarCRM", NR_PSTR("sugarobjects/sugarconfig.php"), NULL}, - {"Xoops", "class/xoopsload.php", NULL}, - {"E107", "e107_handlers/e107_class.php", NULL}, + {"Xoops", NR_PSTR("class/xoopsload.php"), NULL}, + {"E107", NR_PSTR("e107_handlers/e107_class.php"), NULL}, }; +// clang-format: on static size_t num_libraries = sizeof(libraries) / sizeof(nr_library_table_t); +// clang-format: off static nr_library_table_t logging_frameworks[] = { /* Monolog - Logging for PHP */ - {"Monolog", "monolog/logger.php", nr_monolog_enable}, + {"Monolog", NR_PSTR("monolog/logger.php"), nr_monolog_enable}, /* Consolidation/Log - Logging for PHP */ - {"Consolidation/Log", "consolidation/log/src/logger.php", NULL}, + {"Consolidation/Log", NR_PSTR("consolidation/log/src/logger.php"), NULL}, /* laminas-log - Logging for PHP */ - {"laminas-log", "laminas-log/src/logger.php", NULL}, + {"laminas-log", NR_PSTR("laminas-log/src/logger.php"), NULL}, /* cakephp-log - Logging for PHP */ - {"cakephp-log", "cakephp/log/log.php", NULL}, + {"cakephp-log", NR_PSTR("cakephp/log/log.php"), NULL}, /* Analog - Logging for PHP */ - {"Analog", "analog/analog.php", NULL}, + {"Analog", NR_PSTR("analog/analog.php"), NULL}, }; +// clang-format: on static size_t num_logging_frameworks = sizeof(logging_frameworks) / sizeof(nr_library_table_t); @@ -705,7 +713,8 @@ static void nr_php_show_exec_return(NR_EXECUTE_PROTO TSRMLS_DC) { static nrframework_t nr_try_detect_framework( const nr_framework_table_t frameworks[], size_t num_frameworks, - const char* filename TSRMLS_DC); + const char* filename, + const size_t filename_len TSRMLS_DC); static nrframework_t nr_try_force_framework( const nr_framework_table_t frameworks[], size_t num_frameworks, @@ -765,7 +774,8 @@ void nr_framework_create_metric(TSRMLS_D) { */ static void nr_execute_handle_framework(const nr_framework_table_t frameworks[], size_t num_frameworks, - const char* filename TSRMLS_DC) { + const char* filename, + const size_t filename_len TSRMLS_DC) { if (NR_FW_UNSET != NRPRG(current_framework)) { return; } @@ -773,8 +783,8 @@ static void nr_execute_handle_framework(const nr_framework_table_t frameworks[], if (NR_FW_UNSET == NRINI(force_framework)) { nrframework_t detected_framework = NR_FW_UNSET; - detected_framework = nr_try_detect_framework(frameworks, num_frameworks, - filename TSRMLS_CC); + detected_framework = nr_try_detect_framework( + frameworks, num_frameworks, filename, filename_len TSRMLS_CC); if (NR_FW_UNSET != detected_framework) { NRPRG(current_framework) = detected_framework; } @@ -792,6 +802,8 @@ static void nr_execute_handle_framework(const nr_framework_table_t frameworks[], } } +#define STR_AND_LEN(v) v, v##_len + /* * Attempt to detect a framework. * Call the appropriate enable function if we find the framework. @@ -800,13 +812,14 @@ static void nr_execute_handle_framework(const nr_framework_table_t frameworks[], static nrframework_t nr_try_detect_framework( const nr_framework_table_t frameworks[], size_t num_frameworks, - const char* filename TSRMLS_DC) { + const char* filename, + const size_t filename_len TSRMLS_DC) { nrframework_t detected = NR_FW_UNSET; - char* filename_lower = nr_string_to_lowercase(filename); size_t i; for (i = 0; i < num_frameworks; i++) { - if (nr_stridx(filename_lower, frameworks[i].file_to_check) >= 0) { + if (nr_striendswith(STR_AND_LEN(filename), + STR_AND_LEN(frameworks[i].file_to_check))) { /* * If we have a special check function and it tells us to ignore * the file name because some other condition wasn't met, continue @@ -830,7 +843,6 @@ static nrframework_t nr_try_detect_framework( } end: - nr_free(filename_lower); return detected; } @@ -869,12 +881,13 @@ static nrframework_t nr_try_force_framework( return NR_FW_UNSET; } -static void nr_execute_handle_library(const char* filename TSRMLS_DC) { - char* filename_lower = nr_string_to_lowercase(filename); +static void nr_execute_handle_library(const char* filename, + const size_t filename_len TSRMLS_DC) { size_t i; for (i = 0; i < num_libraries; i++) { - if (nr_stridx(filename_lower, libraries[i].file_to_check) >= 0) { + if (nr_striendswith(STR_AND_LEN(filename), + STR_AND_LEN(libraries[i].file_to_check))) { nrl_debug(NRL_INSTRUMENT, "detected library=%s", libraries[i].library_name); @@ -886,18 +899,17 @@ static void nr_execute_handle_library(const char* filename TSRMLS_DC) { } } } - - nr_free(filename_lower); } -static void nr_execute_handle_logging_framework( - const char* filename TSRMLS_DC) { - char* filename_lower = nr_string_to_lowercase(filename); +static void nr_execute_handle_logging_framework(const char* filename, + const size_t filename_len + TSRMLS_DC) { bool is_enabled = false; size_t i; for (i = 0; i < num_logging_frameworks; i++) { - if (nr_stridx(filename_lower, logging_frameworks[i].file_to_check) >= 0) { + if (nr_striendswith(STR_AND_LEN(filename), + STR_AND_LEN(logging_frameworks[i].file_to_check))) { nrl_debug(NRL_INSTRUMENT, "detected library=%s", logging_frameworks[i].library_name); @@ -912,10 +924,10 @@ static void nr_execute_handle_logging_framework( NRPRG(txn), logging_frameworks[i].library_name, is_enabled); } } - - nr_free(filename_lower); } +#undef STR_AND_LEN + /* * Purpose : Detect library and framework usage from a PHP file. * @@ -924,12 +936,20 @@ static void nr_execute_handle_logging_framework( * * Params : 1. Full name of a PHP file. */ -static void nr_php_user_instrumentation_from_file( - const char* filename TSRMLS_DC) { - nr_execute_handle_framework(all_frameworks, num_all_frameworks, - filename TSRMLS_CC); - nr_execute_handle_library(filename TSRMLS_CC); - nr_execute_handle_logging_framework(filename TSRMLS_CC); +static void nr_php_user_instrumentation_from_file(const char* filename, + const size_t filename_len + TSRMLS_DC) { + /* short circuit if filename_len is 0; a single place short circuit */ + if (0 == filename_len) { + nrl_verbosedebug(NRL_AGENT, + "%s - received invalid filename_len for file=%s", __func__, + filename); + return; + } + nr_execute_handle_framework(all_frameworks, num_all_frameworks, filename, + filename_len TSRMLS_CC); + nr_execute_handle_library(filename, filename_len TSRMLS_CC); + nr_execute_handle_logging_framework(filename, filename_len TSRMLS_CC); } /* @@ -940,6 +960,7 @@ static void nr_php_user_instrumentation_from_file( static void nr_php_execute_file(const zend_op_array* op_array, NR_EXECUTE_PROTO TSRMLS_DC) { const char* filename = nr_php_op_array_file_name(op_array); + size_t filename_len = nr_php_op_array_file_name_len(op_array); if (nrunlikely(NR_PHP_PROCESS_GLOBALS(special_flags).show_loaded_files)) { nrl_debug(NRL_AGENT, "loaded file=" NRP_FMT, NRP_FILENAME(filename)); @@ -948,7 +969,7 @@ static void nr_php_execute_file(const zend_op_array* op_array, /* * Check for, and handle, frameworks and libraries. */ - nr_php_user_instrumentation_from_file(filename TSRMLS_CC); + nr_php_user_instrumentation_from_file(filename, filename_len TSRMLS_CC); nr_txn_match_file(NRPRG(txn), filename); @@ -1673,6 +1694,7 @@ void nr_php_user_instrumentation_from_opcache(TSRMLS_D) { nr_php_string_hash_key_t* key_str = NULL; zval* val = NULL; const char* filename; + size_t filename_len; status = nr_php_call(NULL, "opcache_get_status"); @@ -1717,8 +1739,9 @@ void nr_php_user_instrumentation_from_opcache(TSRMLS_D) { (void)val; filename = ZEND_STRING_VALUE(key_str); + filename_len = ZEND_STRING_LEN(key_str); - nr_php_user_instrumentation_from_file(filename TSRMLS_CC); + nr_php_user_instrumentation_from_file(filename, filename_len TSRMLS_CC); } ZEND_HASH_FOREACH_END(); diff --git a/axiom/tests/test_strings.c b/axiom/tests/test_strings.c index ea1576e36..053d2bc60 100644 --- a/axiom/tests/test_strings.c +++ b/axiom/tests/test_strings.c @@ -1228,6 +1228,35 @@ static void test_str_append(void) { nr_free(str); } +static void test_iendswith(void) { + tlib_pass_if_bool_equal("input is NULL", false, + nr_striendswith(NULL, 4, NR_PSTR("bar"))); + + tlib_pass_if_bool_equal("pattern is NULL", false, + nr_striendswith(NR_PSTR("foo"), NULL, 0)); + + tlib_pass_if_bool_equal("input is empty", false, + nr_striendswith(NR_PSTR(""), NR_PSTR("bar"))); + + tlib_pass_if_bool_equal("pattern is empty", true, + nr_striendswith(NR_PSTR("foo"), NR_PSTR(""))); + + tlib_pass_if_bool_equal("input is too short", false, + nr_striendswith(NR_PSTR("ar"), NR_PSTR("bar"))); + + tlib_pass_if_bool_equal( + "no match", false, nr_striendswith(NR_PSTR("foobarbaz"), NR_PSTR("bar"))); + + tlib_pass_if_bool_equal("not quite match", false, + nr_striendswith(NR_PSTR("foobarr"), NR_PSTR("bar"))); + + tlib_pass_if_bool_equal("suffix match", true, + nr_striendswith(NR_PSTR("foobar"), NR_PSTR("bar"))); + + tlib_pass_if_bool_equal("exact match", true, + nr_striendswith(NR_PSTR("bar"), NR_PSTR("bar"))); +} + tlib_parallel_info_t parallel_info = {.suggested_nthreads = 2, .state_size = 0}; void test_main(void* p NRUNUSED) { @@ -1265,6 +1294,7 @@ void test_main(void* p NRUNUSED) { test_formatf(); test_strsplit(); test_str_append(); + test_iendswith(); /* * character tests diff --git a/axiom/util_strings.h b/axiom/util_strings.h index c1d61e0b4..5da515cab 100644 --- a/axiom/util_strings.h +++ b/axiom/util_strings.h @@ -16,6 +16,7 @@ #include "nr_axiom.h" +#include #include #include "util_object.h" @@ -438,4 +439,36 @@ static inline int nr_toupper(int c) { return c; } +/* + * Purpose : Checks whether a string ends with the specified pattern. Note that + * nr_striendswith is case-insensitive. + * + * Params : 1. The input string to examine. + * 2. The input string length. + * 3. The pattern to look for at the end of the string. + * 4. The pattern length. + * + * Returns : The true if input string ends with the pattern or false otherwise. + */ +static inline bool nr_striendswith(const char* s, + const size_t slen, + const char* pattern, + const size_t pattern_len) { + const char* suffix; + + if (slen < pattern_len) { + /* input shorter than pattern */ + return false; + } + + if (NULL == s) { + /* invalid input */ + return false; + } + + /* compare input's suffix with the pattern and return result */ + suffix = s + (slen - pattern_len); + return 0 == nr_stricmp(suffix, pattern); +} + #endif /* UTIL_STRINGS_HDR */ From b566621852d88ffe324fffd1ccf762c53fb5272f Mon Sep 17 00:00:00 2001 From: bduranleau-nr <106178551+bduranleau-nr@users.noreply.github.com> Date: Mon, 4 Dec 2023 15:17:20 -0600 Subject: [PATCH 08/17] refactor: compile wordpress hook regex at minit (#777) Improve agent's performance by compiling `wordpress_hook_regex` once per process at MINIT rather than on every request at RINIT, (and be clean up at MSHUTDOWN rather than RSHUTDOWN). --- agent/fw_wordpress.c | 14 +++++++++++++- agent/fw_wordpress.h | 3 +++ agent/php_minit.c | 5 ++--- agent/php_mshutdown.c | 3 +++ agent/php_newrelic.h | 1 - agent/php_rinit.c | 8 -------- agent/php_rshutdown.c | 1 - 7 files changed, 21 insertions(+), 14 deletions(-) diff --git a/agent/fw_wordpress.c b/agent/fw_wordpress.c index 4f9544636..f8fe2bc34 100644 --- a/agent/fw_wordpress.c +++ b/agent/fw_wordpress.c @@ -21,6 +21,8 @@ #define NR_WORDPRESS_HOOK_PREFIX "Framework/WordPress/Hook/" #define NR_WORDPRESS_PLUGIN_PREFIX "Framework/WordPress/Plugin/" +static nr_regex_t* wordpress_hook_regex; + static size_t zval_len_without_trailing_slash(const zval* zstr) { nr_string_len_t len = Z_STRLEN_P(zstr); const char* str = Z_STRVAL_P(zstr); @@ -428,7 +430,7 @@ static char* nr_wordpress_clean_tag(const zval* tag TSRMLS_DC) { return NULL; } - regex = NRPRG(wordpress_hook_regex); + regex = wordpress_hook_regex; if (NULL == regex) { return NULL; } @@ -594,3 +596,13 @@ void nr_wordpress_enable(TSRMLS_D) { nr_php_add_call_user_func_array_pre_callback( nr_wordpress_call_user_func_array TSRMLS_CC); } + +void nr_wordpress_minit(void) { + wordpress_hook_regex = nr_regex_create( + "(^([a-z_-]+[_-])([0-9a-f_.]+[0-9][0-9a-f.]+)(_{0,1}.*)$|(.*))", + NR_REGEX_CASELESS, 0); +} + +void nr_wordpress_mshutdown(void) { + nr_regex_destroy(&wordpress_hook_regex); +} diff --git a/agent/fw_wordpress.h b/agent/fw_wordpress.h index 3ec916ab1..e8023b420 100644 --- a/agent/fw_wordpress.h +++ b/agent/fw_wordpress.h @@ -29,4 +29,7 @@ char* nr_php_wordpress_core_match_regex(const char* filename TSRMLS_DC); */ char* nr_php_wordpress_plugin_match_regex(const char* filename TSRMLS_DC); +extern void nr_wordpress_minit(void); +extern void nr_wordpress_mshutdown(void); + #endif /* FW_WORDPRESS_HDR */ diff --git a/agent/php_minit.c b/agent/php_minit.c index 64aea90ec..7babac21a 100644 --- a/agent/php_minit.c +++ b/agent/php_minit.c @@ -22,6 +22,7 @@ #include "php_vm.h" #include "php_wrapper.h" #include "fw_laravel.h" +#include "fw_wordpress.h" #include "lib_guzzle4.h" #include "lib_guzzle6.h" #include "nr_agent.h" @@ -35,9 +36,6 @@ #include "util_syscalls.h" #include "util_threads.h" -#include "fw_laravel.h" -#include "lib_guzzle4.h" - static void php_newrelic_init_globals(zend_newrelic_globals* nrg) { if (nrunlikely(NULL == nrg)) { return; @@ -709,6 +707,7 @@ PHP_MINIT_FUNCTION(newrelic) { nr_guzzle4_minit(TSRMLS_C); nr_guzzle6_minit(TSRMLS_C); nr_laravel_minit(TSRMLS_C); + nr_wordpress_minit(); nr_php_set_opcode_handlers(); nrl_debug(NRL_INIT, "MINIT processing done"); diff --git a/agent/php_mshutdown.c b/agent/php_mshutdown.c index c290020ba..1c67d38b4 100644 --- a/agent/php_mshutdown.c +++ b/agent/php_mshutdown.c @@ -15,6 +15,7 @@ #include "php_vm.h" #include "nr_agent.h" #include "util_logging.h" +#include "fw_wordpress.h" #ifdef TAGS void zm_shutdown_newrelic(void); /* ctags landing pad only */ @@ -39,6 +40,8 @@ PHP_MSHUTDOWN_FUNCTION(newrelic) { */ nrl_debug(NRL_INIT, "MSHUTDOWN processing started"); + nr_wordpress_mshutdown(); + /* restore header handler */ sapi_module.header_handler = NR_PHP_PROCESS_GLOBALS(orig_header_handler); NR_PHP_PROCESS_GLOBALS(orig_header_handler) = NULL; diff --git a/agent/php_newrelic.h b/agent/php_newrelic.h index d7edaba65..9222c777f 100644 --- a/agent/php_newrelic.h +++ b/agent/php_newrelic.h @@ -379,7 +379,6 @@ int symfony1_in_error404; /* Whether we are currently within a sfError404Exception::printStackTrace() frame */ char* wordpress_tag; /* The current WordPress tag */ -nr_regex_t* wordpress_hook_regex; /* Regex to sanitize hook names */ nr_regex_t* wordpress_plugin_regex; /* Regex for plugin filenames */ nr_regex_t* wordpress_theme_regex; /* Regex for theme filenames */ nr_regex_t* wordpress_core_regex; /* Regex for plugin filenames */ diff --git a/agent/php_rinit.c b/agent/php_rinit.c index 7295821df..e98c5b198 100644 --- a/agent/php_rinit.c +++ b/agent/php_rinit.c @@ -89,14 +89,6 @@ PHP_RINIT_FUNCTION(newrelic) { nr_php_extension_instrument_rescan(NRPRG(extensions) TSRMLS_CC); } - /* - * Compile regex for WordPress: includes logic for - * hook sanitization regex. - */ - NRPRG(wordpress_hook_regex) = nr_regex_create( - "(^([a-z_-]+[_-])([0-9a-f_.]+[0-9][0-9a-f.]+)(_{0,1}.*)$|(.*))", - NR_REGEX_CASELESS, 0); - NRPRG(mysql_last_conn) = NULL; NRPRG(pgsql_last_conn) = NULL; NRPRG(datastore_connections) = nr_hashmap_create( diff --git a/agent/php_rshutdown.c b/agent/php_rshutdown.c index 184a07c10..a4cee06c2 100644 --- a/agent/php_rshutdown.c +++ b/agent/php_rshutdown.c @@ -98,7 +98,6 @@ int nr_php_post_deactivate(void) { nr_php_exception_filters_destroy(&NRPRG(exception_filters)); nr_regex_destroy(&NRPRG(wordpress_plugin_regex)); nr_regex_destroy(&NRPRG(wordpress_core_regex)); - nr_regex_destroy(&NRPRG(wordpress_hook_regex)); nr_regex_destroy(&NRPRG(wordpress_theme_regex)); nr_hashmap_destroy(&NRPRG(wordpress_file_metadata)); From 0150c096dc959dad60f820defb95d031d692aef9 Mon Sep 17 00:00:00 2001 From: bduranleau-nr <106178551+bduranleau-nr@users.noreply.github.com> Date: Mon, 4 Dec 2023 15:18:15 -0600 Subject: [PATCH 09/17] refactor: cache wordpress clean tag values (#778) Improve agent's performance by caching WordPress 'clean_tag' values of transient tags (hooks) rather than cleaning them with regex matching each time they are encountered in a request. --- agent/fw_wordpress.c | 29 ++++++++++++++++++++--------- agent/php_newrelic.h | 1 + agent/php_rshutdown.c | 1 + 3 files changed, 22 insertions(+), 9 deletions(-) diff --git a/agent/fw_wordpress.c b/agent/fw_wordpress.c index f8fe2bc34..ad67b9368 100644 --- a/agent/fw_wordpress.c +++ b/agent/fw_wordpress.c @@ -412,6 +412,10 @@ static void nr_wordpress_call_user_func_array(zend_function* func, nr_php_wrap_callable(func, nr_wordpress_wrap_hook TSRMLS_CC); } +static void free_tag(void* tag) { + nr_free(tag); +} + /* * Some plugins generate transient tag names. We can detect these by checking * the substrings returned from our regex rule. If the tag is transient, we @@ -420,8 +424,7 @@ static void nr_wordpress_call_user_func_array(zend_function* func, * Example: (old) add_option__transient_timeout_twccr_382402301f44c883bc0137_cat * (new) add_option__transient_timeout_twccr_*_cat */ -static char* nr_wordpress_clean_tag(const zval* tag TSRMLS_DC) { - char* orig_tag = NULL; +static char* nr_wordpress_clean_tag(const zval* tag) { char* clean_tag = NULL; nr_regex_t* regex = NULL; nr_regex_substrings_t* ss = NULL; @@ -435,8 +438,16 @@ static char* nr_wordpress_clean_tag(const zval* tag TSRMLS_DC) { return NULL; } - orig_tag = nr_strndup(Z_STRVAL_P(tag), Z_STRLEN_P(tag)); - ss = nr_regex_match_capture(regex, orig_tag, nr_strlen(orig_tag)); + if (NULL == NRPRG(wordpress_clean_tag_cache)) { + NRPRG(wordpress_clean_tag_cache) = nr_hashmap_create(free_tag); + } + + if (nr_hashmap_get_into(NRPRG(wordpress_clean_tag_cache), Z_STRVAL_P(tag), + Z_STRLEN_P(tag), (void**)&clean_tag)) { + return clean_tag; + } + + ss = nr_regex_match_capture(regex, Z_STRVAL_P(tag), Z_STRLEN_P(tag)); clean_tag = nr_regex_substrings_get(ss, 5); /* @@ -457,7 +468,9 @@ static char* nr_wordpress_clean_tag(const zval* tag TSRMLS_DC) { } nr_regex_substrings_destroy(&ss); - nr_free(orig_tag); + + nr_hashmap_set(NRPRG(wordpress_clean_tag_cache), Z_STRVAL_P(tag), + Z_STRLEN_P(tag), clean_tag); return clean_tag; } @@ -481,9 +494,8 @@ NR_PHP_WRAPPER(nr_wordpress_exec_handle_tag) { */ char* old_tag = NRPRG(wordpress_tag); - NRPRG(wordpress_tag) = nr_wordpress_clean_tag(tag TSRMLS_CC); + NRPRG(wordpress_tag) = nr_wordpress_clean_tag(tag); NR_PHP_WRAPPER_CALL; - nr_free(NRPRG(wordpress_tag)); NRPRG(wordpress_tag) = old_tag; } else { NR_PHP_WRAPPER_CALL; @@ -563,9 +575,8 @@ NR_PHP_WRAPPER(nr_wordpress_apply_filters) { */ char* old_tag = NRPRG(wordpress_tag); - NRPRG(wordpress_tag) = nr_wordpress_clean_tag(tag TSRMLS_CC); + NRPRG(wordpress_tag) = nr_wordpress_clean_tag(tag); NR_PHP_WRAPPER_CALL; - nr_free(NRPRG(wordpress_tag)); NRPRG(wordpress_tag) = old_tag; } else { NR_PHP_WRAPPER_CALL; diff --git a/agent/php_newrelic.h b/agent/php_newrelic.h index 9222c777f..0baed648b 100644 --- a/agent/php_newrelic.h +++ b/agent/php_newrelic.h @@ -384,6 +384,7 @@ nr_regex_t* wordpress_theme_regex; /* Regex for theme filenames */ nr_regex_t* wordpress_core_regex; /* Regex for plugin filenames */ nr_hashmap_t* wordpress_file_metadata; /* Metadata for plugin and theme names given a filename */ +nr_hashmap_t* wordpress_clean_tag_cache; /* Cached clean tags */ char* doctrine_dql; /* The current Doctrine DQL. Only non-NULL while a Doctrine object is on the stack. */ diff --git a/agent/php_rshutdown.c b/agent/php_rshutdown.c index a4cee06c2..b48a3ffd7 100644 --- a/agent/php_rshutdown.c +++ b/agent/php_rshutdown.c @@ -100,6 +100,7 @@ int nr_php_post_deactivate(void) { nr_regex_destroy(&NRPRG(wordpress_core_regex)); nr_regex_destroy(&NRPRG(wordpress_theme_regex)); nr_hashmap_destroy(&NRPRG(wordpress_file_metadata)); + nr_hashmap_destroy(&NRPRG(wordpress_clean_tag_cache)); nr_free(NRPRG(mysql_last_conn)); nr_free(NRPRG(pgsql_last_conn)); From d2db36dbfcc87a23ef16b12c12b4b877ff590453 Mon Sep 17 00:00:00 2001 From: Amber Sistla Date: Mon, 4 Dec 2023 15:08:33 -0700 Subject: [PATCH 10/17] refactor: only call cufa callback when needed (#769) Improve agent's performance by only calling `call_user_function_array` callback when trying to instrument Drupal or WordPress hooks. Imported from #729. --------- Co-authored-by: Michal Nowacki Co-authored-by: bduranleau-nr <106178551+bduranleau-nr@users.noreply.github.com> --- agent/fw_drupal.c | 4 ++++ agent/fw_drupal8.c | 5 ++++- agent/fw_wordpress.c | 13 +++++++++++++ agent/php_newrelic.h | 3 +++ agent/php_rinit.c | 2 ++ agent/php_vm.c | 8 ++++++++ agent/tests/test_internal_instrument.c | 2 ++ 7 files changed, 36 insertions(+), 1 deletion(-) diff --git a/agent/fw_drupal.c b/agent/fw_drupal.c index 13fcc68a9..5305ab480 100644 --- a/agent/fw_drupal.c +++ b/agent/fw_drupal.c @@ -608,12 +608,16 @@ NR_PHP_WRAPPER(nr_drupal_wrap_module_invoke_all) { NRPRG(drupal_module_invoke_all_hook) = nr_strndup(Z_STRVAL_P(hook), Z_STRLEN_P(hook)); NRPRG(drupal_module_invoke_all_hook_len) = Z_STRLEN_P(hook); + NRPRG(check_cufa) = true; NR_PHP_WRAPPER_CALL; nr_free(NRPRG(drupal_module_invoke_all_hook)); NRPRG(drupal_module_invoke_all_hook) = prev_hook; NRPRG(drupal_module_invoke_all_hook_len) = prev_hook_len; + if (NULL == NRPRG(drupal_module_invoke_all_hook)) { + NRPRG(check_cufa) = false; + } leave: nr_php_arg_release(&hook); diff --git a/agent/fw_drupal8.c b/agent/fw_drupal8.c index 05849031a..77695e058 100644 --- a/agent/fw_drupal8.c +++ b/agent/fw_drupal8.c @@ -418,7 +418,7 @@ NR_PHP_WRAPPER(nr_drupal94_invoke_all_with) { NRPRG(drupal_module_invoke_all_hook) = nr_strndup(Z_STRVAL_P(hook), Z_STRLEN_P(hook)); NRPRG(drupal_module_invoke_all_hook_len) = Z_STRLEN_P(hook); - + NRPRG(check_cufa) = true; callback = nr_php_arg_get(2, NR_EXECUTE_ORIG_ARGS TSRMLS_CC); /* This instrumentation will fail if callback has already been wrapped * with a special instrumentation callback in a different context. @@ -432,6 +432,9 @@ NR_PHP_WRAPPER(nr_drupal94_invoke_all_with) { nr_free(NRPRG(drupal_module_invoke_all_hook)); NRPRG(drupal_module_invoke_all_hook) = prev_hook; NRPRG(drupal_module_invoke_all_hook_len) = prev_hook_len; + if (NULL == NRPRG(drupal_module_invoke_all_hook)) { + NRPRG(check_cufa) = false; + } leave: nr_php_arg_release(&hook); diff --git a/agent/fw_wordpress.c b/agent/fw_wordpress.c index ad67b9368..2c419b7ef 100644 --- a/agent/fw_wordpress.c +++ b/agent/fw_wordpress.c @@ -494,10 +494,16 @@ NR_PHP_WRAPPER(nr_wordpress_exec_handle_tag) { */ char* old_tag = NRPRG(wordpress_tag); + NRPRG(check_cufa) = true; + NRPRG(wordpress_tag) = nr_wordpress_clean_tag(tag); NR_PHP_WRAPPER_CALL; NRPRG(wordpress_tag) = old_tag; + if (NULL == NRPRG(wordpress_tag)) { + NRPRG(check_cufa) = false; + } } else { + NRPRG(check_cufa) = false; NR_PHP_WRAPPER_CALL; } @@ -575,10 +581,17 @@ NR_PHP_WRAPPER(nr_wordpress_apply_filters) { */ char* old_tag = NRPRG(wordpress_tag); + NRPRG(check_cufa) = true; + NRPRG(wordpress_tag) = nr_wordpress_clean_tag(tag); + NR_PHP_WRAPPER_CALL; NRPRG(wordpress_tag) = old_tag; + if (NULL == NRPRG(wordpress_tag)) { + NRPRG(check_cufa) = false; + } } else { + NRPRG(check_cufa) = false; NR_PHP_WRAPPER_CALL; } diff --git a/agent/php_newrelic.h b/agent/php_newrelic.h index 0baed648b..85887fe29 100644 --- a/agent/php_newrelic.h +++ b/agent/php_newrelic.h @@ -378,6 +378,9 @@ int symfony1_in_dispatch; /* Whether we are currently within a int symfony1_in_error404; /* Whether we are currently within a sfError404Exception::printStackTrace() frame */ +bool check_cufa; /* Whether we need to check cufa because we are + instrumenting hooks, or whether we can skip cufa */ + char* wordpress_tag; /* The current WordPress tag */ nr_regex_t* wordpress_plugin_regex; /* Regex for plugin filenames */ nr_regex_t* wordpress_theme_regex; /* Regex for theme filenames */ diff --git a/agent/php_rinit.c b/agent/php_rinit.c index e98c5b198..c8045f2e1 100644 --- a/agent/php_rinit.c +++ b/agent/php_rinit.c @@ -89,6 +89,8 @@ PHP_RINIT_FUNCTION(newrelic) { nr_php_extension_instrument_rescan(NRPRG(extensions) TSRMLS_CC); } + NRPRG(check_cufa) = false; + NRPRG(mysql_last_conn) = NULL; NRPRG(pgsql_last_conn) = NULL; NRPRG(datastore_connections) = nr_hashmap_create( diff --git a/agent/php_vm.c b/agent/php_vm.c index 19f4bb215..4f8d7c023 100644 --- a/agent/php_vm.c +++ b/agent/php_vm.c @@ -108,6 +108,14 @@ static int nr_php_handle_cufa_fcall(zend_execute_data* execute_data) { goto call_previous_and_return; } + /* + * If we don't have instrumented hooks that require this, skip to the + * end. + */ + if (false == NRPRG(check_cufa)) { + goto call_previous_and_return; + } + /* * Since we're in the middle of a function call, the Zend Engine is actually * only partway through constructing the new function frame. As a result, it diff --git a/agent/tests/test_internal_instrument.c b/agent/tests/test_internal_instrument.c index de3f19619..0f4f2b3f8 100644 --- a/agent/tests/test_internal_instrument.c +++ b/agent/tests/test_internal_instrument.c @@ -49,6 +49,7 @@ static void test_cufa_direct(TSRMLS_D) { tlib_php_request_start(); + NRPRG(check_cufa) = true; define_cufa_function_f(TSRMLS_C); tlib_php_request_eval( "function g() { return call_user_func_array('f', array()); }" TSRMLS_CC); @@ -74,6 +75,7 @@ static void test_cufa_indirect(TSRMLS_D) { tlib_php_request_start(); + NRPRG(check_cufa) = true; define_cufa_function_f(TSRMLS_C); tlib_php_request_eval( "function g() { $cufa = 'call_user_func_array'; return $cufa('f', " From 1d165dcb7548a1001d7cc5804151ba9896e49cd3 Mon Sep 17 00:00:00 2001 From: Michal Nowacki Date: Tue, 5 Dec 2023 23:26:14 -0500 Subject: [PATCH 11/17] refactor: limit wordpress instrumentation (#786) Improve agent's performance when WordPress hook tracking is disabled with `newrelic.framework.wordpress.hooks` set to `false`. There's no need to wrap WordPress 'hook' functions when WordPress hooks are not to be instrumented. In such case, instrumentation is limited to transaction naming only. --- agent/fw_wordpress.c | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/agent/fw_wordpress.c b/agent/fw_wordpress.c index 2c419b7ef..0a33d7382 100644 --- a/agent/fw_wordpress.c +++ b/agent/fw_wordpress.c @@ -481,18 +481,18 @@ NR_PHP_WRAPPER(nr_wordpress_exec_handle_tag) { NR_UNUSED_SPECIALFN; (void)wraprec; - NR_PHP_WRAPPER_REQUIRE_FRAMEWORK(NR_FW_WORDPRESS); + if (nrlikely(0 != NRINI(wordpress_hooks))) { + NR_PHP_WRAPPER_REQUIRE_FRAMEWORK(NR_FW_WORDPRESS); - tag = nr_php_arg_get(1, NR_EXECUTE_ORIG_ARGS TSRMLS_CC); + tag = nr_php_arg_get(1, NR_EXECUTE_ORIG_ARGS TSRMLS_CC); - if (1 == nr_php_is_zval_non_empty_string(tag) - || (0 != NRINI(wordpress_hooks))) { - /* - * Our general approach here is to set the wordpress_tag global, then let - * the call_user_func_array instrumentation take care of actually timing - * the hooks by checking if it's set. - */ - char* old_tag = NRPRG(wordpress_tag); + if (1 == nr_php_is_zval_non_empty_string(tag)) { + /* + * Our general approach here is to set the wordpress_tag global, then let + * the call_user_func_array instrumentation take care of actually timing + * the hooks by checking if it's set. + */ + char* old_tag = NRPRG(wordpress_tag); NRPRG(check_cufa) = true; @@ -507,7 +507,8 @@ NR_PHP_WRAPPER(nr_wordpress_exec_handle_tag) { NR_PHP_WRAPPER_CALL; } - nr_php_arg_release(&tag); + nr_php_arg_release(&tag); + } } NR_PHP_WRAPPER_END @@ -608,17 +609,19 @@ void nr_wordpress_enable(TSRMLS_D) { nr_php_wrap_user_function(NR_PSTR("apply_filters"), nr_wordpress_apply_filters TSRMLS_CC); - nr_php_wrap_user_function(NR_PSTR("apply_filters_ref_array"), - nr_wordpress_exec_handle_tag TSRMLS_CC); + if (0 != NRINI(wordpress_hooks)) { + nr_php_wrap_user_function(NR_PSTR("apply_filters_ref_array"), + nr_wordpress_exec_handle_tag TSRMLS_CC); - nr_php_wrap_user_function(NR_PSTR("do_action"), - nr_wordpress_exec_handle_tag TSRMLS_CC); + nr_php_wrap_user_function(NR_PSTR("do_action"), + nr_wordpress_exec_handle_tag TSRMLS_CC); - nr_php_wrap_user_function(NR_PSTR("do_action_ref_array"), - nr_wordpress_exec_handle_tag TSRMLS_CC); + nr_php_wrap_user_function(NR_PSTR("do_action_ref_array"), + nr_wordpress_exec_handle_tag TSRMLS_CC); - nr_php_add_call_user_func_array_pre_callback( - nr_wordpress_call_user_func_array TSRMLS_CC); + nr_php_add_call_user_func_array_pre_callback( + nr_wordpress_call_user_func_array TSRMLS_CC); + } } void nr_wordpress_minit(void) { From e9f3e6f0a7257e179915efe0f43f919c65645ca6 Mon Sep 17 00:00:00 2001 From: Michal Nowacki Date: Wed, 6 Dec 2023 10:45:06 -0500 Subject: [PATCH 12/17] tests: add wordpress transaction naming tests (#793) Add tests that verify that the agent names WordPress web transaction as an 'Action' with the name generated from the template used to generate the page when WordPress hooks are enabled as well as disabled. --- src/newrelic/integration/parse.go | 51 +++++++++-------- src/newrelic/integration/test.go | 32 +++++++---- ...t_wordpress_transaction_name_hooks_off.php | 55 +++++++++++++++++++ ...st_wordpress_transaction_name_hooks_on.php | 53 ++++++++++++++++++ 4 files changed, 157 insertions(+), 34 deletions(-) create mode 100644 tests/integration/frameworks/wordpress/test_wordpress_transaction_name_hooks_off.php create mode 100644 tests/integration/frameworks/wordpress/test_wordpress_transaction_name_hooks_on.php diff --git a/src/newrelic/integration/parse.go b/src/newrelic/integration/parse.go index f44c8d22a..f464f7736 100644 --- a/src/newrelic/integration/parse.go +++ b/src/newrelic/integration/parse.go @@ -19,29 +19,30 @@ import ( var ( Directives = map[string]func(*Test, []byte) error{ - "ENVIRONMENT": parseEnv, - "HEADERS": parseHeaders, - "SKIPIF": parseRawSkipIf, - "INI": parseSettings, - "CONFIG": parseConfig, - "DESCRIPTION": parseDescription, - "EXPECT_ANALYTICS_EVENTS": parseAnalyticEvents, - "EXPECT_CUSTOM_EVENTS": parseCustomEvents, - "EXPECT_ERROR_EVENTS": parseErrorEvents, - "EXPECT_SPAN_EVENTS": parseSpanEvents, - "EXPECT_SPAN_EVENTS_LIKE": parseSpanEventsLike, - "EXPECT_LOG_EVENTS": parseLogEvents, - "EXPECT_METRICS": parseMetrics, - "EXPECT_METRICS_EXIST": parseMetricsExist, - "EXPECT": parseExpect, - "EXPECT_REGEX": parseExpectRegex, - "EXPECT_SCRUBBED": parseExpectScrubbed, - "EXPECT_HARVEST": parseExpectHarvest, - "EXPECT_SLOW_SQLS": parseSlowSQLs, - "EXPECT_TRACED_ERRORS": parseTracedErrors, - "EXPECT_TXN_TRACES": parseTxnTraces, - "EXPECT_RESPONSE_HEADERS": parseResponseHeaders, - "XFAIL": parseXFail, + "ENVIRONMENT": parseEnv, + "HEADERS": parseHeaders, + "SKIPIF": parseRawSkipIf, + "INI": parseSettings, + "CONFIG": parseConfig, + "DESCRIPTION": parseDescription, + "EXPECT_ANALYTICS_EVENTS": parseAnalyticEvents, + "EXPECT_CUSTOM_EVENTS": parseCustomEvents, + "EXPECT_ERROR_EVENTS": parseErrorEvents, + "EXPECT_SPAN_EVENTS": parseSpanEvents, + "EXPECT_SPAN_EVENTS_LIKE": parseSpanEventsLike, + "EXPECT_LOG_EVENTS": parseLogEvents, + "EXPECT_METRICS": parseMetrics, + "EXPECT_METRICS_EXIST": parseMetricsExist, + "EXPECT_METRICS_DONT_EXIST": parseMetricsDontExist, + "EXPECT": parseExpect, + "EXPECT_REGEX": parseExpectRegex, + "EXPECT_SCRUBBED": parseExpectScrubbed, + "EXPECT_HARVEST": parseExpectHarvest, + "EXPECT_SLOW_SQLS": parseSlowSQLs, + "EXPECT_TRACED_ERRORS": parseTracedErrors, + "EXPECT_TXN_TRACES": parseTxnTraces, + "EXPECT_RESPONSE_HEADERS": parseResponseHeaders, + "XFAIL": parseXFail, } ) @@ -229,6 +230,10 @@ func parseMetricsExist(test *Test, content []byte) error { test.metricsExist = content return nil } +func parseMetricsDontExist(test *Test, content []byte) error { + test.metricsDontExist = content + return nil +} func parseSlowSQLs(test *Test, content []byte) error { test.slowSQLs = content return nil diff --git a/src/newrelic/integration/test.go b/src/newrelic/integration/test.go index 25945964d..a82948e15 100644 --- a/src/newrelic/integration/test.go +++ b/src/newrelic/integration/test.go @@ -29,17 +29,18 @@ type Test struct { Desc string // Expected Data - analyticEvents []byte - customEvents []byte - errorEvents []byte - spanEvents []byte - spanEventsLike []byte - logEvents []byte - metrics []byte - metricsExist []byte - slowSQLs []byte - tracedErrors []byte - txnTraces []byte + analyticEvents []byte + customEvents []byte + errorEvents []byte + spanEvents []byte + spanEventsLike []byte + logEvents []byte + metrics []byte + metricsExist []byte + metricsDontExist []byte + slowSQLs []byte + tracedErrors []byte + txnTraces []byte // Expected Output expect []byte expectRegex []byte @@ -567,6 +568,15 @@ func (t *Test) Compare(harvest *newrelic.Harvest) { } } + if nil != t.metricsDontExist { + for _, name := range strings.Split(strings.TrimSpace(string(t.metricsDontExist)), "\n") { + name = strings.TrimSpace(name) + if harvest.Metrics.Has(name) { + t.Fail(fmt.Errorf("unexpected metric in harvest: %s\n\nactual metric table: %s", name, harvest.Metrics.DebugJSON())) + } + } + } + // if we expect a harvest and these is not, then we run our tests as per normal t.compareResponseHeaders() diff --git a/tests/integration/frameworks/wordpress/test_wordpress_transaction_name_hooks_off.php b/tests/integration/frameworks/wordpress/test_wordpress_transaction_name_hooks_off.php new file mode 100644 index 000000000..f2b9285e5 --- /dev/null +++ b/tests/integration/frameworks/wordpress/test_wordpress_transaction_name_hooks_off.php @@ -0,0 +1,55 @@ + Date: Wed, 6 Dec 2023 09:49:19 -0600 Subject: [PATCH 13/17] refactor(agent): use string parsing instead of regex to detect WordPress plugins and themes (#765) Replace Regex-intensive logic for getting the plugin, theme and core names on WP instrumentation with string parsing logic ("matchers") that *should* be faster. --------- Co-authored-by: Michal Nowacki --- agent/fw_wordpress.c | 254 +++++++++++++------------------- agent/fw_wordpress.h | 21 ++- agent/php_agent.h | 5 + agent/php_newrelic.h | 15 +- agent/php_rshutdown.c | 7 +- agent/tests/test_fw_wordpress.c | 68 ++++----- axiom/Makefile | 1 + axiom/tests/.gitignore | 1 + axiom/tests/Makefile | 1 + axiom/tests/test_matcher.c | 111 ++++++++++++++ axiom/util_matcher.c | 152 +++++++++++++++++++ axiom/util_matcher.h | 39 +++++ axiom/util_matcher_private.h | 15 ++ 13 files changed, 486 insertions(+), 204 deletions(-) create mode 100644 axiom/tests/test_matcher.c create mode 100644 axiom/util_matcher.c create mode 100644 axiom/util_matcher.h create mode 100644 axiom/util_matcher_private.h diff --git a/agent/fw_wordpress.c b/agent/fw_wordpress.c index 0a33d7382..793245c54 100644 --- a/agent/fw_wordpress.c +++ b/agent/fw_wordpress.c @@ -13,6 +13,7 @@ #include "fw_hooks.h" #include "fw_support.h" #include "util_logging.h" +#include "util_matcher.h" #include "util_memory.h" #include "util_regex.h" #include "util_strings.h" @@ -23,99 +24,22 @@ static nr_regex_t* wordpress_hook_regex; -static size_t zval_len_without_trailing_slash(const zval* zstr) { - nr_string_len_t len = Z_STRLEN_P(zstr); - const char* str = Z_STRVAL_P(zstr); - - if ((len > 0) && ('/' == str[len - 1])) { - len -= 1; - } - - return (size_t)len; -} - -static void add_wildcard_path_component(nrbuf_t* buf) { - nr_buffer_add(buf, NR_PSTR("/(.*?)/|plugins/([^/]*)[.]php$")); -} - -static nr_regex_t* compile_regex_for_path(const zval* path) { - nrbuf_t* buf = nr_buffer_create(2 * Z_STRLEN_P(path), 0); - nr_regex_t* regex = NULL; - - nr_regex_add_quoted_to_buffer(buf, Z_STRVAL_P(path), - zval_len_without_trailing_slash(path)); - add_wildcard_path_component(buf); - nr_buffer_add(buf, NR_PSTR("\0")); - regex = nr_regex_create(nr_buffer_cptr(buf), NR_REGEX_CASELESS, 1); - - nr_buffer_destroy(&buf); - return regex; -} - -static nr_regex_t* compile_regex_for_path_array(const zval* paths) { - nrbuf_t* buf = nr_buffer_create(0, 0); - int first = 1; - zval* path = NULL; - nr_regex_t* regex = NULL; - - nr_buffer_add(buf, NR_PSTR("(")); - ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(paths), path) { - if (!nr_php_is_zval_valid_string(path)) { - nrl_verbosedebug(NRL_FRAMEWORK, - "%s: unexpected non-string value in path array", - __func__); - } else { - if (first) { - first = 0; - } else { - nr_buffer_add(buf, NR_PSTR("|")); - } - - nr_regex_add_quoted_to_buffer(buf, Z_STRVAL_P(path), - zval_len_without_trailing_slash(path)); - } - } - ZEND_HASH_FOREACH_END(); - nr_buffer_add(buf, NR_PSTR(")")); - add_wildcard_path_component(buf); - nr_buffer_add(buf, NR_PSTR("\0")); - - /* - * If the first flag is still set, no valid theme paths existed and we - * should abort. - */ - if (first) { - nrl_verbosedebug(NRL_FRAMEWORK, "%s: no valid elements in the path array", - __func__); - } else { - regex = nr_regex_create(nr_buffer_cptr(buf), NR_REGEX_CASELESS, 1); - } - - nr_buffer_destroy(&buf); - return regex; -} - -static nr_regex_t* compile_regex_for_constant(const char* constant, - const char* suffix TSRMLS_DC) { - nr_regex_t* regex = NULL; - zval* value = nr_php_get_constant(constant TSRMLS_CC); +static nr_matcher_t* create_matcher_for_constant(const char* constant, + const char* suffix) { + zval* value = nr_php_get_constant(constant); if (nr_php_is_zval_valid_string(value)) { nrl_verbosedebug(NRL_FRAMEWORK, "Wordpress: found value = %s for constant=%s", Z_STRVAL_P(value), constant); + nr_matcher_t* matcher = nr_matcher_create(); + char* prefix = nr_formatf("%s%s", Z_STRVAL_P(value), suffix); - nrbuf_t* buf = nr_buffer_create(2 * Z_STRLEN_P(value), 0); + nr_matcher_add_prefix(matcher, prefix); - nr_regex_add_quoted_to_buffer(buf, Z_STRVAL_P(value), - zval_len_without_trailing_slash(value)); - nr_buffer_add(buf, suffix, nr_strlen(suffix)); - add_wildcard_path_component(buf); - nr_buffer_add(buf, NR_PSTR("\0")); - - regex = nr_regex_create(nr_buffer_cptr(buf), NR_REGEX_CASELESS, 1); - - nr_buffer_destroy(&buf); + nr_free(prefix); + nr_php_zval_free(&value); + return matcher; } else { /* * If the constant isn't set, that's not a problem, but if it is and it's @@ -128,51 +52,52 @@ static nr_regex_t* compile_regex_for_constant(const char* constant, } nr_php_zval_free(&value); - return regex; + return NULL; } -static char* try_match_regex(const nr_regex_t* regex, const char* filename) { - char* plugin = NULL; - nr_regex_substrings_t* ss = NULL; +/* + * Purpose : Strip the ".php" file extension from a file name + * + * Params : 1. The string filename + * 2. The filename length + * + * Returns : A newly allocated string stripped of the .php extension + * + */ +static inline char* nr_wordpress_strip_php_suffix(char* filename, int filename_len) { + char* retval = NULL; - ss = nr_regex_match_capture(regex, filename, nr_strlen(filename)); - if (NULL == ss) { - return NULL; + if (!nr_striendswith(filename, filename_len, NR_PSTR(".php"))) { + return filename; } - /* - * The last substring will be the plugin or theme name. - */ - plugin = nr_regex_substrings_get(ss, nr_regex_substrings_count(ss)); - nr_regex_substrings_destroy(&ss); - - return plugin; + retval = nr_strndup(filename, filename_len - (sizeof(".php") - 1)); + nr_free(filename); + return retval; } -static const nr_regex_t* nr_wordpress_core_regex(TSRMLS_D) { - nr_regex_t* regex = NULL; +static nr_matcher_t* nr_wordpress_core_matcher() { + nr_matcher_t* matcher = NULL; - if (NRPRG(wordpress_core_regex)) { - return NRPRG(wordpress_core_regex); + if (NRPRG(wordpress_core_matcher)) { + return NRPRG(wordpress_core_matcher); } - /* - * This will get all the Wordpress core functions located in the `wp-includes` - * (or a subdirectory off of that directory) directory. - */ - - regex - = nr_regex_create("wp-includes.*?/([^/]*)[.]php$", NR_REGEX_CASELESS, 1); + matcher = create_matcher_for_constant("WPINC", ""); + if (NULL == matcher) { + matcher = nr_matcher_create(); + nr_matcher_add_prefix(matcher, "/wp-includes"); + } - NRPRG(wordpress_core_regex) = regex; - return regex; + NRPRG(wordpress_core_matcher) = matcher; + return matcher; } -static const nr_regex_t* nr_wordpress_plugin_regex(TSRMLS_D) { - nr_regex_t* regex = NULL; +static nr_matcher_t* nr_wordpress_plugin_matcher() { + nr_matcher_t* matcher = NULL; - if (NRPRG(wordpress_plugin_regex)) { - return NRPRG(wordpress_plugin_regex); + if (NRPRG(wordpress_plugin_matcher)) { + return NRPRG(wordpress_plugin_matcher); } /* @@ -187,35 +112,37 @@ static const nr_regex_t* nr_wordpress_plugin_regex(TSRMLS_D) { * the best. */ - regex = compile_regex_for_constant("WP_PLUGIN_DIR", "" TSRMLS_CC); - if (!regex) { - regex = compile_regex_for_constant("WP_CONTENT_DIR", "/plugins" TSRMLS_CC); + matcher = create_matcher_for_constant("WP_PLUGIN_DIR", ""); + if (NULL == matcher) { + matcher = create_matcher_for_constant("WP_CONTENT_DIR", "/plugins"); } /* * Fallback if the constants didn't exist or were invalid. */ - if (NULL == regex) { + if (NULL == matcher) { nrl_verbosedebug(NRL_FRAMEWORK, "%s: neither WP_PLUGIN_DIR nor WP_CONTENT_DIR set", __func__); - regex = nr_regex_create("plugins/(.*?)/|plugins/([^/]*)[.]php$", - NR_REGEX_CASELESS, 1); + matcher = nr_matcher_create(); + nr_matcher_add_prefix(matcher, "/wp-content/plugins"); } - NRPRG(wordpress_plugin_regex) = regex; - return regex; + NRPRG(wordpress_plugin_matcher) = matcher; + return matcher; } -static const nr_regex_t* nr_wordpress_theme_regex(TSRMLS_D) { - nr_regex_t* regex = NULL; +static nr_matcher_t* nr_wordpress_theme_matcher() { + nr_matcher_t* matcher = NULL; zval* roots = NULL; - if (NRPRG(wordpress_theme_regex)) { - return NRPRG(wordpress_theme_regex); + if (NRPRG(wordpress_theme_matcher)) { + return NRPRG(wordpress_theme_matcher); } + matcher = nr_matcher_create(); + /* * WordPress 2.9.0 and later include get_theme_roots(), which will give us * either a string with the single theme root (the normal case) or an array @@ -223,36 +150,51 @@ static const nr_regex_t* nr_wordpress_theme_regex(TSRMLS_D) { */ roots = nr_php_call(NULL, "get_theme_roots"); if (nr_php_is_zval_valid_string(roots)) { - regex = compile_regex_for_path(roots); + nr_matcher_add_prefix(matcher, Z_STRVAL_P(roots)); } else if (nr_php_is_zval_valid_array(roots) && (nr_php_zend_hash_num_elements(Z_ARRVAL_P(roots)) > 0)) { - regex = compile_regex_for_path_array(roots); + zval* path = NULL; + ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(roots), path) { + if (nr_php_is_zval_valid_string(path)) { + nr_matcher_add_prefix(matcher, Z_STRVAL_P(path)); + } + } + ZEND_HASH_FOREACH_END(); + } else { + nr_matcher_add_prefix(matcher, "/wp-content/themes"); } - nr_php_zval_free(&roots); - /* - * Fallback path if get_theme_roots() failed to give us anything useful. - */ - if (NULL == regex) { - regex = nr_regex_create("/wp-content/themes/(.*?)/", NR_REGEX_CASELESS, 1); - } + nr_php_zval_free(&roots); - NRPRG(wordpress_theme_regex) = regex; - return regex; + NRPRG(wordpress_theme_matcher) = matcher; + return matcher; } -char* nr_php_wordpress_plugin_match_regex(const char* filename TSRMLS_DC) { +char* nr_php_wordpress_plugin_match_matcher(const char* filename) { char* plugin = NULL; - plugin = try_match_regex(nr_wordpress_plugin_regex(TSRMLS_C), filename); - nr_regex_destroy(&NRPRG(wordpress_plugin_regex)); + int plugin_len; + plugin = nr_matcher_match_ex(nr_wordpress_plugin_matcher(), filename, nr_strlen(filename), &plugin_len); + plugin = nr_wordpress_strip_php_suffix(plugin, plugin_len); + nr_matcher_destroy(&NRPRG(wordpress_plugin_matcher)); return plugin; } -char* nr_php_wordpress_core_match_regex(const char* filename TSRMLS_DC) { - char* plugin = NULL; - plugin = try_match_regex(nr_wordpress_core_regex(TSRMLS_C), filename); - nr_regex_destroy(&NRPRG(wordpress_core_regex)); - return plugin; +char* nr_php_wordpress_theme_match_matcher(const char* filename) { + char* theme = NULL; + int plugin_len; + theme = nr_matcher_match_ex(nr_wordpress_theme_matcher(), filename, nr_strlen(filename), &plugin_len); + theme = nr_wordpress_strip_php_suffix(theme, plugin_len); + nr_matcher_destroy(&NRPRG(wordpress_theme_matcher)); + return theme; +} + +char* nr_php_wordpress_core_match_matcher(const char* filename) { + char* core = NULL; + int plugin_len; + core = nr_matcher_match_r_ex(nr_wordpress_core_matcher(), filename, nr_strlen(filename), &plugin_len); + core = nr_wordpress_strip_php_suffix(core, plugin_len); + nr_matcher_destroy(&NRPRG(wordpress_core_matcher)); + return core; } static void nr_wordpress_create_metric(nr_segment_t* segment, @@ -277,6 +219,7 @@ static char* nr_wordpress_plugin_from_function(zend_function* func TSRMLS_DC) { const char* filename = NULL; size_t filename_len; char* plugin = NULL; + int plugin_len; if (NULL == func) { return NULL; @@ -290,7 +233,7 @@ static char* nr_wordpress_plugin_from_function(zend_function* func TSRMLS_DC) { NRP_PHP(NRPRG(wordpress_tag))); return NULL; } - filename_len = nr_strlen(filename); + filename_len = nr_php_function_filename_len(func); if (NRPRG(wordpress_file_metadata)) { if (nr_hashmap_get_into(NRPRG(wordpress_file_metadata), filename, @@ -298,7 +241,7 @@ static char* nr_wordpress_plugin_from_function(zend_function* func TSRMLS_DC) { nrl_verbosedebug(NRL_FRAMEWORK, "Wordpress: found in cache: " "plugin= %s and filename=" NRP_FMT, - plugin, NRP_FILENAME(filename)); + NRSAFESTR(plugin), NRP_FILENAME(filename)); return plugin; } } else { @@ -308,17 +251,20 @@ static char* nr_wordpress_plugin_from_function(zend_function* func TSRMLS_DC) { "Wordpress: NOT found in cache: " "filename=" NRP_FMT, NRP_FILENAME(filename)); - plugin = try_match_regex(nr_wordpress_plugin_regex(TSRMLS_C), filename); + plugin = nr_matcher_match_ex(nr_wordpress_plugin_matcher(), filename, filename_len, &plugin_len); + plugin = nr_wordpress_strip_php_suffix(plugin, plugin_len); if (plugin) { goto cache_and_return; } - plugin = try_match_regex(nr_wordpress_theme_regex(TSRMLS_C), filename); + plugin = nr_matcher_match_ex(nr_wordpress_theme_matcher(), filename, filename_len, &plugin_len); + plugin = nr_wordpress_strip_php_suffix(plugin, plugin_len); if (plugin) { goto cache_and_return; } - plugin = try_match_regex(nr_wordpress_core_regex(TSRMLS_C), filename); + plugin = nr_matcher_match_r_ex(nr_wordpress_core_matcher(), filename, filename_len, &plugin_len); + plugin = nr_wordpress_strip_php_suffix(plugin, plugin_len); if (plugin) { /* * The core wordpress functions are anonymized, so we don't need to return @@ -342,7 +288,7 @@ static char* nr_wordpress_plugin_from_function(zend_function* func TSRMLS_DC) { cache_and_return: /* * Even if plugin is NULL, we'll still cache that. Hooks in WordPress's core - * will be NULL, and we need not re-run the regexes each time. + * will be NULL, and we need not re-run the matchers each time. */ nr_hashmap_set(NRPRG(wordpress_file_metadata), filename, filename_len, plugin); diff --git a/agent/fw_wordpress.h b/agent/fw_wordpress.h index e8023b420..d6b121cc3 100644 --- a/agent/fw_wordpress.h +++ b/agent/fw_wordpress.h @@ -11,23 +11,32 @@ #define FW_WORDPRESS_HDR /* - * Purpose : ONLY for testing to verify that the appropriate regex was created + * Purpose : ONLY for testing to verify that the appropriate matcher was created * for determining if a filename belongs to the WP core (located - * off of the `wp-includes` directory). It destroys the regex at + * off of the `wp-includes` directory). It destroys the matcher at * the end so again, this is only for testing purposes. * * Returns : The matching core name; otherwise NULL. */ -char* nr_php_wordpress_core_match_regex(const char* filename TSRMLS_DC); +char* nr_php_wordpress_core_match_matcher(const char* filename); /* - * Purpose : ONLY for testing to verify that the appropriate regex was created + * Purpose : ONLY for testing to verify that the appropriate matcher was created * for determining if a filename belongs to a plugin. It destroys - * the regex at the end so again, this is only for testing purposes. + * the matcher at the end so again, this is only for testing purposes. * * Returns : The matching plugin; otherwise NULL */ -char* nr_php_wordpress_plugin_match_regex(const char* filename TSRMLS_DC); +char* nr_php_wordpress_plugin_match_matcher(const char* filename); + +/* + * Purpose : ONLY for testing to verify that the appropriate matcher was created + * for determining if a filename belongs to a theme. It destroys + * the matcher at the end so again, this is only for testing purposes. + * + * Returns : The matching theme; otherwise NULL + */ +char* nr_php_wordpress_theme_match_matcher(const char* filename); extern void nr_wordpress_minit(void); extern void nr_wordpress_mshutdown(void); diff --git a/agent/php_agent.h b/agent/php_agent.h index 7877325d3..1c024dc54 100644 --- a/agent/php_agent.h +++ b/agent/php_agent.h @@ -702,6 +702,11 @@ nr_php_op_array_scope_name(const zend_op_array* op_array) { return NULL; } +static inline nr_string_len_t NRPURE +nr_php_function_filename_len(zend_function* func) { + return func ? nr_php_op_array_file_name_len(&func->op_array) : 0; +} + static inline const char* NRPURE nr_php_ini_entry_name(const zend_ini_entry* entry) { #if ZEND_MODULE_API_NO >= ZEND_7_0_X_API_NO /* PHP 7.0+ */ diff --git a/agent/php_newrelic.h b/agent/php_newrelic.h index 85887fe29..82d746b78 100644 --- a/agent/php_newrelic.h +++ b/agent/php_newrelic.h @@ -15,6 +15,7 @@ #include "nr_txn.h" #include "php_extension.h" #include "util_hashmap.h" +#include "util_matcher.h" #include "util_vector.h" #define PHP_NEWRELIC_EXT_NAME "newrelic" @@ -381,13 +382,13 @@ int symfony1_in_error404; /* Whether we are currently within a bool check_cufa; /* Whether we need to check cufa because we are instrumenting hooks, or whether we can skip cufa */ -char* wordpress_tag; /* The current WordPress tag */ -nr_regex_t* wordpress_plugin_regex; /* Regex for plugin filenames */ -nr_regex_t* wordpress_theme_regex; /* Regex for theme filenames */ -nr_regex_t* wordpress_core_regex; /* Regex for plugin filenames */ -nr_hashmap_t* wordpress_file_metadata; /* Metadata for plugin and theme names - given a filename */ -nr_hashmap_t* wordpress_clean_tag_cache; /* Cached clean tags */ +char* wordpress_tag; /* The current WordPress tag */ +nr_matcher_t* wordpress_plugin_matcher; /* Matcher for plugin filenames */ +nr_matcher_t* wordpress_theme_matcher; /* Matcher for theme filenames */ +nr_matcher_t* wordpress_core_matcher; /* Matcher for plugin filenames */ +nr_hashmap_t* wordpress_file_metadata; /* Metadata for plugin and theme names + given a filename */ +nr_hashmap_t* wordpress_clean_tag_cache; /* Cached clean tags */ char* doctrine_dql; /* The current Doctrine DQL. Only non-NULL while a Doctrine object is on the stack. */ diff --git a/agent/php_rshutdown.c b/agent/php_rshutdown.c index b48a3ffd7..1e68f4648 100644 --- a/agent/php_rshutdown.c +++ b/agent/php_rshutdown.c @@ -96,9 +96,10 @@ int nr_php_post_deactivate(void) { nr_php_remove_transient_user_instrumentation(); nr_php_exception_filters_destroy(&NRPRG(exception_filters)); - nr_regex_destroy(&NRPRG(wordpress_plugin_regex)); - nr_regex_destroy(&NRPRG(wordpress_core_regex)); - nr_regex_destroy(&NRPRG(wordpress_theme_regex)); + + nr_matcher_destroy(&NRPRG(wordpress_plugin_matcher)); + nr_matcher_destroy(&NRPRG(wordpress_core_matcher)); + nr_matcher_destroy(&NRPRG(wordpress_theme_matcher)); nr_hashmap_destroy(&NRPRG(wordpress_file_metadata)); nr_hashmap_destroy(&NRPRG(wordpress_clean_tag_cache)); diff --git a/agent/tests/test_fw_wordpress.c b/agent/tests/test_fw_wordpress.c index 7ba170e27..03b3fce85 100644 --- a/agent/tests/test_fw_wordpress.c +++ b/agent/tests/test_fw_wordpress.c @@ -25,55 +25,55 @@ tlib_parallel_info_t parallel_info = {.suggested_nthreads = -1, .state_size = 0}; /* - * This will test whether the regular expression checking works to determine + * This will test whether the matcher checking works to determine * the name of a plugin from a filename when the "plugin" is a .php file. */ -static void test_wordpress_core_regex(TSRMLS_D) { +static void test_wordpress_core_matcher() { char* plugin = NULL; /* Test with invalid input. */ - plugin = nr_php_wordpress_core_match_regex(NULL TSRMLS_CC); + plugin = nr_php_wordpress_core_match_matcher(NULL); tlib_pass_if_null( - "Wordpress function regex matching should return NULL when given NULL.", + "Wordpress function matcher matching should return NULL when given NULL.", plugin); - plugin = nr_php_wordpress_core_match_regex( - "/wp-content/plugins/affiliatelite.php" TSRMLS_CC); + plugin = nr_php_wordpress_core_match_matcher( + "/wp-content/plugins/affiliatelite.php"); tlib_pass_if_null( - "wordpress core regex matching should not work from the regular plugins " + "wordpress core matcher matching should not work from the regular plugins " "directory", plugin); - plugin = nr_php_wordpress_core_match_regex( - "/www-data/premium.wpmudev.org/wp-content/affiliatelite.php" TSRMLS_CC); + plugin = nr_php_wordpress_core_match_matcher( + "/www-data/premium.wpmudev.org/wp-content/affiliatelite.php"); tlib_pass_if_null( - "wordpress core regex matching should not work from a non-standard " + "wordpress core matcher matching should not work from a non-standard " "directory.", plugin); /* Test with valid input. */ - plugin = nr_php_wordpress_core_match_regex( - "/wordpress/wordpress/wp-includes/query.php" TSRMLS_CC); + plugin = nr_php_wordpress_core_match_matcher( + "/wordpress/wordpress/wp-includes/query.php"); tlib_pass_if_not_null( - "wordpress core regex matching should work from a standard " + "wordpress core matcher matching should work from a standard " "directory.", plugin); tlib_pass_if_str_equal( - "wordpress core regex matching should work from a standard " + "wordpress core matcher matching should work from a standard " "directory.", "query", plugin); nr_free(plugin); - plugin = nr_php_wordpress_core_match_regex( - "/wordpress/wordpress/wp-includes/block/query.php" TSRMLS_CC); + plugin = nr_php_wordpress_core_match_matcher( + "/wordpress/wordpress/wp-includes/block/query.php"); tlib_pass_if_not_null( - "wordpress core regex matching should work from a standard " + "wordpress core matcher matching should work from a standard " "directory with a subdirectory", plugin); tlib_pass_if_str_equal( - "wordpress core regex matching should work from a standard " + "wordpress core matcher matching should work from a standard " "directory with a subdirectory.", "query", plugin); @@ -81,44 +81,44 @@ static void test_wordpress_core_regex(TSRMLS_D) { } /* - * This will test whether the regular expression checking works to determine + * This will test whether the matcher checking works to determine * the name of a plugin from a filename when the plugin is not a .php file */ -static void test_wordpress_plugin_regex(TSRMLS_D) { +static void test_wordpress_plugin_matcher() { char* plugin = NULL; char* filename = "/www-data/premium.wpmudev.org/wp-content/plugins/plugin/" "affiliatelite.php"; /* Test with invalid input. */ - plugin = nr_php_wordpress_plugin_match_regex(NULL TSRMLS_CC); + plugin = nr_php_wordpress_plugin_match_matcher(NULL); tlib_pass_if_null( - "Wordpress plugin regex should return NULL when given NULL.", plugin); + "Wordpress plugin matcher should return NULL when given NULL.", plugin); - plugin = nr_php_wordpress_plugin_match_regex( - "/wp-content/affiliatelite.php" TSRMLS_CC); + plugin = nr_php_wordpress_plugin_match_matcher( + "/wp-content/affiliatelite.php"); tlib_pass_if_null( - "Wordpress plugin regex should return NULL if the filename is not in the " + "Wordpress plugin matcher should return NULL if the filename is not in the " "correct plugin directory.", plugin); /* Test with valid input. */ - plugin = nr_php_wordpress_plugin_match_regex( - "/wp-content/plugins/affiliatelite.php" TSRMLS_CC); + plugin = nr_php_wordpress_plugin_match_matcher( + "/wp-content/plugins/affiliatelite.php"); tlib_pass_if_not_null( - "Wordpress plugin regex should return plugin name even if the plugin is " + "Wordpress plugin matcher should return plugin name even if the plugin is " "a function " "not a directory.", plugin); - tlib_pass_if_str_equal("Wordpress plugin regex should work.", "affiliatelite", + tlib_pass_if_str_equal("Wordpress plugin matcher should work.", "affiliatelite", plugin); nr_free(plugin); - plugin = nr_php_wordpress_plugin_match_regex(filename TSRMLS_CC); - tlib_pass_if_not_null("Wordpress plugin regex should work.", plugin); - tlib_pass_if_str_equal("Wordpress plugin regex should work.", "plugin", + plugin = nr_php_wordpress_plugin_match_matcher(filename); + tlib_pass_if_not_null("Wordpress plugin matcher should work.", plugin); + tlib_pass_if_str_equal("Wordpress plugin matcher should work.", "plugin", plugin); nr_free(plugin); } @@ -128,7 +128,7 @@ void test_main(void* p NRUNUSED) { void*** tsrm_ls = NULL; #endif /* ZTS && !PHP7 */ tlib_php_engine_create("" PTSRMLS_CC); - test_wordpress_plugin_regex(TSRMLS_C); - test_wordpress_core_regex(TSRMLS_C); + test_wordpress_plugin_matcher(); + test_wordpress_core_matcher(); tlib_php_engine_destroy(TSRMLS_C); } diff --git a/axiom/Makefile b/axiom/Makefile index 8dc69c1dc..c84117417 100644 --- a/axiom/Makefile +++ b/axiom/Makefile @@ -135,6 +135,7 @@ OBJS := \ util_json.o \ util_logging.o \ util_labels.o \ + util_matcher.o \ util_md5.o \ util_memory.o \ util_metrics.o \ diff --git a/axiom/tests/.gitignore b/axiom/tests/.gitignore index 470f8d116..e2e2db564 100644 --- a/axiom/tests/.gitignore +++ b/axiom/tests/.gitignore @@ -71,6 +71,7 @@ test_log_events test_log_level test_logging test_logging_parallel +test_matcher test_math test_memory test_metrics diff --git a/axiom/tests/Makefile b/axiom/tests/Makefile index 3dc109869..0a37821bb 100644 --- a/axiom/tests/Makefile +++ b/axiom/tests/Makefile @@ -152,6 +152,7 @@ TESTS := \ test_log_events \ test_log_level \ test_logging \ + test_matcher \ test_math \ test_memory \ test_metrics \ diff --git a/axiom/tests/test_matcher.c b/axiom/tests/test_matcher.c new file mode 100644 index 000000000..9bc198b55 --- /dev/null +++ b/axiom/tests/test_matcher.c @@ -0,0 +1,111 @@ +/* + * Copyright 2020 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +#include "nr_axiom.h" + +#include "util_matcher.h" + +#include "tlib_main.h" + +static void test_match_multiple(void) { + char* found; + nr_matcher_t* matcher; + + matcher = nr_matcher_create(); + tlib_pass_if_bool_equal("add prefix", true, + nr_matcher_add_prefix(matcher, "/foo")); + tlib_pass_if_bool_equal("add prefix", true, + nr_matcher_add_prefix(matcher, "/bar//")); + + tlib_pass_if_null("needle not matched", nr_matcher_match(matcher, "")); + tlib_pass_if_null("needle not matched", nr_matcher_match(matcher, "foo")); + tlib_pass_if_null("needle not matched", nr_matcher_match(matcher, "/bar")); + + found = nr_matcher_match(matcher, "/foo/baz/quux"); + tlib_pass_if_str_equal("needle match", "baz", found); + nr_free(found); + + found = nr_matcher_match(matcher, "/foo/baz//quux"); + tlib_pass_if_str_equal("needle match", "baz", found); + nr_free(found); + + found = nr_matcher_match(matcher, "/bar/xxx"); + tlib_pass_if_str_equal("needle match", "xxx", found); + nr_free(found); + + nr_matcher_destroy(&matcher); +} + +static void test_match_single(void) { + char* found; + nr_matcher_t* matcher = nr_matcher_create(); + + nr_matcher_add_prefix(matcher, "/foo/bar"); + + tlib_pass_if_null("needle not matched", nr_matcher_match(matcher, "")); + tlib_pass_if_null("needle not matched", nr_matcher_match(matcher, "foo")); + tlib_pass_if_null("needle not matched", nr_matcher_match(matcher, "/bar")); + + found = nr_matcher_match(matcher, "/foo/bar/quux"); + tlib_pass_if_str_equal("needle match", "quux", found); + nr_free(found); + + found = nr_matcher_match(matcher, "/foo/bar//quux"); + tlib_pass_if_str_equal("needle match", "", found); + nr_free(found); + + found = nr_matcher_match(matcher, "/foo/bar/quux/baz"); + tlib_pass_if_str_equal("needle match", "quux", found); + nr_free(found); + + nr_matcher_destroy(&matcher); +} + +static void test_match_ex(void) { + char* found; + int len; + nr_matcher_t* matcher = nr_matcher_create(); + + nr_matcher_add_prefix(matcher, "/foo/bar"); + found = nr_matcher_match_ex(matcher, NR_PSTR(""), &len); + tlib_pass_if_null("needle not matched", found); + tlib_pass_if_equal("needle match len", 0, len, int, "%d") + nr_free(found); + + found = nr_matcher_match_ex(matcher, NR_PSTR("foo"), &len); + tlib_pass_if_null("needle not matched", found); + tlib_pass_if_equal("needle match len", 0, len, int, "%d") + nr_free(found); + + found = nr_matcher_match_ex(matcher, NR_PSTR("/bar"), &len); + tlib_pass_if_null("needle not matched", found); + tlib_pass_if_equal("needle match len", 0, len, int, "%d") + nr_free(found); + + found = nr_matcher_match_ex(matcher, NR_PSTR("/foo/bar/quux"), &len); + tlib_pass_if_str_equal("needle match", "quux", found); + tlib_pass_if_equal("needle match len", 4, len, int, "%d") + nr_free(found); + + found = nr_matcher_match_ex(matcher, NR_PSTR("/foo/bar//quux"), &len); + tlib_pass_if_str_equal("needle match", "", found); + tlib_pass_if_equal("needle match len", 0, len, int, "%d") + nr_free(found); + + found = nr_matcher_match_ex(matcher, NR_PSTR("/foo/bar/quux/baz"), &len); + tlib_pass_if_str_equal("needle match", "quux", found); + tlib_pass_if_equal("needle match len", 4, len, int, "%d") + nr_free(found); + + nr_matcher_destroy(&matcher); +} + +tlib_parallel_info_t parallel_info = {.suggested_nthreads = 2, .state_size = 0}; + +void test_main(void* p NRUNUSED) { + test_match_multiple(); + test_match_single(); + test_match_ex(); +} diff --git a/axiom/util_matcher.c b/axiom/util_matcher.c new file mode 100644 index 000000000..4f1ad36ec --- /dev/null +++ b/axiom/util_matcher.c @@ -0,0 +1,152 @@ +/* + * Copyright 2020 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +#include "util_matcher.h" +#include "util_matcher_private.h" + +#include "util_memory.h" +#include "util_strings.h" + +typedef struct { + char* cp; + int len; +} matcher_prefix; + +static void nr_matcher_prefix_dtor(void* _p, void* userdata NRUNUSED) { + matcher_prefix* p = (matcher_prefix*)_p; + nr_free(p->cp); + nr_free(p); +} + +nr_matcher_t* nr_matcher_create(void) { + nr_matcher_t* matcher; + + matcher = nr_malloc(sizeof(nr_matcher_t)); + nr_vector_init(&matcher->prefixes, 0, nr_matcher_prefix_dtor, NULL); + + return matcher; +} + +void nr_matcher_destroy(nr_matcher_t** matcher_ptr) { + if (NULL == matcher_ptr || NULL == *matcher_ptr) { + return; + } + + nr_vector_deinit(&((*matcher_ptr)->prefixes)); + nr_realfree((void**)matcher_ptr); +} + +bool nr_matcher_add_prefix(nr_matcher_t* matcher, const char* str) { + int i; + matcher_prefix* prefix; + + if (NULL == matcher || NULL == str) { + return false; + } + + if (NULL == (prefix = nr_calloc(1, sizeof(matcher_prefix)))) { + return false; + } + prefix->len = nr_strlen(str); + while (prefix->len > 0 && '/' == str[prefix->len - 1]) { + prefix->len--; + } + + prefix->len += 1; // +1 for the trailing '/' + if (NULL == (prefix->cp = nr_malloc(prefix->len + 1))) { // +1 for the '\0' + nr_matcher_prefix_dtor(prefix, NULL); + return false; + } + for (i = 0; i < prefix->len; i++) { + prefix->cp[i] = nr_tolower(str[i]); + } + prefix->cp[prefix->len - 1] = '/'; + prefix->cp[prefix->len] = '\0'; + + return nr_vector_push_back(&matcher->prefixes, prefix); +} + +#define SET_SAFE(p, v) \ + do { \ + if (NULL != p) \ + *p = (v); \ + } while (0); +static char* nr_matcher_match_internal(nr_matcher_t* matcher, + const char* input, + int input_len, + int* match_len, + bool core) { + size_t i; + char* input_lc; + char* match = NULL; + size_t num_prefixes; + + SET_SAFE(match_len, 0); + + if (NULL == matcher || NULL == input) { + return NULL; + } + + num_prefixes = nr_vector_size(&matcher->prefixes); + input_lc = nr_string_to_lowercase(input); + + for (i = 0; i < num_prefixes; i++) { + const char* found; + const matcher_prefix* prefix = nr_vector_get(&matcher->prefixes, i); + + found = nr_strstr(input, prefix->cp); + if (found) { + const char* slash; + + found += prefix->len; + if (true == core) { + slash = nr_strrchr(found, '/'); + } else { + slash = nr_strchr(found, '/'); + } + if (NULL == slash) { + match = nr_strdup(found); + SET_SAFE(match_len, input_len - (found - input)); + } else { + if (true == core) { + const char* offset = input + input_len; + match = nr_strndup(slash + 1, offset - slash); + SET_SAFE(match_len, offset - (slash + 1)); + } else { + match = nr_strndup(found, slash - found); + SET_SAFE(match_len, slash - found); + } + } + break; + } + } + + nr_free(input_lc); + return match; +} + +char* nr_matcher_match_ex(nr_matcher_t* matcher, + const char* input, + int input_len, + int* match_len) { + return nr_matcher_match_internal(matcher, input, input_len, match_len, false); +} + +char* nr_matcher_match(nr_matcher_t* matcher, const char* input) { + return nr_matcher_match_internal(matcher, input, nr_strlen(input), NULL, + false); +} + +char* nr_matcher_match_r_ex(nr_matcher_t* matcher, + const char* input, + int input_len, + int* match_len) { + return nr_matcher_match_internal(matcher, input, input_len, match_len, true); +} + +char* nr_matcher_match_r(nr_matcher_t* matcher, const char* input) { + return nr_matcher_match_internal(matcher, input, nr_strlen(input), NULL, + true); +} diff --git a/axiom/util_matcher.h b/axiom/util_matcher.h new file mode 100644 index 000000000..2e4b12a53 --- /dev/null +++ b/axiom/util_matcher.h @@ -0,0 +1,39 @@ +/* + * Copyright 2020 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +#ifndef UTIL_MATCHER_HDR +#define UTIL_MATCHER_HDR + +#include + +typedef struct _nr_matcher_t nr_matcher_t; + +extern nr_matcher_t* nr_matcher_create(void); + +extern void nr_matcher_destroy(nr_matcher_t** matcher_ptr); + +extern bool nr_matcher_add_prefix(nr_matcher_t* matcher, const char* prefix); + +/* + * Return a substring from an input string using a matcher definition from the + * left side of the string + */ +extern char* nr_matcher_match_ex(nr_matcher_t* matcher, + const char* input, + int input_len, + int* match_len); +extern char* nr_matcher_match(nr_matcher_t* matcher, const char* input); + +/* + * Return a substring from an input string using a matcher definition from the + * right side of the string + */ +extern char* nr_matcher_match_r_ex(nr_matcher_t* matcher, + const char* input, + int input_len, + int* match_len); +extern char* nr_matcher_match_r(nr_matcher_t* matcher, const char* input); + +#endif diff --git a/axiom/util_matcher_private.h b/axiom/util_matcher_private.h new file mode 100644 index 000000000..e6c7f3e79 --- /dev/null +++ b/axiom/util_matcher_private.h @@ -0,0 +1,15 @@ +/* + * Copyright 2020 New Relic Corporation. All rights reserved. + * SPDX-License-Identifier: Apache-2.0 + */ + +#ifndef UTIL_MATCHER_PRIVATE_HDR +#define UTIL_MATCHER_PRIVATE_HDR + +#include "util_vector.h" + +typedef struct _nr_matcher_t { + nr_vector_t prefixes; +} nr_matcher_t; + +#endif /* UTIL_MATCHER_PRIVATE_HDR */ From 9b9cd6452df903533af9bd3449f50cfd461779b3 Mon Sep 17 00:00:00 2001 From: Michal Nowacki Date: Wed, 6 Dec 2023 11:53:43 -0500 Subject: [PATCH 14/17] style: fix indentation (#796) --- agent/fw_wordpress.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/agent/fw_wordpress.c b/agent/fw_wordpress.c index 793245c54..3a84450cf 100644 --- a/agent/fw_wordpress.c +++ b/agent/fw_wordpress.c @@ -440,18 +440,18 @@ NR_PHP_WRAPPER(nr_wordpress_exec_handle_tag) { */ char* old_tag = NRPRG(wordpress_tag); - NRPRG(check_cufa) = true; + NRPRG(check_cufa) = true; - NRPRG(wordpress_tag) = nr_wordpress_clean_tag(tag); - NR_PHP_WRAPPER_CALL; - NRPRG(wordpress_tag) = old_tag; - if (NULL == NRPRG(wordpress_tag)) { + NRPRG(wordpress_tag) = nr_wordpress_clean_tag(tag); + NR_PHP_WRAPPER_CALL; + NRPRG(wordpress_tag) = old_tag; + if (NULL == NRPRG(wordpress_tag)) { + NRPRG(check_cufa) = false; + } + } else { NRPRG(check_cufa) = false; + NR_PHP_WRAPPER_CALL; } - } else { - NRPRG(check_cufa) = false; - NR_PHP_WRAPPER_CALL; - } nr_php_arg_release(&tag); } From c8fe3a8cdcb374f80fca7e88d095032d858ab5a1 Mon Sep 17 00:00:00 2001 From: bduranleau-nr <106178551+bduranleau-nr@users.noreply.github.com> Date: Mon, 11 Dec 2023 16:15:19 -0600 Subject: [PATCH 15/17] tests: fix deprecated redis function tests (#800) `lGet` and `lRemove` are deprecated functions that no longer run on later versions of the Redis extension. Pin the tests to the last known working version and gate via SKIPIF. --- .../integration/redis/test_instance_list.php | 8 +-- tests/integration/redis/test_lget.php | 67 +++++++++++++++++++ tests/integration/redis/test_list.php | 23 ++----- .../redis/test_list_logging_off.php | 23 ++----- tests/integration/redis/test_lremove.php | 64 ++++++++++++++++++ 5 files changed, 145 insertions(+), 40 deletions(-) create mode 100644 tests/integration/redis/test_lget.php create mode 100644 tests/integration/redis/test_lremove.php diff --git a/tests/integration/redis/test_instance_list.php b/tests/integration/redis/test_instance_list.php index e925de3e9..d9e7deca6 100644 --- a/tests/integration/redis/test_instance_list.php +++ b/tests/integration/redis/test_instance_list.php @@ -89,8 +89,7 @@ function test_redis() { tap_equal(2, $redis->rPush($key, 'C'), 'append C'); tap_equal(3, $redis->lPush($key, 'A'), 'prepend A'); - /* Redis->lGet is deprecated, but use it once to verify it works */ - tap_equal('A', @$redis->lGet($key, 0), 'retrieve element 0'); + tap_equal('A', $redis->lIndex($key, 0), 'retrieve element 0'); tap_equal('B', $redis->lIndex($key, 1), 'retrieve element 1'); tap_equal('C', $redis->lIndex($key, 2), 'retrieve element 2'); tap_equal('C', $redis->lIndex($key, -1), 'retrieve last element'); @@ -119,8 +118,7 @@ function test_redis() { tap_equal(4, $redis->rpush($key, 'A', 'B', 'B', 'C'), 'add new elements'); - /* Redis->lRemove is depreacted, but use it once to verity it works */ - tap_equal(1, @$redis->lRemove($key, 'B', 1), 'remove first occurence of B'); + tap_equal(1, @$redis->lRem($key, 'B', 1), 'remove first occurence of B'); tap_equal(['A', 'B', 'C'], $redis->lrange($key, 0, -1), 'verify list elements'); tap_equal(0, $redis->lRem($key, 'NOT_IN_LIST', 1), 'remove missing element'); @@ -146,7 +144,6 @@ function test_redis() { 'Datastore/operation/Redis/del', 'Datastore/operation/Redis/exists', 'Datastore/operation/Redis/expire', - 'Datastore/operation/Redis/lget', 'Datastore/operation/Redis/lindex', 'Datastore/operation/Redis/linsert', 'Datastore/operation/Redis/llen', @@ -155,7 +152,6 @@ function test_redis() { 'Datastore/operation/Redis/lpushx', 'Datastore/operation/Redis/lrange', 'Datastore/operation/Redis/lrem', - 'Datastore/operation/Redis/lremove', 'Datastore/operation/Redis/lset', 'Datastore/operation/Redis/ltrim', 'Datastore/operation/Redis/rpop', diff --git a/tests/integration/redis/test_lget.php b/tests/integration/redis/test_lget.php new file mode 100644 index 000000000..a0c9ed88d --- /dev/null +++ b/tests/integration/redis/test_lget.php @@ -0,0 +1,67 @@ +')) { + die("skip: Redis <= 5.3.7 required\n"); +} + +require("skipif.inc"); +*/ + +/*INI +newrelic.datastore_tracer.database_name_reporting.enabled = 1 +newrelic.datastore_tracer.instance_reporting.enabled = 1 +*/ + +/*EXPECT +ok - append B +ok - append C +ok - prepend A +ok - retrieve element 0 +*/ + +/*EXPECT_METRICS_EXIST +Datastore/operation/Redis/lget +*/ + +require_once(realpath (dirname ( __FILE__ )) . '/../../include/helpers.php'); +require_once(realpath (dirname ( __FILE__ )) . '/../../include/integration.php'); +require_once(realpath (dirname ( __FILE__ )) . '/../../include/tap.php'); +require_once(realpath (dirname ( __FILE__ )) . '/redis.inc'); + +function test_redis() { + global $REDIS_HOST, $REDIS_PORT; + + $redis = new Redis(); + $redis->connect($REDIS_HOST, $REDIS_PORT); + + /* generate unique key names to use for this test run */ + $key = randstr(16); + $key2 = "{$key}_b"; + if ($redis->exists([$key, $key2])) { + die("skip: key(s) already exist: $key, $key2\n"); + } + + /* Ensure the keys don't persist (too much) longer than the test. */ + $redis->expire($key, 30 /* seconds */); + $redis->expire($key2, 30 /* seconds */); + + tap_equal(1, $redis->rPush($key, 'B'), 'append B'); + tap_equal(2, $redis->rPush($key, 'C'), 'append C'); + tap_equal(3, $redis->lPush($key, 'A'), 'prepend A'); + + /* Redis->lGet is deprecated, but use it once to verify it works */ + tap_equal('A', @$redis->lGet($key, 0), 'retrieve element 0'); +} + +test_redis(); diff --git a/tests/integration/redis/test_list.php b/tests/integration/redis/test_list.php index 8ed77cc67..185e7cea0 100644 --- a/tests/integration/redis/test_list.php +++ b/tests/integration/redis/test_list.php @@ -82,12 +82,9 @@ [{"name":"Datastore/operation/Redis/expire"}, [2, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/expire", "scope":"OtherTransaction/php__FILE__"}, [2, "??", "??", "??", "??", "??"]], - [{"name":"Datastore/operation/Redis/lget"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Datastore/operation/Redis/lget", - "scope":"OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Datastore/operation/Redis/lindex"}, [8, "??", "??", "??", "??", "??"]], + [{"name":"Datastore/operation/Redis/lindex"}, [9, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/lindex", - "scope":"OtherTransaction/php__FILE__"}, [8, "??", "??", "??", "??", "??"]], + "scope":"OtherTransaction/php__FILE__"}, [9, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/linsert"}, [2, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/linsert", "scope":"OtherTransaction/php__FILE__"}, [2, "??", "??", "??", "??", "??"]], @@ -106,12 +103,9 @@ [{"name":"Datastore/operation/Redis/lrange"}, [4, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/lrange", "scope":"OtherTransaction/php__FILE__"}, [4, "??", "??", "??", "??", "??"]], - [{"name":"Datastore/operation/Redis/lrem"}, [1, "??", "??", "??", "??", "??"]], + [{"name":"Datastore/operation/Redis/lrem"}, [2, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/lrem", - "scope":"OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Datastore/operation/Redis/lremove"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Datastore/operation/Redis/lremove", - "scope":"OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], + "scope":"OtherTransaction/php__FILE__"}, [2, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/lset"}, [1, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/lset", "scope":"OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], @@ -141,9 +135,6 @@ ] */ - - - require_once(realpath (dirname ( __FILE__ )) . '/../../include/helpers.php'); require_once(realpath (dirname ( __FILE__ )) . '/../../include/tap.php'); require_once(realpath (dirname ( __FILE__ )) . '/redis.inc'); @@ -169,8 +160,7 @@ function test_redis() { tap_equal(2, $redis->rPush($key, 'C'), 'append C'); tap_equal(3, $redis->lPush($key, 'A'), 'prepend A'); - /* Redis->lGet is deprecated, but use it once to verify it works */ - tap_equal('A', @$redis->lGet($key, 0), 'retrieve element 0'); + tap_equal('A', $redis->lIndex($key, 0), 'retrieve element 0'); tap_equal('B', $redis->lIndex($key, 1), 'retrieve element 1'); tap_equal('C', $redis->lIndex($key, 2), 'retrieve element 2'); tap_equal('C', $redis->lIndex($key, -1), 'retrieve last element'); @@ -199,8 +189,7 @@ function test_redis() { tap_equal(4, $redis->rpush($key, 'A', 'B', 'B', 'C'), 'add new elements'); - /* Redis->lRemove is depreacted, but use it once to verity it works */ - tap_equal(1, @$redis->lRemove($key, 'B', 1), 'remove first occurence of B'); + tap_equal(1, @$redis->lRem($key, 'B', 1), 'remove first occurence of B'); tap_equal(['A', 'B', 'C'], $redis->lrange($key, 0, -1), 'verify list elements'); tap_equal(0, $redis->lRem($key, 'NOT_IN_LIST', 1), 'remove missing element'); diff --git a/tests/integration/redis/test_list_logging_off.php b/tests/integration/redis/test_list_logging_off.php index f693065da..e1a9aeb35 100644 --- a/tests/integration/redis/test_list_logging_off.php +++ b/tests/integration/redis/test_list_logging_off.php @@ -85,12 +85,9 @@ [{"name":"Datastore/operation/Redis/expire"}, [2, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/expire", "scope":"OtherTransaction/php__FILE__"}, [2, "??", "??", "??", "??", "??"]], - [{"name":"Datastore/operation/Redis/lget"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Datastore/operation/Redis/lget", - "scope":"OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Datastore/operation/Redis/lindex"}, [8, "??", "??", "??", "??", "??"]], + [{"name":"Datastore/operation/Redis/lindex"}, [9, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/lindex", - "scope":"OtherTransaction/php__FILE__"}, [8, "??", "??", "??", "??", "??"]], + "scope":"OtherTransaction/php__FILE__"}, [9, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/linsert"}, [2, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/linsert", "scope":"OtherTransaction/php__FILE__"}, [2, "??", "??", "??", "??", "??"]], @@ -109,12 +106,9 @@ [{"name":"Datastore/operation/Redis/lrange"}, [4, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/lrange", "scope":"OtherTransaction/php__FILE__"}, [4, "??", "??", "??", "??", "??"]], - [{"name":"Datastore/operation/Redis/lrem"}, [1, "??", "??", "??", "??", "??"]], + [{"name":"Datastore/operation/Redis/lrem"}, [2, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/lrem", - "scope":"OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Datastore/operation/Redis/lremove"}, [1, "??", "??", "??", "??", "??"]], - [{"name":"Datastore/operation/Redis/lremove", - "scope":"OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], + "scope":"OtherTransaction/php__FILE__"}, [2, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/lset"}, [1, "??", "??", "??", "??", "??"]], [{"name":"Datastore/operation/Redis/lset", "scope":"OtherTransaction/php__FILE__"}, [1, "??", "??", "??", "??", "??"]], @@ -144,9 +138,6 @@ ] */ - - - require_once(realpath (dirname ( __FILE__ )) . '/../../include/helpers.php'); require_once(realpath (dirname ( __FILE__ )) . '/../../include/tap.php'); require_once(realpath (dirname ( __FILE__ )) . '/redis.inc'); @@ -172,8 +163,7 @@ function test_redis() { tap_equal(2, $redis->rPush($key, 'C'), 'append C'); tap_equal(3, $redis->lPush($key, 'A'), 'prepend A'); - /* Redis->lGet is deprecated, but use it once to verify it works */ - tap_equal('A', @$redis->lGet($key, 0), 'retrieve element 0'); + tap_equal('A', $redis->lIndex($key, 0), 'retrieve element 0'); tap_equal('B', $redis->lIndex($key, 1), 'retrieve element 1'); tap_equal('C', $redis->lIndex($key, 2), 'retrieve element 2'); tap_equal('C', $redis->lIndex($key, -1), 'retrieve last element'); @@ -202,8 +192,7 @@ function test_redis() { tap_equal(4, $redis->rpush($key, 'A', 'B', 'B', 'C'), 'add new elements'); - /* Redis->lRemove is depreacted, but use it once to verity it works */ - tap_equal(1, @$redis->lRemove($key, 'B', 1), 'remove first occurence of B'); + tap_equal(1, @$redis->lRem($key, 'B', 1), 'remove first occurence of B'); tap_equal(['A', 'B', 'C'], $redis->lrange($key, 0, -1), 'verify list elements'); tap_equal(0, $redis->lRem($key, 'NOT_IN_LIST', 1), 'remove missing element'); diff --git a/tests/integration/redis/test_lremove.php b/tests/integration/redis/test_lremove.php new file mode 100644 index 000000000..49861c08b --- /dev/null +++ b/tests/integration/redis/test_lremove.php @@ -0,0 +1,64 @@ +')) { + die("skip: Redis <= 5.3.7 required\n"); +} + +require("skipif.inc"); +*/ + +/*INI +newrelic.datastore_tracer.database_name_reporting.enabled = 1 +newrelic.datastore_tracer.instance_reporting.enabled = 1 +*/ + +/*EXPECT +ok - add new elements +ok - remove first occurence of B +ok - verify list elements +*/ + +/*EXPECT_METRICS_EXIST +Datastore/operation/Redis/lremove +*/ + +require_once(realpath (dirname ( __FILE__ )) . '/../../include/helpers.php'); +require_once(realpath (dirname ( __FILE__ )) . '/../../include/integration.php'); +require_once(realpath (dirname ( __FILE__ )) . '/../../include/tap.php'); +require_once(realpath (dirname ( __FILE__ )) . '/redis.inc'); + +function test_redis() { + global $REDIS_HOST, $REDIS_PORT; + + $redis = new Redis(); + $redis->connect($REDIS_HOST, $REDIS_PORT); + + /* generate unique key names to use for this test run */ + $key = randstr(16); + $key2 = "{$key}_b"; + if ($redis->exists([$key, $key2])) { + die("skip: key(s) already exist: $key, $key2\n"); + } + + /* Ensure the keys don't persist (too much) longer than the test. */ + $redis->expire($key, 30 /* seconds */); + $redis->expire($key2, 30 /* seconds */); + + tap_equal(4, $redis->rpush($key, 'A', 'B', 'B', 'C'), 'add new elements'); + + /* Redis->lRemove is depreacted, but use it once to verity it works */ + tap_equal(1, @$redis->lRemove($key, 'B', 1), 'remove first occurence of B'); + tap_equal(['A', 'B', 'C'], $redis->lrange($key, 0, -1), 'verify list elements'); +} +test_redis(); From 2f6cb3f8fb23202f99e2cdda06362a54c61642e5 Mon Sep 17 00:00:00 2001 From: bduranleau-nr <106178551+bduranleau-nr@users.noreply.github.com> Date: Mon, 18 Dec 2023 10:04:25 -0600 Subject: [PATCH 16/17] feat: add PHP 8.3 support (#797) Adds non-OAPI PHP 8.3 support to the agent. --------- Co-authored-by: Michael Fulbright --- .github/workflows/code-coverage-baseline.yml | 4 ++-- .github/workflows/make-agent.yml | 2 +- .github/workflows/make-integration-tests.yml | 2 +- .github/workflows/test-agent.yml | 8 ++++---- agent/newrelic-install.sh | 13 +++++++++++-- agent/php_includes.h | 1 + docs/development.md | 2 +- files/Dockerfile | 2 +- make/php_versions.mk | 2 +- make/release.mk | 1 + 10 files changed, 24 insertions(+), 13 deletions(-) diff --git a/.github/workflows/code-coverage-baseline.yml b/.github/workflows/code-coverage-baseline.yml index 952ab8459..d5a700c28 100644 --- a/.github/workflows/code-coverage-baseline.yml +++ b/.github/workflows/code-coverage-baseline.yml @@ -69,7 +69,7 @@ jobs: matrix: platform: [gnu, musl] arch: [amd64] - php: ['7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2'] + php: ['7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2', '8.3'] include: - codecov: 0 - platform: gnu @@ -147,7 +147,7 @@ jobs: matrix: platform: [gnu, musl] arch: [amd64] - php: ['7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2'] + php: ['7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2', '8.3'] include: - codecov: 0 - platform: gnu diff --git a/.github/workflows/make-agent.yml b/.github/workflows/make-agent.yml index b10a2a45d..9d2d76dd5 100644 --- a/.github/workflows/make-agent.yml +++ b/.github/workflows/make-agent.yml @@ -32,7 +32,7 @@ jobs: strategy: matrix: platform: [gnu, musl] - php: ['8.0', '8.1', '8.2'] + php: ['8.0', '8.1', '8.2', '8.3'] steps: - name: Checkout Repo uses: actions/checkout@v3 diff --git a/.github/workflows/make-integration-tests.yml b/.github/workflows/make-integration-tests.yml index 8128e98cf..ab90a136b 100644 --- a/.github/workflows/make-integration-tests.yml +++ b/.github/workflows/make-integration-tests.yml @@ -34,7 +34,7 @@ jobs: fail-fast: true matrix: platform: [gnu, musl] - php: ['8.0', '8.1', '8.2'] + php: ['8.0', '8.1', '8.2', '8.3'] steps: - name: Checkout integration tests uses: actions/checkout@v3 diff --git a/.github/workflows/test-agent.yml b/.github/workflows/test-agent.yml index 1d78f05d8..359c7c24f 100644 --- a/.github/workflows/test-agent.yml +++ b/.github/workflows/test-agent.yml @@ -67,12 +67,12 @@ jobs: env: IMAGE_NAME: newrelic/nr-php-agent-builder IMAGE_TAG: make-php - IMAGE_VERSION: ${{vars.MAKE_PHP_VERSION}} + IMAGE_VERSION: v2 strategy: matrix: platform: [gnu, musl] arch: [amd64, arm64] - php: ['7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2'] + php: ['7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2', '8.3'] exclude: - arch: arm64 php: '7.0' @@ -183,7 +183,7 @@ jobs: matrix: platform: [gnu, musl] arch: [amd64, arm64] - php: ['7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2'] + php: ['7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2', '8.3'] exclude: - arch: arm64 php: '7.0' @@ -248,7 +248,7 @@ jobs: LIBC: ${{matrix.platform}} PLATFORM: linux/${{matrix.arch}} AGENT_CODE: ${{github.workspace}}/php-agent - IMAGE_VERSION: ${{vars.MAKE_PHP_VERSION}} + IMAGE_VERSION: v2 working-directory: ./php-agent run: | make test-services-start diff --git a/agent/newrelic-install.sh b/agent/newrelic-install.sh index 27edb843b..d538bb260 100755 --- a/agent/newrelic-install.sh +++ b/agent/newrelic-install.sh @@ -335,10 +335,10 @@ for pmv in "20151012" "20160303" "20170718" "20180731" "20190902"; do done fi # Currently supported versions: -# (8.0, 8.1, 8.2) +# (8.0, 8.1, 8.2, 8.3) # for x64 and aarch64 if [ ${arch} = x64 ] || [ ${arch} = aarch64 ]; then - for pmv in "20200930" "20210902" "20220829"; do + for pmv in "20200930" "20210902" "20220829" "20230831"; do check_file "${ilibdir}/agent/${arch}/newrelic-${pmv}.so" done fi @@ -543,6 +543,7 @@ add_to_path /usr/local/php-7.4/bin add_to_path /usr/local/php-8.0/bin add_to_path /usr/local/php-8.1/bin add_to_path /usr/local/php-8.2/bin +add_to_path /usr/local/php-8.3/bin add_to_path /opt/local/bin add_to_path /usr/php/bin @@ -555,6 +556,7 @@ add_to_path /usr/php-7.4/bin add_to_path /usr/php-8.0/bin add_to_path /usr/php-8.1/bin add_to_path /usr/php-8.2/bin +add_to_path /usr/php-8.3/bin add_to_path /usr/php/7.0/bin add_to_path /usr/php/7.1/bin @@ -564,6 +566,7 @@ add_to_path /usr/php/7.4/bin add_to_path /usr/php/8.0/bin add_to_path /usr/php/8.1/bin add_to_path /usr/php/8.2/bin +add_to_path /usr/php/8.3/bin add_to_path /opt/php/bin add_to_path /opt/zend/bin @@ -576,6 +579,7 @@ add_to_path /opt/php-7.4/bin add_to_path /opt/php-8.0/bin add_to_path /opt/php-8.1/bin add_to_path /opt/php-8.2/bin +add_to_path /opt/php-8.3/bin if [ -n "${NR_INSTALL_PATH}" ]; then oIFS="${IFS}" @@ -1060,6 +1064,10 @@ for this copy of PHP. We apologize for the inconvenience. pi_php8="yes" ;; + 8.3.*) + pi_php8="yes" + ;; + *) error "unsupported version '${pi_ver}' of PHP found at: ${pdir} @@ -1239,6 +1247,7 @@ does not exist. This particular instance of PHP will be skipped. 8.0.*) pi_modver="20200930" ;; 8.1.*) pi_modver="20210902" ;; 8.2.*) pi_modver="20220829" ;; + 8.3.*) pi_modver="20230831" ;; esac log "${pdir}: pi_modver=${pi_modver}" diff --git a/agent/php_includes.h b/agent/php_includes.h index 2a350b9c1..8952fab0b 100644 --- a/agent/php_includes.h +++ b/agent/php_includes.h @@ -53,6 +53,7 @@ #define ZEND_8_0_X_API_NO 20200930 #define ZEND_8_1_X_API_NO 20210902 #define ZEND_8_2_X_API_NO 20220829 +#define ZEND_8_3_X_API_NO 20230831 #if ZEND_MODULE_API_NO >= ZEND_5_6_X_API_NO #include "Zend/zend_virtual_cwd.h" diff --git a/docs/development.md b/docs/development.md index 64157900b..c85755903 100644 --- a/docs/development.md +++ b/docs/development.md @@ -58,7 +58,7 @@ _(most operating systems package these with `-dev` or `-devel` suffixes)_ ### PHP -The PHP agent supports PHP versions `7.0`, `7.1`, `7.2`, `7.3`, `7.4`,`8.0`, `8.1`, and `8.2`. +The PHP agent supports PHP versions `7.0`, `7.1`, `7.2`, `7.3`, `7.4`,`8.0`, `8.1`, '8.2', and `8.3`. ## Build the PHP Agent diff --git a/files/Dockerfile b/files/Dockerfile index b0bec8bdd..f13cd367b 100644 --- a/files/Dockerfile +++ b/files/Dockerfile @@ -9,7 +9,7 @@ ARG PHP_VER -FROM php:${PHP_VER:-8.2} +FROM php:${PHP_VER:-8.3} RUN docker-php-source extract diff --git a/make/php_versions.mk b/make/php_versions.mk index c3477bac9..0629bfabd 100644 --- a/make/php_versions.mk +++ b/make/php_versions.mk @@ -3,4 +3,4 @@ # SPDX-License-Identifier: Apache-2.0 # -PHP_VERSION_LIST=$${PHPS:-8.2 8.1 8.0 7.4 7.3 7.2 7.1 7.0} +PHP_VERSION_LIST=$${PHPS:-8.3 8.2 8.1 8.0 7.4 7.3 7.2 7.1 7.0} diff --git a/make/release.mk b/make/release.mk index 0f3ce7e0a..6bb9eba61 100644 --- a/make/release.mk +++ b/make/release.mk @@ -163,6 +163,7 @@ release-$1-zts: Makefile agent | releases/$$(RELEASE_OS)/agent/$$(RELEASE_ARCH)/ endef +$(eval $(call RELEASE_AGENT_TARGET,8.3,20230831)) $(eval $(call RELEASE_AGENT_TARGET,8.2,20220829)) $(eval $(call RELEASE_AGENT_TARGET,8.1,20210902)) $(eval $(call RELEASE_AGENT_TARGET,8.0,20200930)) From 1c793c99d69f97294b60d3447c2983ad08ecfbd9 Mon Sep 17 00:00:00 2001 From: bduranleau-nr <106178551+bduranleau-nr@users.noreply.github.com> Date: Mon, 18 Dec 2023 15:51:08 -0600 Subject: [PATCH 17/17] chore: unpin PHP image version (#804) --- .github/workflows/test-agent.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test-agent.yml b/.github/workflows/test-agent.yml index 359c7c24f..2736d2cc1 100644 --- a/.github/workflows/test-agent.yml +++ b/.github/workflows/test-agent.yml @@ -67,7 +67,7 @@ jobs: env: IMAGE_NAME: newrelic/nr-php-agent-builder IMAGE_TAG: make-php - IMAGE_VERSION: v2 + IMAGE_VERSION: ${{vars.MAKE_PHP_VERSION}} strategy: matrix: platform: [gnu, musl] @@ -248,7 +248,7 @@ jobs: LIBC: ${{matrix.platform}} PLATFORM: linux/${{matrix.arch}} AGENT_CODE: ${{github.workspace}}/php-agent - IMAGE_VERSION: v2 + IMAGE_VERSION: ${{vars.MAKE_PHP_VERSION}} working-directory: ./php-agent run: | make test-services-start