Allow for RUM Manual Instrumentation

New Relic updated their docs:

IMPORTANT

Drupal 10.2 introduced a new change → that causes it to set a content-length header. New Relic PHP agent is unable to auto-inject the browser auto-instrumentation when the HTTP header field Content-Length is set. To keep using browser monitoring, disable browser auto-instrumentation and manually insert the JavaScript header and footer into your templates.

For manual instrumentation to be enabled, the disable_autorum setting will need to be disabled.

I'm thinking we'd be better off to remove the disable_autorum setting and replace it with a rum_instrumentation settings with three options: disabled, auto, and manual.

Interested in having this fix ready for a client site! Do you think it may be available later this month?

@Erik_MC - If your team would be able to test the merge request and report back, that would be helpful in moving the issue forward. The MR is coded based on New Relic's documentation; however, I don't have a New Relic instance to test against myself.

I have verified that Javascript snippets are not injected on Drupal 10 version, and after adding Chris MR !5, it is working correctly. Additionally, I have attached a video of the verification process.

@vipin.mittal18 - Thank you for your review.

It's not explicitly stated above, but here's the expected behavior for each of the three settings:

Pre 10.2.0
Disable browser monitoring - No JS inserted
Allow auto-instrumentation - JS inserted (assuming auto-instrumentation is enabled on server)
Manual instrumentation - JS inserted

10.2.0+
Disable browser monitoring - No JS inserted
Allow auto-instrumentation - No JS inserted (because auto-instrumentation is incompatible with D10.2.0+)
Manual instrumentation - JS inserted

My opinion is same as yours @chris

The maintainers are requested to release these fixes as most of the customers are being impacted and losing monitoring and tracking.

Makes sense. I remember browser instrumentation generally not working so I've never run it though so I've got some questions for people that do use it:

1. Are there possibly additional problems with caching with how manual adds adds code? Specifically, is the "header" being added unique to the request? I see the settings tag but should this be a lazy callback or have additional tags to avoid page/render caches?
2. In the post update, should it take the Drupal version into account and migrate to manual instead of migrating to a auto that behaves like manual with a not immediately obvious warning? Or maybe it should warn the user during the update? Unsure.

@neclimdul - Thanks for reviewing this merge request!

1. Are there possibly additional problems with caching with how manual adds adds code? Specifically, is the "header" being added unique to the request? I see the settings tag but should this be a lazy callback or have additional tags to avoid page/render caches?

I'm not aware of any caching-related issues. The two functions, newrelic_get_browser_timing_header() and newrelic_get_browser_timing_footer() are unaware of anything related to the request or the response. In the 7.x branch of this module, they are implemented without any accommodations for caching.

2. In the post update, should it take the Drupal version into account and migrate to manual instead of migrating to a auto that behaves like manual with a not immediately obvious warning? Or maybe it should warn the user during the update? Unsure.

All that the post_update function does is preserve the same behavior following the update. The merge request does add a check to new_relic_rpm_requirements() that alerts site owners about the issue of auto-instrumentation and D10.2.0+.

Here's a screenshot of the requirements error message, which is added in the MR:

We could run the same compatibility check as in new_relic_rpm_requirements() and return a warning string if the site is configured to allow auto-instrumentation on D10.2.0+.

We are actively using this patch in a production environment. It does seem to work, however, there has been question about the placement of the snippets.

Shouldn't the JS timings header code get inserted into the HEAD tag? Would it make more sense to use something like newrelic_get_browser_timing_header() to move it up as much as possible?

Per the New Relic manual instrumentation documentation:

Insert the return value of newrelic_get_browser_timing_header() as part of the output page's <head> tag, preferably as the very first thing.

Same with the footer:

As the very last thing before the closing </body> tag in the document, or as close to it as possible, insert the return value of newrelic_get_browser_timing_footer().

@jack.minster - Thanks for the feedback! I'm glad to hear the MR code is working for you all.

Re placement of the header JS, I do see that in their documentation, too; however, when I look at their WordPress and Drupal 7 examples, they're calling the newrelic_get_browser_timing_header() inside <body>.

The placement of footer JS is trickier. The MR currently uses hook_page_bottom() to insert the footer JS; however, most JS is loaded after that before the </body> tag. I've tried various methods to address this without success:

1. Call hook_page_attachments() to add a script to scripts_bottom:

function new_relic_rpm_page_attachments(array &$attachments) {
  $attachments['#attached']['scripts_bottom'][] = [
    [
      '#tag' => 'script',
      '#attributes' => [
        'type' => 'text/javascript',
      ],
      '#value' => 'console.log("test");',
    ],
    'new_relic_rpm_rum_footer',
  ];
}

This isn't allowed:

LogicException: You are not allowed to use scripts_bottom in #attached. in Drupal\Core\Render\HtmlResponseAttachmentsProcessor->processAttachments() (line 150 of core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php).

2. Register a library, pass the return value of newrelic_get_browser_timing_footer() via drupalSettings to a JS file, and then pass that value into eval():

new_relic_rpm.libraries.yml:

rum_footer:
  js:
    js/new-relic-browser-monitoring.js: {}
  dependencies:
    - core/drupalSettings

new_relic_rpm.module:

function new_relic_rpm_page_attachments(array &$attachments) {
  $attachments['#attached']['library'][] = 'new_relic_rpm/rum_footer';
  $attachments['#attached']['drupalSettings']['rum_footer']['markup'] = 'console.log("test")';
}

js/new-relic-browser-monitoring.js:

(function (Drupal, drupalSettings) {
  Drupal.behaviors.NRRumFooter = {
    attach: function (context, settings) {
      once('myBehavior', 'html').forEach(function (element) {
        eval(drupalSettings.rum_footer.markup);
      })
    }
  };
})(Drupal, drupalSettings);

This executes the JS code in scripts_bottom; however, we can't weight the library with a high value to push the JS insertion to the bottom.

rum_footer:
  js:
    js/new-relic-browser-monitoring.js:
      weight: 999
  dependencies:
    - core/drupalSettings

Exception:

UnexpectedValueException: The new_relic_rpm/rum_footer library defines a positive weight for 'js/new-relic-browser-monitoring.js'. Only negative weights are allowed (but should be avoided). Instead of a positive weight, specify accurate dependencies for this library. in Drupal\Core\Asset\LibraryDiscoveryParser->buildByExtension() (line 197 of core/lib/Drupal/Core/Asset/LibraryDiscoveryParser.php).

I'm at a bit of a loss. It may not be possible to get that JS code immediately before </body> using Drupal's API. It may be necessary to customize html.twig.html.

That said, if someone could weigh in regarding the placement of the footer JS before/after all of the libraries JS, that would be helpful. It strikes me that we may not get valid browser monitoring results if the footer JS is inserted before the rest of the JS on the page.

@chris-burge I know that NR also shows examples with the header snippet inside the body tag instead of inside the head tag, which is contradictory. It does render as the first child element of the body tag currently, though. I'm waiting for some more specific from some of our NR performance experts to see if they recommend a change.

As for the footer, I'm not sure about that either. I've asked our lead developer for some feedback as I know he was able to get the header snippet up in the head tag, perhaps he has ideas for the footer snippet as well.

@jack.minster - I'll be interested in feedback you get from your New Relic performance experts.

Re the footer JS, I found that you can control library weights by decorating the html_response.attachments_processor service. I need to clean up my local code, and then I'll push it to the MR.

GitLab CI isn't configure for this project yet, so automated tests won't run, but tests are passing locally:

$ ../vendor/phpunit/phpunit/phpunit -c core/ modules/contrib/new_relic_rpm/tests/src/ --verbose
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.27
Configuration: /var/www/html/web/core/phpunit.xml.dist

Testing /var/www/html/web/modules/contrib/new_relic_rpm/tests/src
...................                                               19 / 19 (100%)

Time: 00:04.735, Memory: 8.00 MB

OK (19 tests, 33 assertions)

HTML output was generated
https://d10-lbca.ddev.site/sites/simpletest/browser_output/Drupal_Tests_new_relic_rpm_Functional_AdminUiTest-9-53556339.html
https://d10-lbca.ddev.site/sites/simpletest/browser_output/Drupal_Tests_new_relic_rpm_Functional_AdminUiTest-10-53556339.html
https://d10-lbca.ddev.site/sites/simpletest/browser_output/Drupal_Tests_new_relic_rpm_Functional_AdminUiTest-11-53556339.html
https://d10-lbca.ddev.site/sites/simpletest/browser_output/Drupal_Tests_new_relic_rpm_Functional_AdminUiTest-12-53556339.html
https://d10-lbca.ddev.site/sites/simpletest/browser_output/Drupal_Tests_new_relic_rpm_FunctionalJavascript_InstrumentationTest-5-95610470.html
https://d10-lbca.ddev.site/sites/simpletest/browser_output/Drupal_Tests_new_relic_rpm_FunctionalJavascript_InstrumentationTest-6-95610470.html

