My module manually instruments New Relic Real User Monitoring. I created it for my employer because we have an older version of the New Relic agent installed on our server, and it's not properly injecting its JavaScript anymore and causing JS errors. They don't want to update the agent yet, so we're going to manually instrument it instead of relying on its automatic injection. New Relic has instructions for doing this in Drupal, so I take it other people have had the same problem. Unfortunately, those instructions show you how to add their JavaScript to the Garland page.tpl.php, which obviously isn't the Drupal way to do it. So I thought I'd try my hand at contributing a module for it. My thought was to create a 1.x release that just does what my employer needs (but in an extensible way) and then see what the community asks for in later versions. A brief search doesn't turn up any similar modules.
The module is for Drupal 6.
| Comment | File | Size | Author |
|---|---|---|---|
| #1 | pareview-new-relic.txt | 1.72 KB | andrewyager |
Comments
Comment #1
andrewyager commentedIronically, I was looking for something that did this earlier this week. I would recommend putting some more information about the module (e.g. what integration this module provides). It might be worth having a two sentence overview of what New Relic is, as well as a link to their site.
I've done a quick manual review of the code, and I would recommend a few things:
You already do this in your module file.
I've run the automated review against your module, and it's picked up some issues.
It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:
This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Go and review some other project applications, so we can get back to yours sooner.
Comment #2
traviscarden commentedThank you for the helpful review, @andrewyager! I was unaware of PAReview.sh, which very nice! I have made the proposed changes and then a few, and I'm ready for further review.
Comment #3
afox commentedHi @TravisCarden,
I went through your module. Here's some notes:
.info -file
configure = admin/settings/newrelicapiis D7 only..admin.inc -file
You're using the t()-function incorrectly when you use links.
Instead of
use this
You should test my code above first. Wrote it from memory.
For more info on t()-function usage, see http://api.drupal.org/api/drupal/includes%21common.inc/function/t/6
.module -file
Using hook_init() is not the best choice here, as it is completely passed when caching is enabled.
At first I thought it was ok, since you're merely adding JS, which is ok to do here. But I noticed you're using newrelic_get_browser_timing_header & newrelic_get_browser_timing_footer which should be run on every page load if I've understood correctly.
So, I suggest using hook_boot instead of hook_init to make sure it loads every time, even with cache enabled.
See API: http://api.drupal.org/api/drupal/developer%21hooks%21core.php/function/h...
Other notes
There's already http://drupal.org/project/new_relic_rpm as you've probably noticed. So why should there exist two New Relic modules? What's the difference, should they merge?
Comment #4
traviscarden commentedI'm abandoning this for the time being. Thanks, @andrewyager and @afox, for the instructive reviews.
Comment #5
patrickd commentedI'm very sorry about the delay! :(
Feel free to come back later if you like.
Comment #5.0
patrickd commentedUpdated issue summary.