🇺🇸United States @Chris Burge

Account created on 22 February 2012, over 12 years ago
  • Technical Account Manager at Acquia 
#

Merge Requests

More

Recent comments

🇺🇸United States Chris Burge

I'm running into the same notice in the issue description, and it's filling the logs every request. I can't explain it, but patch #4 fixes it. No more notices.

🇺🇸United States Chris Burge

I missed this issue when opening Allow for RUM Manual Instrumentation Needs review , which implements this feature request in MR-5.

🇺🇸United States Chris Burge

@godotislate - I like the approach of caching attachments in a class property. One thing I noticed when stepping through the method with a debugger is that editor_load() still gets called every loop if the text format doesn't have an editor assigned to it.

I just pushed a commit to the MR that add an $editors property to the class and checks if the text format has been processed yet instead of checking if $settings['editor']['formats'][$format_id] is set. This ensures editor_load() is only called once for each text format.

I'm not sure what kind of text coverage could be added for this MR. We can't test performance with unit tests, and the existing test coverage should ensure we haven't broken anything.

🇺🇸United States Chris Burge

Provider docs: https://support.bluebillywig.com/advanced-topics/oembed-service/
oEmbed specs: https://oembed.com/

https://*.bbvms.com/p/*/c/*.html?inheritDimensions=true&placementOption=default appears to be a valid resource URL.

The provider endpoint URL, as specified by bbvms.com is https://<publication>.bbvms.com/oembed/; however, the endpoint URL cannot contain wildcards. I'm not sure how you'd do this at runtime off the top of my head. You might need to decorate the media.oembed.resource_fetcher service with custom code and modify the ResourceFetcher::fetchResource() method to modify the URL before the resource is fetched.

🇺🇸United States Chris Burge

@sea2709 - That's exactly the issue. We can't change Html::cleanCssIdentifier without causing CSS bugs.

#13

I'm wondering if the best way to solve this issue is to use a configuration flag/setting similar to the $conf['allow_css_double_underscores'] setting in D7 that allowed BEM double underscore CSS selectors.

I think the behavior should be configurable. Maybe a $setting to define additional characters?

🇺🇸United States Chris Burge

@erwangel - This was fixed in 🐛 Calling static trait method is deprecated (redux) Fixed but hasn't been released yet.

🇺🇸United States Chris Burge

@senzaesclusiva - To clarify - Is the issue resolved for you?

🇺🇸United States Chris Burge

I'm not seeing any changes in 2.0.0 that explains the exception. We do have two prior reports of issues with Rumble.com; however. Please see #3253757-14: Unable to create the remote video .

🇺🇸United States Chris Burge

@justcaldwell - Thanks for opening this issue!

🇺🇸United States Chris Burge

On second thought, we can just change the trait to a class.

🇺🇸United States Chris Burge

It looks like we'll need to remove HelperTrait and just define a procedural function in the module file.

🇺🇸United States Chris Burge

🐛 Allow form redirects to ignore ?destination query parameter Fixed was committed and released in D10.2.0: https://www.drupal.org/node/3375113

Here's the code I used to attempt to implement the fix:

function layout_builder_operation_link_form_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) {
  if ($form_state->getFormObject() instanceof \Drupal\layout_builder\Form\OverridesEntityForm) {
    $form_state->setIgnoreDestination();
  }
}

I confirmed with my IDE that ->setIgnoreDestination() is firing; however, the destination query parameter is still being rendered. I don't believe the "fix" addresses the use case for operation links. I think we might just need to update the comment in layout_builder_operation_link_preprocess_links() to remove the reference to the core issue we were waiting on.

🇺🇸United States Chris Burge

D11 support was already added in 📌 Add Drupal 11 Support Fixed - it just needs released.

🇺🇸United States Chris Burge

Manual testing was successful. Logs are clean.

🇺🇸United States Chris Burge

@aaron.ferris - Thanks for jumping in and providing a solution!

🇺🇸United States Chris Burge

Access checks are only necessary on content entities, not on config entities: https://www.drupal.org/node/3201242 .

🇺🇸United States Chris Burge

When I asked around among colleagues today, I was pointed to this blog post: https://dev.acquia.com/blog/drupalelementstyle-add-styles-drupal-media-c....

@bkosborne - Is this solution functionally equivalent to the plugin you authored?

🇺🇸United States Chris Burge

This makes sense. I have a customer who leverages paragraphs for their site, often with a dozen or more rich text fields on given node edit page. Page load times are quite slow in these instances.

The ::getAttachments() method belongs to the Drupal\editor\Plugin\EditorManager class, so moving this issue to editor.module.

Static caching seems reasonable. &drupal_static() can be used. Editors and text formats have a 1:1 relationship. Text formats have permissions and control access their editor, so I don't see any permission issue here with caching.

We'll want to make sure to preserve the behavior of hook_editor_js_settings_alter() (i.e. $this->moduleHandler->alter('editor_js_settings', $settings); and below should not be cached.

🇺🇸United States Chris Burge

Needed to run tests. Maintainers should feel free to remove .gitlab-ci.yml file prior to merging and merge 📌 Switch from Drupal CI to GitLab CI Needs review separately.

🇺🇸United States Chris Burge

@_m - Code is merged! Thanks

🇺🇸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...

🇺🇸United States Chris Burge

@phenaproxima - Thanks for the code review

🇺🇸United States Chris Burge

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

🇺🇸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

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.

🇺🇸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 Chris Burge

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.

🇺🇸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 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().

🇺🇸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.

🇺🇸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

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

🇺🇸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 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. In HtmlResponseAttachmentsProcessorDecorator, the MR is now modifying the ::processAssetLibraries() method to insert the JS into scripts_bottom, bypassing the #attached types restrictions of ::processAttachments().

🇺🇸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?

🇺🇸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.

🇺🇸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
🇺🇸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.

🇺🇸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 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.

🇺🇸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+.

🇺🇸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() 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+.

🇺🇸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 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

🇺🇸United States Chris Burge

Is Features configured to export permissions with roles? It sounds like that is the issue.

🇺🇸United States Chris Burge

@Shubham_Kumar - Thanks for testing the module out!

🇺🇸United States Chris Burge

Chris Burge made their first commit to this issue’s fork.

🇺🇸United States Chris Burge

It sounds like the behavior only presented on existing blocks, which would be the referenced issue; however, the fact that clearing cache and trying a different browser didn't work isn't consistent with prior reports. This module uses core APIs to render the links, so, if there is an issue, it would most probably be a core issue.

🇺🇸United States Chris Burge

I'm wondering if this issue might be the same as #3193067: Manage attributes link not displaying for existing blocks . @eit2103, can you try the same steps in a fresh browser session (e.g. incognito mode)?

🇺🇸United States Chris Burge

Can you help me understand what you mean by "standard layout builder pages" and by editing "the layout of individual content items"? A screenshot of the latter might be helpful.

🇺🇸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.

🇺🇸United States Chris Burge

Unit tests are passing. The cspell, phpcs, and phpstan failures can be deferred.

🇺🇸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 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.

🇺🇸United States Chris Burge

I just added config_split as a test dependency to the merge request.

🇺🇸United States Chris Burge

Based on this module's README, it appears that config_split is optional:

When using config_split it allows you to dynamically enable and import
configuration from a config split for each of your custom installation
profiles.

Production build 0.71.5 2024