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	❌	70/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 48.93617% with 24 lines in your changes missing coverage. Please review.

Project coverage is 77.95%. Comparing base (996dcc9) to head (04cbaf6).
Report is 1 commits behind head on dev.

Files with missing lines	Patch %	Lines
agent/fw_drupal8.c	48.93%	24 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1010      +/-   ##
==========================================
- Coverage   78.16%   77.95%   -0.22%     
==========================================
  Files         197      197              
  Lines       27295    27465     +170     
==========================================
+ Hits        21336    21411      +75     
- Misses       5959     6054      +95

Flag	Coverage Δ
agent-for-php-7.2	`78.06% <22.22%> (-0.10%)`	⬇️
agent-for-php-7.3	`78.08% <22.22%> (-0.10%)`	⬇️
agent-for-php-7.4	`77.83% <45.45%> (-0.06%)`	⬇️
agent-for-php-8.0	`77.20% <50.00%> (-0.06%)`	⬇️
agent-for-php-8.1	`77.70% <50.00%> (-0.22%)`	⬇️
agent-for-php-8.2	`77.30% <50.00%> (-0.22%)`	⬇️
agent-for-php-8.3	`77.30% <50.00%> (-0.22%)`	⬇️
agent-for-php-8.4	`77.32% <50.00%> (-0.22%)`	⬇️

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

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.