jkrall / analytical

Gem for managing multiple analytics services in your rails app.
MIT License
381 stars 93 forks source link

Add option for dynamic modules #67

Closed dipth closed 11 years ago

dipth commented 11 years ago

Especially handy in a multi-tenancy setup where each tenant might not want the same modules and keys.

dipth commented 11 years ago

@jkrall do you have time to take a look at this one?

sfsekaran commented 11 years ago

I'm still not decided on whether we should include this or not, but I think if we include this it could auto-detect whether we're passing in custom module configuration by checking if the modules are an array or a hash:

# original
analytical :modules => [:google]
# augmented
analytical :modules => { google: {...}, kissmetrics: {...} }
# or in your case:
analytical :modules => lambda{ |controller| controller.analytical_modules }

The point is I'd rather not add a separate configuration key for this if possible.

dipth commented 11 years ago

@sfsekaran you mean something like this? 1122d2e4c08a22a49b8fa775da3309167cbd0ae0

sfsekaran commented 11 years ago

Yes, much like that :+1:

I'd like to get another maintainer's opinion before merging. @nirvdrum or @freerobby, any problems with this non-intrusive feature?

nirvdrum commented 11 years ago

Overall, I really like this. I don't want to bikeshed, but it might be nice if the Proc variant had a way to fallback to whatever the config file has. Maybe if its yielded value is nil, you treat it as a no-op. E.g., it could be used to override config for just certain controllers or maybe just certain environments. But that could be done later as well.

dipth commented 11 years ago

@nirvdrum I like your idea but it might be a bit tricky to implement without changing the API or adding new config-keys.

The problem is that if we pass a modules-option to the call to analytical, the modules loaded from the yml-file never actually gets stored anywhere.

One way to do it would be like this:

  def analytical(options={})
    self.analytical_options = options

    config_options = { :modules => [], :file_modules => [] }
    File.open("#{::Rails.root}/config/analytical.yml") do |f|
      file_options = YAML::load(ERB.new(f.read).result).symbolize_keys
      env = (::Rails.env || :production).to_sym
      file_options = file_options[env] if file_options.has_key?(env)
      file_options.each do |k, v|
        if v.respond_to?(:symbolize_keys)
          # module configuration
          config_options[k.to_sym] = v.symbolize_keys
          config_options[:modules] << k.to_sym unless options && options[:modules]
          config_options[:file_modules] << k.to_sym
        else
          # regular option
          config_options[k.to_sym] = v
        end
      end if file_options
    end if File.exists?("#{::Rails.root}/config/analytical.yml")

    self.analytical_options = self.analytical_options.reverse_merge config_options
  end

Which makes sure that we can later see what modules were originally loaded from the yml-file, but then we are back at the original problem with introducing another key in the configuration hash

sfsekaran commented 11 years ago

@nirvdrum, good point, and thanks @dipth for elaborating. Based on this, I'm going to merge this request and we can investigate good fallback support afterward. :hammer: