fix(agent): properly name drupal cached pages #1010

bduranleau-nr · 2025-01-21T21:30:34Z

Previously, Drupal cached pages were aggregated under the page_cache name and metric. This PR injects logic that allows the agent to retrieve the _controller information on a request to meaningfully name a txn involving a cached page.

Adds new INI setting newrelic.framework.drupal.page_cache_naming.enabled to toggle behavior on/off.

Previously, Drupal cached pages were aggregated under the `page_cache` name and metric. This PR injects logic that allows the agent to retrieve the `_controller` information on a request to meaningfully name a txn involving a cached page.

bduranleau-nr · 2025-01-21T21:30:54Z

agent/fw_drupal8.c

-                                       NULL,
-                                       &NRPRG(exception_filters))) {
+                                       NULL, &NRPRG(exception_filters))) {


newrelic-php-agent-bot · 2025-01-21T21:30:59Z

Test Suite	Status	Result
Multiverse	❌	4/8 passing
SOAK	❌	69/72 passing

bduranleau-nr · 2025-01-21T21:31:01Z

agent/fw_drupal8.c

-                                           const zend_class_entry* ce,
-                                           const char* method,
-                                           size_t method_len,
-                                           nrspecialfn_t before_callback,
-                                           nrspecialfn_t after_callback,
-                                           nrspecialfn_t clean_callback) {
+    const zend_class_entry* ce,
+    const char* method,
+    size_t method_len,
+    nrspecialfn_t before_callback,
+    nrspecialfn_t after_callback,
+    nrspecialfn_t clean_callback) {


bduranleau-nr · 2025-01-21T21:31:06Z

agent/fw_drupal8.c

-                              class_method, nr_strlen(class_method),
-                              before_callback, after_callback, clean_callback);
+        class_method, nr_strlen(class_method), before_callback, after_callback,
+        clean_callback);

    nr_free(class_method);
  }
 }
-#endif // OAPI
+#endif  // OAPI


bduranleau-nr · 2025-01-21T21:31:18Z

agent/fw_drupal8.c

-  zval* curr_hook
-      = (zval*)nr_stack_get_top(&NRPRG(drupal_invoke_all_hooks));
+  zval* curr_hook = (zval*)nr_stack_get_top(&NRPRG(drupal_invoke_all_hooks));


bduranleau-nr · 2025-01-21T21:31:23Z

agent/fw_drupal8.c

-#endif // OAPI
+#endif  // OAPI


bduranleau-nr · 2025-01-21T21:31:27Z

agent/fw_drupal8.c

-#endif // not OAPI
+#endif  // not OAPI


bduranleau-nr · 2025-01-21T21:31:32Z

agent/fw_drupal8.c

-#endif // OAPI
+#endif  // OAPI


bduranleau-nr · 2025-01-21T21:31:35Z

agent/fw_drupal8.c

-#endif // OAPI
+#endif  // OAPI


bduranleau-nr · 2025-01-21T21:31:41Z

agent/fw_drupal8.c

-#endif // not OAPI
+#endif  // not OAPI

-leave: ;
+leave:;
 #if ZEND_MODULE_API_NO < ZEND_8_0_X_API_NO \
    || defined OVERWRITE_ZEND_EXECUTE_DATA
  /* for OAPI, the _after callback handles this free */
  nr_php_arg_release(&hook);
-#endif // not OAPI
+#endif  // not OAPI


bduranleau-nr · 2025-01-21T21:31:46Z

agent/fw_drupal8.c

-#endif // OAPI
+#endif  // OAPI


bduranleau-nr · 2025-01-21T21:31:55Z

agent/fw_drupal8.c

-                                 ce, NR_PSTR("invokeallwith"),
-                                 nr_drupal94_invoke_all_with,
-                                 nr_drupal94_invoke_all_with_after,
-                                 nr_drupal94_invoke_all_with_clean);
+      ce, NR_PSTR("invokeallwith"), nr_drupal94_invoke_all_with,
+      nr_drupal94_invoke_all_with_after, nr_drupal94_invoke_all_with_clean);


bduranleau-nr · 2025-01-21T21:40:23Z

Do we want to add some kind of indication to the txn name or metric that this was the result of a cached page hit? For example, prepending the controller name/route with cached? Current implementation has no differentiation between a request that hits a cached page vs a non-cached page in the metrics the agent will report to the collector- maybe users would find this information useful to determine how often their cache is getting hit, or if there are problems specifically with cached pages vs non-cached.

codecov-commenter · 2025-01-21T21:53:58Z

Codecov Report

Attention: Patch coverage is 40.74074% with 32 lines in your changes missing coverage. Please review.

Project coverage is 77.93%. Comparing base (996dcc9) to head (17a32d3).
Report is 1 commits behind head on dev.

Files with missing lines	Patch %	Lines
agent/fw_drupal8.c	40.74%	32 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1010      +/-   ##
==========================================
- Coverage   78.16%   77.93%   -0.24%     
==========================================
  Files         197      197              
  Lines       27295    27472     +177     
==========================================
+ Hits        21336    21410      +74     
- Misses       5959     6062     +103

Flag	Coverage Δ
agent-for-php-7.2	`78.04% <19.23%> (-0.12%)`	⬇️
agent-for-php-7.3	`78.06% <19.23%> (-0.12%)`	⬇️
agent-for-php-7.4	`77.80% <37.25%> (-0.09%)`	⬇️
agent-for-php-8.0	`77.18% <41.50%> (-0.09%)`	⬇️
agent-for-php-8.1	`77.67% <41.50%> (-0.25%)`	⬇️
agent-for-php-8.2	`77.28% <41.50%> (-0.24%)`	⬇️
agent-for-php-8.3	`77.28% <41.50%> (-0.24%)`	⬇️
agent-for-php-8.4	`77.29% <41.50%> (-0.24%)`	⬇️

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

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

agent/fw_drupal8.c

mfulb · 2025-01-23T20:18:08Z

agent/fw_drupal8.c

+  if (NULL == nrfn) {
+    nrl_warning(NRL_FRAMEWORK,
+                "Failed to retrieve NR cached page naming function");
+    goto end;


This could probably set name to "page_cache" since this is what we just did above when naming wasn't requested. Maybe change the warning to include a notice that "page_cache" was being used as the name because of the failure.

Maybe a better solution to multiple name = nr_strdup("page_cache") checks is to just check after NR_PHP_WRAPPER_CALL if name == NULL and set it to page_cache that way?

My concern was that it appeared we had a case where we missed naming - if your solution handles these it should work fine.

I think 04cbaf6 is a slight improvement on the fallback naming

On testing, that seems to have broken something... I'll have to look into this a little more

agent/fw_drupal8.c

This change handles the edge case exposed in our tests where the agent may fail to inject the cache naming function at enable because it looked for drupal and symfony classes before composer autoload had time to load them. In the normal flow, this would only be a temporary problem since nr_drupal_enable should be re-run every request, and subsequent requests should have these resources available. Our test setup, however, shows that if in the process of handling one request, another request is generated and the agent doesn't re-trigger RINIT processing, the function name from the first request will bleed through to the second request and result in an incorrect name. To fix, extract logic responsible for injecting the cache naming function to its own function and call both at enable and within the PageCache::get function instrumentation, provided the injection function (newrelic_name_cached) is not detected when we look for it. This should strike the best balance between performance and correctness.

feat(agent): properly name drupal cached pages

5ccb3a6

Previously, Drupal cached pages were aggregated under the `page_cache` name and metric. This PR injects logic that allows the agent to retrieve the `_controller` information on a request to meaningfully name a txn involving a cached page.