Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/uri_parsing #22

Merged
merged 5 commits into from
Sep 30, 2014
Merged

Feature/uri_parsing #22

merged 5 commits into from
Sep 30, 2014

Conversation

arronmabrey
Copy link
Contributor

@dwbutler This PR impliments uri_config parsing as discussed here #19

@dwbutler
Copy link
Owner

Thanks, looks great!

There are a few things I would modify.

  1. I think the URI should be passed in as an options hash like any other option, i.e. uri: 'unix://tmp/socket'
  2. I think it would be good to automatically read in configuration from a LOGSTASH_URI environment variable, if present.

@arronmabrey
Copy link
Contributor Author

@dwbutler Okay great, I can do those updates.

@arronmabrey
Copy link
Contributor Author

I made the update from accepting the raw url "udp://localhost:5228" to a uri parameter {uri: "udp://localhost:5228"}

I had one question about the ENV['LOGSTASH_URI']. What should have higher precedence?
e.g.

uri = ENV['LOGSTASH_URI'] || opts[:uri]

or

uri = opts[:uri] || ENV['LOGSTASH_URI']

@dwbutler
Copy link
Owner

Hm... I think I would expect options passed in to take precedence over the environment. (option number 2)

@arronmabrey
Copy link
Contributor Author

I think I actually prefer not having the environment variable hardcoded into the gem. I would rather use the new uri_parsing feature like this instead.

LogStashLogger::Device.new(uri: ENV['LOGSTASH_URI'])

This way the user has the flexibility to determine their own fallback policy.

For example in our production environment we want ENV['LOGSTASH_URI'] to take precedence.
Additionally, as side note, we already have a differently named environment variable ENV['SMCC_LOGGING_URL'] that would require each server to be updated.

All this is leading me to believe, it's better left up to the user to define the needed behavior.

This uri_parsing feature handles all heavy lifting and allows them to just simply pass an environment URL directly into the constructor, which I think is the big win here.

-- Arron

@dwbutler
Copy link
Owner

That makes sense. Only the user really knows what environment variable they want to store configuration in. Besides, you might have multiple logstash configurations defined.

@dwbutler
Copy link
Owner

One more thing - if the URI is invalid, it should probably just raise the exception so the problem is easier to notice and fix. I would be confused if my URI configuration were silently ignored and the default device were configured instead.

@arronmabrey
Copy link
Contributor Author

@dwbutler Yah, I think you're right. PR is updated with that change.

@dwbutler
Copy link
Owner

Awesome, thanks!

dwbutler added a commit that referenced this pull request Sep 30, 2014
@dwbutler dwbutler merged commit ea25fe0 into dwbutler:master Sep 30, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants