feat(agent): Drupal hook attribute instrumentation

[bduranleau-nr]

Adds support for Drupal Attribute Hooks added in Drupal 11.1.

[newrelic-php-agent-bot]

Test Suite	Status	Result
Multiverse	✅	8/8 passing
SOAK	✅	79/79 passing

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 33.33333% with 28 lines in your changes missing coverage. Please review.

Project coverage is 77.87%. Comparing base (8ddc8af) to head (c811020).
Report is 1 commits behind head on dev.

Files with missing lines	Patch %	Lines
agent/fw_drupal8.c	33.33%	28 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1030      +/-   ##
==========================================
+ Coverage   77.58%   77.87%   +0.28%     
==========================================
  Files         198      198              
  Lines       27715    27755      +40     
==========================================
+ Hits        21503    21613     +110     
+ Misses       6212     6142      -70     

Flag	Coverage Δ
agent-for-php-7.2	77.97% <0.00%> (+0.24%)	⬆️
agent-for-php-7.3	77.99% <0.00%> (+0.24%)	⬆️
agent-for-php-7.4	77.75% <33.33%> (+0.29%)	⬆️
agent-for-php-8.0	77.14% <33.33%> (+0.29%)	⬆️
agent-for-php-8.1	77.64% <33.33%> (+0.29%)	⬆️
agent-for-php-8.2	77.25% <33.33%> (+0.29%)	⬆️
agent-for-php-8.3	77.25% <33.33%> (?)
agent-for-php-8.4	77.26% <33.33%> (+0.29%)	⬆️

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.

[lavarou]

newrelic-php-agent's code base doesn't seem to have a standard around structuring the code. Golang has the following standard defined in Effective Go:

[...] a common situation where code must guard against a sequence of error conditions. The code reads well if the successful flow of control runs down the page, eliminating error cases as they arise. Since error cases tend to end in return statements, the resulting code needs no else statements.

The readability of the above could could definitely benefit from applying Effective Go standards. Consider:

  char *reason = NULL;
  bool hooks_instrumented = false;
  hook_implementation_map = nr_php_get_zval_object_property(
      module_handler, "hookImplementationsMap");
  if (NULL == hook_implementation_map) {
    reason = "NULL hookImplementationsMap object property";
    goto LEAVE;
  }
  if (!nr_php_is_zval_valid_array(hook_implementation_map)) {
    goto LEAVE;
  }
...
  // all checks passed, hooks have been instrumented:
  hooks_instrumented = true;
LEAVE:
  NR_FREE_HOOK_MEM
  if (NULL != reason) {
    nrl_warning(NRL_FRAMEWORK, reason);
  }
  return hooks_instrumented;

[zsistla]

hook_implementation_map doesn't need to be checked for NULL here because nr_php_is_zval_valid_array (like all nr_php_is_zval_* funcs) also checks for NULL.

[bduranleau-nr]

I intentionally chose not to use goto here. In general I believe goto is a bad practice that interrupts the structure of code and adds complexity and possible side-effects.

[zsistla]

If you remove the strdups and free the nr_formatted hookpath string right after it's used, these could be just returns to exit early because at that point, it won't need to call NR_FREE_HOOK_MEM everywhere.

[lavarou]

goto is a C language construct, like switch or if. And, like any construct, it can be used to improve code’s maintainability and readability but it can also be used to make the code a mess. I'm in favor of Flattening Arrow Code. I'm not saying goto is always the way to go, but in this particular case, it would help.

[bduranleau-nr]

Given the refactors, do you still want to see goto used to escape the loops, or is just returning enough?

[lavarou]

Considering current code structure, it's ok to flatten/short-circuit nested ifs with return.

[lavarou]

nr_drupal_hook_attribute_instrument will only return true if it's able to walk the entire hookImplementationsMap. What will happen if it fails mid way? Which hook instrumentation will be used? Does nr_drupal_hook_attribute_instrument cleanup its partial work if it fails to walk the entire hookImplementationsMap? Could you add a test for this case?

[bduranleau-nr]

If walking the map fails halfway through, we exit the function and return false. The wraprecs that are already created remain, and we revert back to the old method of wrapping hooks.

Which hook instrumentation will be used?

The answer would be "both". The attribute based instrumentation would persist for anything it managed to wrap, and the other methods act as a fallback to hopefully plug the gaps. I don't see anticipate any negative side-effects from this, do you?

Could you add a test for this case?

This would be difficult and doesn't fit neatly into our established test paradigms, The dependency here is a lot of Drupal code, from Drupal::moduleHandler to the hookImplementationsMap. Is there a specific concern you have with this failing partway though walking the map?