fetch_timeout_millis does not appear to be respected for Remote Evaluation

[therabidbanana]

Reviewing traces in Datadog for HTTP requests sent by the Ruby client I can see several requests logged at 1+ second even though I've set fetch_timeout_millis to 500

Expected Behavior

config = AmplitudeExperiment::RemoteEvaluationConfig.new(fetch_timeout_millis: 500)
experiment = AmplitudeExperiment.initialize_remote(experiment_key, config)
variants = experiment.fetch(...) # expected to take <=500 milliseconds

Current Behavior

variants = experiment.fetch(...) # occasionally takes 1000+ milliseconds

Possible Solution

Either fractional seconds are not allowed, documentation is incorrect for how to set this, or we need to add additional configuration for open_timeout to the persistent http client.

https://ruby-doc.org/stdlib-2.4.1/libdoc/net/http/rdoc/Net/HTTP.html#open_timeout-attribute-method

Steps to Reproduce

Hard to reproduce as it requires a slow connection to occur - might be able to fake it with network connection settings changed to simulate spotty 3G or something?

Environment

• SDK Version: 1.1.1
• Ruby Version: 3.1.3

[bgiori]

Hi @therabidbanana, thanks for reporting this issue. I should be able to get to this very soon.

[bgiori]

@therabidbanana I think this is due to the connection timeout having a different configuration. E.g. we're only currently configuring the read_timeout

Would your expectation be that the configuration variable also set the connection timeout? Or would a separate configuration option be preferred?

Thanks again for submitting this issue, sorry for the delay!

[therabidbanana]

That's tough - I'm not sure what makes the most sense. I'd probably allow options to configure each part because it's easiest rather than trying to programmatically determine which percentage of fetch_timeout to apply to read vs open timeout.

[bgiori]

Ok, I can add an additional config for read vs connect timeout. Thanks again for your input.

[jacoobb]

Hi everyone,
I have the same problem I need connection timeout.
@bgiori Can you tell me what is the state of progress?
🙇

[jacoobb]

I found a bug in AmplitudeExperiment::RemoteEvaluationClient.do_fetch method:

   def do_fetch(user, timeout_millis)
      start_time = Time.now
      user_context = add_context(user)
      headers = {
        'Authorization' => "Api-Key #{@api_key}",
        'Content-Type' => 'application/json;charset=utf-8'
      }
      read_timeout = timeout_millis / 1000 if (timeout_millis / 1000) > 0
      http = PersistentHttpClient.get(@uri, { read_timeout: read_timeout }, @api_key)
      request = Net::HTTP::Post.new(@uri, headers)
      request.body = user_context.to_json
      if request.body.length > 8000
        @logger.warn("[Experiment] encoded user object length #{request.body.length} cannot be cached by CDN; must be < 8KB")
      end
      @logger.debug("[Experiment] Fetch variants for user: #{request.body}")
      response = http.request(request)
      end_time = Time.now
      elapsed = (end_time - start_time) * 1000.0
      @logger.debug("[Experiment] Fetch complete in #{elapsed.round(3)} ms")
      json = JSON.parse(response.body)
      variants = parse_json_variants(json)
      @logger.debug("[Experiment] Fetched variants: #{variants}")
      variants
    end

in line: read_timeout = timeout_millis / 1000 if (timeout_millis / 1000) > 0 should be
read_timeout = timeout_millis.to_f / 1000 if (timeout_millis.to_f / 1000).positive?

This is the reason fetch_timeout_millis doesn't work. read_timeout value is nil for each timeout_millis values below 1000.

[tyiuhc]

@therabidbanana @jacoobb Many thanks for raising this issue! The fix has been implemented in v1.2.1

[jkurdel]

Ok, I can add an additional config for read vs connect timeout. Thanks again for your input.

Hi @bgiori @tyiuhc

I see this issue being closed but there is no connection timeout being added. Could you add this additional conifg? Should I open the new issue?