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

DEFAULT_TIMEOUT is not passed to Faraday connection as an option #84

Open
SeanSmithers opened this issue Dec 16, 2020 · 5 comments
Open

Comments

@SeanSmithers
Copy link

When HTTParty was replaced with Faraday (in #55) some HTTParty options were converted to constants (https://github.com/decision-labs/fcm/pull/55/files#diff-0068abed920b95db498a1c507ae5a10f86145599f5dd2431b36ccbf492d7fe7dL6-L10)

DEFAULT_TIMEOUT and FORMAT are now unused.

It would be great to pass DEFAULT_TIMEOUT as an option to Faraday, example:

  def for_uri(uri, extra_headers = {})
    connection = ::Faraday.new(:url => uri) do |faraday|
      faraday.adapter  Faraday.default_adapter
      faraday.options["timeout"] = DEFAULT_TIMEOUT
      faraday.headers["Content-Type"] = "application/json"
      faraday.headers["Authorization"] = "key=#{api_key}"
      extra_headers.each do |key, value|
        faraday.headers[key] = value
      end
    end
    yield connection
  end

A bonus would be to allow some way for consumers of FCM to be able to pass options like this in when initializing and have them passed to the underlying HTTP library.

ghiculescu added a commit to ghiculescu/fcm that referenced this issue Dec 23, 2020
Since decision-labs#55 you can't pass client options anymore (well you can but they aren't used for anything).

This results in decision-labs#84 which I'm +1 on, but while it's not resolved, better to not imply anything in the docs.
@sabman
Copy link
Member

sabman commented Jun 3, 2021

I am thinking of adding support for .env and/or ENV variables to overwrite the defaults. Thoughts?

@SeanSmithers
Copy link
Author

@sabman support for .env and/or ENV variables could help here, yes.

Would these variables allow a consumer to overwrite just the timeout option or would it be possible to overwrite a number of different options on the underlying HTTP library?, e.g. open_timeout as well (but perhaps even things like request/response logging)

@sabman
Copy link
Member

sabman commented Jun 17, 2021

Yes we can probably add other items too. Only thing I'd wanna ensure is avoid accidental name clashes. So maybe namespacing the environment variables with FCM_VAR_XXX might be the way to go.

@jensljungblad
Copy link
Contributor

@sabman Could you do something similar to the elasticsearch gem?

Elasticsearch::Client.new(
  host: ENV.fetch("ELASTICSEARCH_HOST", nil),
  user: ENV.fetch("ELASTICSEARCH_USER", nil),
  password: ENV.fetch("ELASTICSEARCH_PASSWORD", nil),
  request_timeout: 10
) do |faraday|
  faraday.request(:instrumentation, name: "request.elasticsearch")
end

Giving access to the faraday object allows you to attach own instrumentation etc.

@Linuus
Copy link

Linuus commented Feb 22, 2022

The issue with DEFAULT_TIMEOUT not being used has been addressed in: #96

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants