- Issue created by @Chris Burge
- Status changed to Needs review
9 months ago 6:37pm 29 April 2024 - @chris-burge opened merge request.
- Status changed to Needs work
8 months ago 7:39pm 8 May 2024 - 🇺🇸United States Chris Burge
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 fieldContent-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 arum_instrumentation
settings with three options:disabled
,auto
, andmanual
. - Status changed to Needs review
8 months ago 11:25pm 8 May 2024 - 🇺🇸United States Erik_MC
Interested in having this fix ready for a client site! Do you think it may be available later this month?
- 🇺🇸United States Chris Burge
@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.
- Status changed to RTBC
8 months ago 6:05pm 4 June 2024 - 🇮🇳India vipin.mittal18 Greater Noida
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.
- 🇺🇸United States Chris Burge
@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 inserted10.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 - 🇮🇳India vipin.mittal18 Greater Noida
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.
- 🇺🇸United States neclimdul Houston, TX
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. - 🇺🇸United States Chris Burge
@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()
andnewrelic_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+. - 🇺🇸United States Chris Burge
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 ofnewrelic_get_browser_timing_footer()
.- 🇺🇸United States Chris Burge
@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 toscripts_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()
viadrupalSettings
to a JS file, and then pass that value intoeval()
: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 thehead
tag, which is contradictory. It does render as the first child element of thebody
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.- Status changed to Needs work
7 months ago 9:00pm 12 June 2024 - 🇺🇸United States Chris Burge
@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. - Status changed to Needs review
7 months ago 11:10pm 12 June 2024 - 🇺🇸United States Chris Burge
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
- 🇿🇦South Africa pieterdtt
@jack.minster @chris-burge
Patch for making head injection to be configurable.
- Assigned to Chris Burge
- Status changed to Needs work
7 months ago 12:54pm 13 June 2024 - 🇺🇸United States Chris Burge
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.- 🇺🇸United States Chris Burge
@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?
- Status changed to Needs review
7 months ago 9:03pm 13 June 2024 - 🇺🇸United States Chris Burge
@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. InHtmlResponseAttachmentsProcessorDecorator
, the MR is now modifying the::processAssetLibraries()
method to insert the JS intoscripts_bottom
, bypassing the#attached
types restrictions of::processAttachments()
. - Issue was unassigned.
- 🇺🇸United States Chris Burge
@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). - 🇺🇸United States neclimdul Houston, TX
I'm not sure I understand why we're not using standard methods for attaching things to the footer.
- 🇺🇸United States Chris Burge
@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 ofnewrelic_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 withHtmlResponseAttachmentsProcessorDecorator::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.
- 🇺🇸United States Chris Burge
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.
- 🇺🇸United States Chris Burge
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. - Assigned to Chris Burge
- Status changed to Needs work
7 months ago 7:35pm 20 June 2024 - 🇺🇸United States Chris Burge
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 thatnewrelic_get_browser_timing_footer()
is getting called multiple times during the same request. If eithernewrelic_get_browser_timing_header()
ornewrelic_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()
andExtensionAdapter::newrelic_get_browser_timing_footer()
to leveragedrupal_static()
. - Issue was unassigned.
- Status changed to Needs review
7 months ago 3:00pm 24 June 2024 - 🇺🇸United States Chris Burge
When big_pipe is enabled,
HtmlResponseAttachmentsProcessor->processAttachments()
gets called several times, which meansnewrelic_get_browser_timing_footer()
gets called multiple times, which means all calls after the initial call return an empty string. The latest MR code usesdrupal_static()
to cache the return value ofnewrelic_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 innewrelic_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. - 🇺🇸United States afireintheattic
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?
- 🇺🇸United States Chris Burge
@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'.
- 🇺🇸United States afireintheattic
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-1605581669I know you're running lando (with which I'm not familiar) but perhaps that will help you test locally?
- 🇺🇸United States Chris Burge
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 SymfonyResponseEvent
. 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!
@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!
- Status changed to Needs work
7 months ago 2:03pm 5 July 2024 - 🇺🇸United States phenaproxima Massachusetts
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.
- 🇺🇸United States Chris Burge
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 📌 Switch from Drupal CI to GitLab CI Needs review before merging.
- 🇺🇸United States Chris Burge
Tests are passing again - now to address the rest of the technical review.
- Status changed to Needs review
6 months ago 1:56am 8 July 2024 - 🇺🇸United States Chris Burge
@phenaproxima - Thanks for the code review
- 🇺🇸United States Chris Burge
@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... - 🇮🇳India kamleshpatidar
Added patch https://git.drupalcode.org/project/new_relic_rpm/-/merge_requests/5 on Drupal 10.2 website. it is working fine.
- 🇺🇸United States Chris Burge
Note: We may be able to get rid of the
HtmlResponseAttachmentsProcessorDecorator
code when ✨ Allow additional keys in #attached Needs review lands in core.