Remaining self deprecation notices (3)

  1x: user_role_names() is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Use \Drupal\user\Entity\Role::loadMultiple() and, if necessary, an inline implementation instead. See https://www.drupal.org/node/3349759
    1x in AdminUiTest::testSettingsPage from Drupal\Tests\new_relic_rpm\Functional

  1x: user_roles() is deprecated in drupal:10.2.0 and is removed from drupal:11.0.0. Use \Drupal\user\Entity\Role::loadMultiple() and, if necessary, an inline implementation instead. See https://www.drupal.org/node/3349759
    1x in AdminUiTest::testSettingsPage from Drupal\Tests\new_relic_rpm\Functional

  1x: Method "Symfony\Component\EventDispatcher\EventSubscriberInterface::getSubscribedEvents()" might add "array" as a native return type declaration in the future. Do the same in implementation "Drupal\new_relic_rpm\EventSubscriber\RoutingTransactionNameSubscriber" now to avoid errors or add an explicit @return annotation to suppress this message.
    1x in RoutingTransactionNameSubscriberTest::testSetsTransactionNameForAllRoutes from Drupal\Tests\new_relic_rpm\Unit\EventListener

@jack.minster @chris-burge

Patch for making head injection to be configurable.

When I look at a Drupal site where RUM auto-instrumentation is used, I do find the header JS in <head>. I think that's the way to go.

@chris-burge for some reason the library and settings aren't getting attached in our Acquia environment and therefore the footer snippet isn't getting generated/executed. We're going to localize the patch prior to your last commit and use it as a local patch until we determine the issue.

The patch from @pieterdtt was to demonstrate how he get the header snippet moved up into the head tag if you'd like to investigate that as an option.

@jack.minster - Thanks for that feedback. I'll look into the footer JS insertion issue. I've been doing some work with the test coverage for that piece. One change I've made (but haven't pushed yet) is to execute JS that adds an element to the DOM in the test. The current test results in a false positive.

I do have some work in progress that moves the header JS into <head>.

@jack.minster - Would you mind sending me a DM on my Drupal.org contact form with your contact info and Acquia application docroot?

@jack.minster - Could you try out the latest code push to the MR? The header JS is now rendered in <HEAD> near the very top. I also abandoned the library hack for the footer. In HtmlResponseAttachmentsProcessorDecorator, the MR is now modifying the ::processAssetLibraries() method to insert the JS into scripts_bottom, bypassing the #attached types restrictions of ::processAttachments().

@pieterdtt - Thanks for patch #21. I incorporated it into the MR. Based on how New Relic's auto-instrumentation inserts the header JS into <head>, I think the best thing is to always insert the JS there (i.e. no need for placement configuration).

I'm not sure I understand why we're not using standard methods for attaching things to the footer.

@neclimdul - Drupal's APIs don't provide a way to insert the New Relic footer JS below Javascript inserted by Drupal's Libraries API. NR's docs for manual instrumentation state:

As the very last thing before the closing </body> tag in the document, or as close to it as possible, insert the return value of newrelic_get_browser_timing_footer().

