Shopify / statsd-instrument

A StatsD client for Ruby apps. Provides metaprogramming methods to inject StatsD instrumentation into your code.
http://shopify.github.io/statsd-instrument
MIT License
570 stars 94 forks source link

Refactor constructing a client based on environment variables #230

Closed wvanbergen closed 4 years ago

wvanbergen commented 4 years ago

Right now, you can instantiate a client based on the environment, or you can instantiate a client from scratch. What is not easy to to instantiate a client that uses most characteristics from the environment, but overrides some settings.

To do this you can either do:

The specific use case I have in mind is that for many apps, we instantiate the client from environment, but want to set a prefix manually based on the name of the application.

To make this easier, I have updated the default values for Client#initialize to not be hardcoded, but coming from the environment instead.

I am not quite sure if this is the right way of doing this, so feel free to disagree with this approach

wvanbergen commented 4 years ago

Alternatively, we could leave new as is with hardcoded defaults, but add an from_env factory method that works like this.

Edouard-chin commented 4 years ago

Alternatively, we could leave new as is with hardcoded defaults, but add an from_env factory method that works like this.

This seems like a better approach to me. I know that most of the time we build a client and need most of the characteristics from the env but for the rare occasions where that's not the case then we'd have the opposite issue with this patch.

Edouard-chin commented 4 years ago

Also, I think we should deprecate usage of clone_with_options and get rid of it in the next major release. As you said its not very easy to discover and its purpose is gonna be useless if we decide to implement the from_env factory

wvanbergen commented 4 years ago

It serves a different use case. It allows you to instantiate new clients that are the same as a different client, except for some options.

E.g., it allows you to instantiate a separate client for a specific purpose. E.g. background processing, for which we always set a BackGroundQueue prefix and the job class name as tag. Other than that, you want it to behave the same as the main client.

The idea would be to do something like this: BackgroundQueue.statsd = StatsD.singleton_client.clone_with_options(prefix: ...). In this case I don't care how the singleton client was configured originally (manually or through environment variables). I just want my cloned client to be configured the same except for my overrides.

This is one of the main advantages of the new client not being a singleton anymore.

clone_with_options might not be the best name though?

Edouard-chin commented 4 years ago

Ah yes ok make sense. I still think having clone_with_options is redundant if we have from_env.

What I propose is this:

class Client
  def self.copy_from(from:, **options)
    case from
    when 'env'
      build(prefix: StatsD::Instrument::Environment.current.statsd_prefix, ...)
    when Client
      build(prefix: options[:prefix] || from.prefix, ...)
    end
  end

  def self.build(options)
    new(**options)
  end
end
wvanbergen commented 4 years ago

I pushed a bunch of updates: