Problem/Motivation

This issue is a follow up to #1415860: Add manual instrumentation of RUM?.

Beginning in Drupal 10.2.0, Drupal now sets a content-length header for most responses.

This breaks New Relic's auto-instrumentation:

Auto-instrumentation does not work when the HTTP header field Content-Length is set. To use browser monitoring in this situation, disable auto-instrumentation and manually insert the JavaScript header and footer into your templates.

Per Manually instrument via agent API , newrelic_get_browser_timing_header() and
newrelic_get_browser_timing_footer() need inserted into the header and footer, respectively.

Proposed resolution

  1. Replace the 'disable_autorum' setting with a 'rum_instrumentation' setting with three options: 'Disable browser monitoring' (disabled), 'Allow auto-instrumentation' (auto), and 'Manual instrumentation' (manual).
  2. Conditional upon the setting, insert New Relic markup.

Remaining tasks

  • Merge request
  • Testing/review

User interface changes

Replace the 'Disable AutoRUM' checkbox with a 'RUM instrumentation' select element on the 'new_relic_rpm.settings' form.

API changes

None.

Data model changes

'disable_autorum' setting is replaced by a 'rum_instrumentation' setting for 'new_relic_rpm.settings'.
Config change is handled by new_relic_rpm_post_update_instrumentation().

CommentFileSizeAuthor
#21 interdiff_1-15.txt3.62 KBpieterdtt
#21 new_relic_rpm-manual-instrumentation-3444218-15.patch23.36 KBpieterdtt
#14 3444218_screenshot.png47.62 KBchris burge
#9 Drupal 10+ After MR New Relic.mp419.33 MBvipin.mittal18
#9 Drupal 10+ New Relic not working.mov16.44 MBvipin.mittal18

Issue fork new_relic_rpm-3444218

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Chris Burge created an issue. See original summary.

chris burge’s picture

chris burge commented
Status: Active » Needs review

Chris Burge opened merge request !5

chris burge’s picture

chris burge commented
Status: Needs review » Needs work

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.

chris burge’s picture

chris burge commented
Status: Needs work » Needs review
chris burge’s picture

chris burge commented
Issue summary: View changes
erik_mc’s picture

erik_mc commented

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

chris burge’s picture

chris burge commented

@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.

vipin.mittal18’s picture

vipin.mittal18
He/Him
Primary language English
Location Greater Noida, India
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Drupal 10+ New Relic not working.mov16.44 MB
new Drupal 10+ After MR New Relic.mp419.33 MB

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

chris burge’s picture

chris burge commented

@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

Another way of thinking about "Allow auto-instrumentation" is "do nothing".

vipin.mittal18’s picture

vipin.mittal18
He/Him
Primary language English
Location Greater Noida, India
commented

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.

neclimdul’s picture

neclimdul
He/Him
commented

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.

chris burge’s picture

chris burge commented

@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+.

chris burge’s picture

chris burge commented
StatusFileSize
new 3444218_screenshot.png47.62 KB

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

Incompatible configuration detected Auto-instrumentation is incompatible with Drupal 10.2.0+ due to the Content-Length header.

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

jack.minster’s picture

jack.minster commented

Thank you for this patch!

We are actively using it 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 hook_page_attachments() 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().

chris burge’s picture

chris burge commented

@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>. When I look at rendered page output, I'm seeing it almost after the <body> tag. Is it appearing farther down in your application's markup?

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.

jack.minster’s picture

jack.minster commented

@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.

chris burge’s picture

chris burge commented
Status: Reviewed & tested by the community » Needs work

@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.

chris burge’s picture

chris burge commented
Status: Needs work » Needs review
chris burge’s picture

chris burge commented

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
pieterdtt’s picture

pieterdtt commented
StatusFileSize
new new_relic_rpm-manual-instrumentation-3444218-15.patch23.36 KB
new interdiff_1-15.txt3.62 KB

@jack.minster @chris-burge

Patch for making head injection configurable.

chris burge’s picture

chris burge commented
Assigned: Unassigned » chris burge
Status: Needs review » Needs work

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.

jack.minster’s picture

jack.minster commented

@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.

chris burge’s picture

chris burge commented

@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?

chris burge’s picture

chris burge commented
Status: Needs work » Needs review

@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().

chris burge’s picture

chris burge commented
Assigned: chris burge » Unassigned

@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).

neclimdul’s picture

neclimdul
He/Him
commented

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

chris burge’s picture

chris burge commented

@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 service, as well.

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

chris burge’s picture

chris burge commented

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.

chris burge’s picture

chris burge commented

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.

chris burge’s picture

chris burge commented
Assigned: Unassigned » chris burge
Status: Needs review » Needs work

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::getBrowserTimingHeader() and ExtensionAdapter::getBrowserTimingFooter() to leverage drupal_static().

chris burge’s picture

chris burge commented
Assigned: chris burge » Unassigned
Status: Needs work » Needs review

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.

afireintheattic’s picture

afireintheattic commented

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?

chris burge’s picture

chris burge commented

@afireintheattic - 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'.

afireintheattic’s picture

afireintheattic commented

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?

jack.minster’s picture

jack.minster commented

@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?

chris burge’s picture

chris burge commented

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.

jack.minster’s picture

jack.minster commented

@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!

jack.minster’s picture

jack.minster commented

@chris-burge - one of our performance engineers reviewed the code snippets in our staging environment and confirmed that everything looks good! If your MR doesn't get any traction in the next several days, I'll be applying the lastest version of it as a patch to our code base so it can go to production. Thanks for all your work!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work

I don't have any domain knowledge here but I gave this a code review and there are some things I think could be tightened up, and one outright bug-creating condition that should be fixed before this is merged.

chris burge’s picture

chris burge commented

There's too much going on in this MR to not be running tests. I added .gitlab-ci.yml. The maintainers should feel free to remove the file and commit separately in #3446404: Switch from Drupal CI to GitLab CI before merging.

chris burge’s picture

chris burge commented

Tests are passing again - now to address the rest of the technical review.

chris burge’s picture

chris burge commented
Status: Needs work » Needs review

@phenaproxima - Thanks for the code review

chris burge’s picture

chris burge commented

@neclimdul - Sorry for flooding your inbox, but I think we're really close on this MR now.

In terms of outstanding items, there's been discussion around how to describe the settings' behaviors:
https://git.drupalcode.org/project/new_relic_rpm/-/merge_requests/5#note...

I'm open to revising documentation and relabeling. It strikes me the machine name 'auto' could be better named 'no-action'.

There was also discussion regarding the UpdateTest test class:
https://git.drupalcode.org/project/new_relic_rpm/-/merge_requests/5#note...

kamleshpatidar’s picture

kamleshpatidar commented

Added patch https://git.drupalcode.org/project/new_relic_rpm/-/merge_requests/5 on Drupal 10.2 website. it is working fine.

chris burge’s picture

chris burge commented

Note: We may be able to get rid of the HtmlResponseAttachmentsProcessorDecorator code when #3448503: Allow modules to use #attached for additional data lands in core.

chris burge’s picture

chris burge commented

On the latest commit, I set the decoration_priority to -100 so that our new_relic_rpm.html_response.attachments_processor decorator gets called earlier.

There are a handful of contrib modules that decorate core's html_response.attachments_processor service. In the case of Acquia's Site Studio module, the module decorates the core service but then breaks the decorator pattern in its implementation. As a result, no decorators with a higher weight are called. (While the current decoration_priority is 100, it was defaulted to 0 in versions earlier than 8.x-8.0.2 of the Site Studio module.)

almunnings’s picture

almunnings
Location Melbourne, 🇦🇺
commented

Modules that also implement changes to the AttachmentsResponse can result in the error:

Drupal\new_relic_rpm\Render\HtmlResponseAttachmentsProcessorDecorator::__construct(): Argument #1 ($decorated) must be of type Drupal\Core\Render\HtmlResponseAttachmentsProcessor, Drupal\webprofiler\Render\HtmlResponseAttachmentsProcessor given, called in /app/web/core/lib/Drupal/Component/DependencyInjection/Container.php on line 261

Steps to reproduce

Install the webprofiler module.

Proposed resolution

/src/Render/HtmlResponseAttachmentsProcessorDecorator.php

Change: private readonly HtmlResponseAttachmentsProcessor $decorated,
To: private readonly AttachmentsResponseProcessorInterface $decorated,

chris burge’s picture

chris burge commented

@almunnings - That's a good catch. I updated the MR.

niral098’s picture

niral098
Location NJ, United States
commented

Thanks for the work so far!
I'm testing the MR as a patch on Drupal version 10.4.6, but it's not working as expected. The script isn't being applied to the footer at all, although it does appear in the header on a few pages. Additionally, I'm seeing the following JavaScript error in the browser console:
"New Relic Warning: https://github.com/newrelic/newrelic-browser-agent/blob/main/docs/warnin..."
Still investigating what's going on.

programeta’s picture

programeta commented

Thanks for the work so far!

I'm testing the MR as patch on Drupal 11.1.6, but it's not working as expected.

UPDATED: Sorry, I had a bad configuration API Key. It's working properly

chris burge’s picture

chris burge commented

@niral098 - The header JS is inserted with hook_page_attachments(), so I'd recommend looking for other implementations of this hook that may be overwriting the value of html_head. Re the footer JS, that's inserted by decorating the html_response.attachments_processor service, and this has been problematic in some instances. I'd recommend looking for other services decorating the html_response.attachments_processor service.

@programeta - "it's not working as expected" - What's not working? Is the header JS not being inserted consistently? Is the footer JS not being inserted consistently? Are you seeing errors in the console? We need to know what's not working in order to consider your feedback.

almunnings’s picture

almunnings
Location Melbourne, 🇦🇺
commented

Howdy!
This MR is no longer applicable to 2.x-dev, and might need a tweak for Drupal 11 support.

Should we branch for 2.2.x support, or just continue in the same MR?
I'm happy to help out, just don't want to step on toes! :D

joelsteidl’s picture

joelsteidl commented

It seems like this functionality was included as part of the 2.2 release?

chris burge’s picture

chris burge commented

The 2.2.0 release only included two commits, one for GitLab CI/CD and one for D11 support: https://git.drupalcode.org/project/new_relic_rpm/-/commits/2.2.0?ref_typ....

joelsteidl’s picture

joelsteidl commented

Got it. So this issue is still applicable and a re-roll would be needed based on the commits that made it into 2.2?

chris burge’s picture

chris burge commented

@joelsteidl - I'm seeing the MR is mergeable, so a rebase shouldn't be required.

joelsteidl’s picture

joelsteidl commented

@chris burge

Hopefully I don't get in trouble, but I rebased the MR branch with 2.x and that made it so the patch can be applied to 2.2.

I think that's probably want we want since a couple specific commits were made to 2.x.

hfernandes changed the visibility of the branch 3444218-allow-for-rum to hidden.

hfernandes changed the visibility of the branch 3444218-allow-for-rum to active.

hfernandes’s picture

hfernandes commented

Hello,

I identified an issue in the code when using the "Ignore roles" option in the New Relic configuration.

Steps to reproduce

  1. Go to the New Relic configuration and select Administrator in the Ignore roles field.
  2. Save the form and clear caches.
  3. Access the website with an Administrator role.
  4. Access the website as an anonymous user.

Expected result
Anonymous users should be tracked correctly.

Actual result
Anonymous users are not tracked correctly.

The function getBrowserTimingFooter() caches the response of newrelic_get_browser_timing_footer().
When accessed with an Administrator user, this function returns an empty string. That empty string is then cached and reused for anonymous users, causing the tracking issue.

Implemented fix
Update the code to include a method to verify if the transaction is ignored and return NULL on getBrowserTimingFooter() in that case.

edsoncarlos’s picture

edsoncarlos commented

Hi @hfernandes , I found this issue related to the cache name ID:

src/ExtensionAdapter/ExtensionAdapter.php

$key = 'new_relic_rpm:rum_footer_js';
$cache = \Drupal::cache();
$cached_js = $cache->get($key);

It seems that we need to concatenate the transaction name to the $key. Currently, with the way the $key is formed, all the transaction names on my New Relic dashboard appear the same.

hfernandes’s picture

hfernandes commented

Hi @edsoncarlos, when you say "transaction name", do you mean that in the Page views section of the Browser dashboard you only see a single page being reported? If so, that's not happening on my end. I think the script returned by newrelic_get_browser_timing_footer is always the same and that's why it's being cached - but I might be wrong.

I'm using the latest version of the patch I created.

rduterte’s picture

rduterte
Primary language Tagalog
Location PH
commented

Hi all,

I’m adding this comment here as others may encounter the same issue.

I tested the patch
new_relic_rpm-manual-instrumentation-3444218-15.patch
with our current setup: Drupal Core 10.4.8 and PHP 8.3.

I set up my own New Relic PHP agent in my local Drupal project using DDEV and configured Browser Real User Monitoring (RUM) to Manual Instrumentation, as this is the change introduced by the patch.

After doing this, the site breaks with the following error:

Uncaught PHP Exception TypeError: "Unsupported operand types: string * int" at /mnt/www/html/cambunipressnouwptest/docroot/core/lib/Drupal/Core/Render/Element.php line 92 request_id="v-6ca773c4-f03f-11f0-9a0c-1bbf72ef8025"

This issue is caused by changes in the hook
hook_new_relic_rpm_page_attachments, specifically this line:

$weight = \Drupal::config('new_relic_rpm.settings')->get('head_injection_weight');

The value of this variable is a string, as shown by the following command and output:

drush php:eval "var_dump(\Drupal::config('new_relic_rpm.settings')->get('head_injection_weight'));"
Output: string(0) ""

In PHP 8+, arithmetic operations such as floor($weight * 1000) on a string throw a:

TypeError: Unsupported operand types: string * int

Reference:
https://www.php.net/manual/en/migration80.incompatible.php

This behavior was introduced in PHP 8.0+, where arithmetic operators consistently throw a TypeError for non-numeric operands.

However, the MR patch
https://git.drupalcode.org/project/new_relic_rpm/-/merge_requests/5.patch

is working as expected so far, and we have not encountered any issues with it.