Below is some sample HTML output from a fresh Drupal site. At the very top is where markup is inserted with hook_page_bottom(). At the bottom is code inserted with HtmlResponseAttachmentsProcessorDecorator::processAssetLibraries().

    <div>Inserted with hook_page_bottom()</div>
    
  <script type="application/json" data-drupal-selector="drupal-settings-json">{"path":{"baseUrl":"\/","pathPrefix":"","currentPath":"node","currentPathIsAdmin":false,"isFront":true,"currentLanguage":"en"},"pluralDelimiter":"\u0003","suppressDeprecationErrors":true,"ajaxPageState":{"libraries":"eJx9UFuOgzAMvFAgZ9iTVCaZLdaGGNkGltsvRYVqkdqf2PPwKBrKA9ebi5SOND5n6woEeiu1vcxQro7q_237tNPMSWro-H4beUQ8lrCxjl-fqMSs00ilfTFN4fpjny3P-M2kOGRKzjP26yBlW1XiN5BPUGnmOzlLbQxbWCZdT3GUBYrcdGvTFUmvCANp6ptKqrJc2YUzgvWiniY_PnLgYKs5htiRIVxavOIWlmjE16PKMBn0CHvse4sWZsZicX_bQfJU8AduD62d","theme":"olivero","theme_token":null},"ajaxTrustedUrl":{"\/search\/node":true},"bigPipePlaceholderIds":{"callback=Drupal%5CCore%5CRender%5CElement%5CStatusMessages%3A%3ArenderMessages\u0026args%5B0%5D\u0026token=_HAdUpwWmet0TOTe2PSiJuMntExoshbm1kh2wQzzzAA":true,"callback=announcements_feed.lazy_builders%3ArenderAnnouncements\u0026\u0026token=m--BNYDqgTao0j2QoLFf1TwfQN-Bm9xPlckw9QAC1h4":true,"callback=user.toolbar_link_builder%3ArenderToolbarLinks\u0026\u0026token=QPmvukHqpEJJ4rYEzUUFx0ERxrtfmQ9TMOp_hvyLHEk":true,"callback=user.toolbar_link_builder%3ArenderDisplayName\u0026\u0026token=XGROnxBLjNjdNZJ_VcOhVZtenx2tDkIKPfojS_uZFXA":true,"callback=shortcut.lazy_builders%3AlazyLinks\u0026\u0026token=5-XBI-QHgyU_l7Bu0FizHR7YBqK2bTVTBFF8Z0DVTis":true},"toolbar":{"breakpoints":{"toolbar.narrow":"only screen and (min-width: 16.5em)","toolbar.standard":"only screen and (min-width: 38.125em)","toolbar.wide":"only screen and (min-width: 61em)"},"subtreesHash":"DvNlpBZa2F73VL_ZCBelzSCBD08FX9eADzsmGiR_J_o"},"user":{"uid":"1","permissionsHash":"e04100661cd70d74a85410e24e7c918449cfc6bbc6d71bae1bf7fb8fa4d2bd8f"}}</script>
<script src="/core/assets/vendor/jquery/jquery.min.js?v=3.7.1"></script>
<script src="/core/assets/vendor/underscore/underscore-min.js?v=1.13.6"></script>
<script src="/core/assets/vendor/once/once.min.js?v=1.0.1"></script>
<script src="/core/assets/vendor/backbone/backbone-min.js?v=1.5.0"></script>
<script src="/core/misc/drupalSettingsLoader.js?v=10.2.6-dev"></script>
<script src="/core/misc/drupal.js?v=10.2.6-dev"></script>
<script src="/core/misc/drupal.init.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/contextual.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/models/StateModel.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/views/AuralView.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/views/KeyboardView.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/views/RegionView.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/views/VisualView.js?v=10.2.6-dev"></script>
<script src="/core/assets/vendor/tabbable/index.umd.min.js?v=6.2.0"></script>
<script src="/core/misc/progress.js?v=10.2.6-dev"></script>
<script src="/core/assets/vendor/loadjs/loadjs.min.js?v=4.2.0"></script>
<script src="/core/misc/debounce.js?v=10.2.6-dev"></script>
<script src="/core/misc/announce.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/navigation-utils.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/checkbox.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/messages.js?v=10.2.6-dev"></script>
<script src="/core/misc/message.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/message.theme.js?v=10.2.6-dev"></script>
<script src="/core/misc/ajax.js?v=10.2.6-dev"></script>
<script src="/core/misc/active-link.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/navigation.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/second-level-navigation.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/nav-resize.js?v=10.2.6-dev"></script>
<script src="/core/themes/olivero/js/search.js?v=10.2.6-dev"></script>
<script src="/core/misc/displace.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/toolbar.menu.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/toolbar.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/models/MenuModel.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/models/ToolbarModel.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/views/BodyVisualView.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/views/MenuVisualView.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/views/ToolbarAuralView.js?v=10.2.6-dev"></script>
<script src="/core/modules/toolbar/js/views/ToolbarVisualView.js?v=10.2.6-dev"></script>
<script src="/core/misc/tabbingmanager.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/contextual.toolbar.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/toolbar/models/StateModel.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/toolbar/views/AuralView.js?v=10.2.6-dev"></script>
<script src="/core/modules/contextual/js/toolbar/views/VisualView.js?v=10.2.6-dev"></script>
<script src="/modules/contrib/admin_toolbar/js/admin_toolbar.js?sf2suw"></script>
<script src="/modules/contrib/admin_toolbar/js/jquery.hoverIntent.js?sf2suw"></script>
<script src="/modules/contrib/admin_toolbar/js/admin_toolbar.hoverintent.js?sf2suw"></script>
<script src="/core/modules/toolbar/js/escapeAdmin.js?v=10.2.6-dev"></script>
<script src="/core/modules/big_pipe/js/big_pipe.js?v=10.2.6-dev"></script><div id="drupal-live-announce" class="visually-hidden" aria-live="polite" aria-busy="false">Tray "Administration menu" opened.</div>
<script type="text/javascript">console.log("Inserted with HtmlResponseAttachmentsProcessorDecorator::processAssetLibraries()");</script>

