Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix unexpected nil error with Mongo #34

Merged
merged 1 commit into from

4 participants

@raphaelcm

This pull request fixes the following bug:

ruby 1.8.7
rails 3.0.20
mongo 1.4.0
rpm_contrib 2.1.11

> conn = Mongo::Connection.new("localhost")
NoMethodError: You have a nil object when you didn't expect it!
You might have expected an instance of Array.
The error occurred while evaluating nil.[]
    from /var/bundler/turtle/ruby/1.8/gems/rpm_contrib-2.1.11/lib/rpm_contrib/instrumentation/mongo.rb:18:in `instrument'
    from /var/bundler/turtle/ruby/1.8/gems/mongo-1.4.0/lib/../lib/mongo/cursor.rb:463:in `send_initial_query'
    from /var/bundler/turtle/ruby/1.8/gems/mongo-1.4.0/lib/../lib/mongo/cursor.rb:454:in `refresh'
    from /var/bundler/turtle/ruby/1.8/gems/mongo-1.4.0/lib/../lib/mongo/cursor.rb:124:in `next_document'
    from /var/bundler/turtle/ruby/1.8/gems/mongo-1.4.0/lib/../lib/mongo/db.rb:505:in `command'
    from /var/bundler/turtle/ruby/1.8/gems/mongo-1.4.0/lib/../lib/mongo/connection.rb:732:in `check_is_master'
    from /var/bundler/turtle/ruby/1.8/gems/mongo-1.4.0/lib/../lib/mongo/connection.rb:513:in `connect'
    from /var/bundler/turtle/ruby/1.8/gems/mongo-1.4.0/lib/../lib/mongo/connection.rb:688:in `setup'
    from /var/bundler/turtle/ruby/1.8/gems/mongo-1.4.0/lib/../lib/mongo/connection.rb:104:in `initialize'

This happens because the send_initial_query method in the mongo gem (at least version 1.4.0) passes the payload object to the instrument method, even if it is nil.

    def send_initial_query
      message = construct_query_message
      payload = instrument_payload if @logger
      instrument(:find, payload) do
        results, @n_received, @cursor_id = @connection.receive_message(
          Mongo::Constants::OP_QUERY, message, nil, @socket, @command,
          @read_preference, @options & OP_QUERY_EXHAUST != 0)
        @returned += @n_received
        @cache += results
        @query_run = true
        close_cursor_if_query_complete
      end
    end

The instrument_with_newrelic_trace method sets a default value for the parameter, but that only applies if the parameter is missing from the method call. If nil is passed in explicitly, then payload will remain nil, causing the exception when payload[:collection] is attempted.

      def instrument_with_newrelic_trace(name, payload = {}, &blk)
        collection = payload[:collection]
        if collection == '$cmd'
          f = payload[:selector].first
          name, collection = f if f
        end

        trace_execution_scoped("Database/#{collection}/#{name}") do
          t0 = Time.now
          res = instrument_without_newrelic_trace(name, payload, &blk)
          NewRelic::Agent.instance.transaction_sampler.notice_sql(payload.inspect, nil, (Time.now - t0).to_f)
          res
        end
      end

This pull request adds a line to explicitly set payload to its default value ({}) if it is nil.

@jasonrclark
Collaborator

As the README indicates, this project is deprecated so we're not taking pull requests on it.

Our recommendation is to look to other plugins that are actively maintained. In the case of Mongo one does exist at https://github.com/stevebartholomew/newrelic_moped.

Alternatively, you can feel free to use your fork of rpm_contrib. There won't likely be upstream changes to keep up with, which should make that fairly easy.

@jasonrclark jasonrclark closed this
@raphaelcm

As the README indicates, support for Mongoid 3/Moped was spun off to another project.

This issue is with the Mongo Gem, which isn't supported by newrelic_moped. So it would not have been appropriate to submit a pull request there.

At the very least, my pull request now means that this bug is documented, so other people using rpm_contrib with the mongo gem can find out how to fix it.

@jasonrclark
Collaborator

Hey @raphaelcm. Reading over my response yesterday, I rushed things and came off harshly, and I apologize. I neglected to thank you for looking closely at this and crafting a fix. Not everyone takes the time to contribute back, and we appreciate the work you put into it.

I also missed the fact that newrelic_moped doesn't support the Mongo gem, which I should have realized before suggesting it. In light of that, I'm going to reopen and pull this in.

We're in the process of breaking up rpm_contrib into separate gems/libraries that instrument individual projects (e.g. newrelic_mongo), rather than one monolithic project that attempts to instrument them all. We think this will provide a much better experience for our users now that we live in the github era. This is similar to the approach the Resque project takes to plugins. There's some more details in the rpm_contrib project's README.

We're looking for volunteers to work with us and maintain instrumentation libraries specific to a project like the Mongo gem. As a user of the Mongo gem, did you have any interest in helping out with that? We'd gladly link to an extracted gem of the rpm_contrib instrumentation from our side.

Thanks!

@jasonrclark jasonrclark reopened this
@jasonrclark jasonrclark merged commit 7dddc82 into from
@raphaelcm

Thanks for taking a second look, I appreciate it.

I'm far from an expert on the mongo gem (or custom New Relic instrumentation), but we use mongoid2 and the mongo gem in production at my company so it'd be nice to have up-to-date New Relic instrumentation for it. So, yes, I'd be interested in helping out with that. What would be the next steps?

@samg
Owner

@raphaelcm - The next step would be to pull the existing instrumentation into a separate project. You can look at https://github.com/evanphx/newrelic-redis or https://github.com/realestate-com-au/newrelic-sequel or the others listed in the README as an example. Also I'm happy to give any advice or answer questions about how to do that. You can also email me at sam at newrelic.com.

Once you have something that's working we'll update rpm_contrib to link to that project in the README.

Let me know how that sounds and if there's anything we can do to help.

Thanks!

@raphaelcm

Hi @samg thanks. I've pulled out the mongo/mongoid2 instrumentation code into a separate repo: https://github.com/raphaelcm/newrelic_mongo

It's not ready for prime time yet but should be by the end of the weekend. Just wanted to let you know that progress is being made!

Thanks,

Raphael

@benweint
Collaborator

@raphaelcm Awesome! Let us know when you feel like it's ready to be listed in the README and we'll put a link to it there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 7, 2013
  1. @raphaelcm
This page is out of date. Refresh to see the latest.
Showing with 4 additions and 0 deletions.
  1. +3 −0  .gitignore
  2. +1 −0  lib/rpm_contrib/instrumentation/mongo.rb
View
3  .gitignore
@@ -13,6 +13,9 @@ tmtags
## VIM
*.swp
+## RUBYMINE
+.idea/
+
## CTAGS
tags
TAGS
View
1  lib/rpm_contrib/instrumentation/mongo.rb
@@ -15,6 +15,7 @@
include NewRelic::Agent::MethodTracer
def instrument_with_newrelic_trace(name, payload = {}, &blk)
+ payload = {} if payload.nil?
collection = payload[:collection]
if collection == '$cmd'
f = payload[:selector].first
Something went wrong with that request. Please try again.