Using a decorator preserves the original service and also allows other code to decorate the html_response.attachments_processor, as well.

@jack.minster may wish to weigh in, as well.

Just pushed code to properly decorate the service. Unfortunately, seven of the methods on the class are protected, so I had to copy/paste them all over to the decorator. (The decorator can't access protected methods on the inner service.) I also added a test to ensure the decorator doesn't break core functionality.

Last code push takes a different approach (still using a decorator); however, it uses str_replace() to insert the rendered footer JS into the rendered markup before returning $this->decorated->processAttachments($response). This approach avoids the code duplication issue.

I received some feedback out of thread that I investigated. Here's a summary of the feedback:

The header JS renders without issue and consistently; however, the footer JS only renders on the first page load immediately after a Drupal cache rebuild.

The header JS is inserted using hook_page_attachments(), complete with a cache dependency. Simply put, it works as designed. Some wizardry is needed to get the footer JS inserted below other JS. What appears to be happening is that newrelic_get_browser_timing_footer() is getting called multiple times during the same request. If either newrelic_get_browser_timing_header() or newrelic_get_browser_timing_footer() are called more than once during the same request, they'll return an empty string on subsequent requests. I verified this manually.

I'm debating abandoning the service decoration approach and using an event subscriber to alter the DOM directly. I'm also considering modifying ExtensionAdapter::newrelic_get_browser_timing_header() and ExtensionAdapter::newrelic_get_browser_timing_footer() to leverage drupal_static().

When big_pipe is enabled, HtmlResponseAttachmentsProcessor->processAttachments() gets called several times, which means newrelic_get_browser_timing_footer() gets called multiple times, which means all calls after the initial call return an empty string. The latest MR code uses drupal_static() to cache the return value of newrelic_get_browser_timing_footer().

In manual testing, I was able to reproduce this behavior with Big Pipe enabled; however, enabling Big Pipe in InstrumentationTest didn't result in newrelic_get_browser_timing_footer() being called multiple times, so I added an event subscriber to the new_relic_rpm_intstrumentation_test module to simulate the behavior.

Hello! I am testing this on a site running Drupal 10.2.4, with this patch applied and "RUM Instrumentation" set to "Manual instrumentation"; however, even after clearing caches this does not seem to be adding any scripts to the header or footer. Is there perhaps a step that I am missing?

@e.ryan.schmidt@gmail.com - Is the New Relic PHP extension loaded?

A quick way to check is to navigate to the 'Status Report' page and scroll down to 'New Relic PHP Library', which should have a value of 'Exists'.

Hey Chris, thanks for following up! It looks like that extension isn't loaded, but I am testing this locally (via Lando). Unfortunately, New Relic is currently only added to the Production server, and my client requires a better way to verify this on lower environments before it will be approved to add to Production. Looking over the codebase and it seems like there is a pretty hard dependency on the New Relic PHP extension being installed, so I assume there is not a way to test whether or not the manual instrumentation option is properly adding the RUM JS unless testing on a server that has the PHP extension installed as well?

@afireintheattic - I was able to get the NR agent running with ddev using the config files here:
https://github.com/ddev/ddev-contrib/pull/103#issuecomment-1605581669

I know you're running lando (with which I'm not familiar) but perhaps that will help you test locally?

There is some interaction between Drupal's internal cache system and newrelic_get_browser_timing_footer() that I can't explain. I'm logged in as user 1. If I load any admin page, the footer JS is consistently rendered. If I visit non-admin pages, the footer JS is only rendered on the first page load after a cache rebuild. I even tried disabling the decorator and acting on the markup directly in an event subscriber, acting on the Symfony ResponseEvent. Stashing the footer JS in the default cache bin seems to do the trick, however.

@chris-burge - I tested this in my local ddev environment as well as in one of our Acquia environments and so far everything seems to be working as expected. The snippets are rendering in the "correct" place (high up in the <head> tag and pretty low in the <body> tag).

I have also confirmed that browser page view timing, errors and session traces are appearing in New Relic as well. I will try to see if I can get some additional eyes on this as well!