fix(flags): Add appropriate timeouts, fix some other misc things

lib/posthog/client.rb

@@ -23,6 +23,7 @@ class Client
     # @option opts [Proc] :on_error Handles error calls from the API.
     # @option opts [String] :host Fully qualified hostname of the PostHog server. Defaults to `https://app.posthog.com`
     # @option opts [Integer] :feature_flags_polling_interval How often to poll for feature flag definition changes. Measured in seconds, defaults to 30.
+    # @option opts [Integer] :feature_flag_request_timeout_seconds How long to wait for feature flag evaluation. Measured in seconds, defaults to 3.
     def initialize(opts = {})
       symbolize_keys!(opts)
 
@@ -48,7 +49,9 @@ def initialize(opts = {})
           opts[:feature_flags_polling_interval],
           opts[:personal_api_key],
           @api_key,
-          opts[:host]
+          opts[:host],
+          opts[:feature_flag_request_timeout_seconds] || Defaults::FeatureFlags::FLAG_REQUEST_TIMEOUT_SECONDS,
+          opts[:on_error]
         )
 
       @distinct_id_has_sent_flag_calls = SizeLimitedHash.new(Defaults::MAX_HASH_SIZE) { |hash, key| hash[key] = Array.new }
@@ -90,7 +93,7 @@ def capture(attrs)
       symbolize_keys! attrs
 
       if attrs[:send_feature_flags]
-        feature_variants = @feature_flags_poller._get_active_feature_variants(attrs[:distinct_id], attrs[:groups])
+        feature_variants = @feature_flags_poller.get_feature_variants(attrs[:distinct_id], attrs[:groups] || {})
 
         attrs[:feature_variants] = feature_variants
       end

lib/posthog/defaults.rb

@@ -16,6 +16,10 @@ module Request
       RETRIES = 10
     end
 
+    module FeatureFlags
+      FLAG_REQUEST_TIMEOUT_SECONDS = 3
+    end
+
     module Queue
       MAX_SIZE = 10_000
     end

lib/posthog/feature_flags.rb

@@ -14,7 +14,7 @@ class FeatureFlagsPoller
     include PostHog::Logging
     include PostHog::Utils
 
-    def initialize(polling_interval, personal_api_key, project_api_key, host)
+    def initialize(polling_interval, personal_api_key, project_api_key, host, feature_flag_request_timeout_seconds, on_error = nil)
       @polling_interval = polling_interval || 30
       @personal_api_key = personal_api_key
       @project_api_key = project_api_key
@@ -23,6 +23,8 @@ def initialize(polling_interval, personal_api_key, project_api_key, host)
       @group_type_mapping = Concurrent::Hash.new
       @loaded_flags_successfully_once = Concurrent::AtomicBoolean.new
       @feature_flags_by_key = nil
+      @feature_flag_request_timeout_seconds = feature_flag_request_timeout_seconds
+      @on_error = on_error || proc { |status, error| }
 
       @task =
         Concurrent::TimerTask.new(
@@ -46,30 +48,22 @@ def load_feature_flags(force_reload = false)
       end
     end
 
-    def get_feature_variants(distinct_id, groups={}, person_properties={}, group_properties={})
-      decide_data = get_decide(distinct_id, groups, person_properties, group_properties)
+    def get_feature_variants(distinct_id, groups={}, person_properties={}, group_properties={}, raise_on_error=false)
+      # TODO: Convert to options hash for easier argument passing
+      decide_data = get_all_flags_and_payloads(distinct_id, groups, person_properties, group_properties, false, raise_on_error)
       if !decide_data.key?(:featureFlags)
-        logger.error "Missing feature flags key: #{decide_data.to_json}"
+        logger.debug "Missing feature flags key: #{decide_data.to_json}"
+        return {}
       else
         stringify_keys(decide_data[:featureFlags] || {})
       end
     end
 
-    def _get_active_feature_variants(distinct_id, groups={}, person_properties={}, group_properties={})
-      feature_variants = get_feature_variants(distinct_id, groups, person_properties, group_properties)
-      active_feature_variants = {}
-      feature_variants.each do |key, value|
-        if value != false
-          active_feature_variants[key] = value
-        end
-      end
-      active_feature_variants
-    end
-
     def get_feature_payloads(distinct_id, groups = {}, person_properties = {}, group_properties = {}, only_evaluate_locally = false)
-      decide_data = get_decide(distinct_id, groups, person_properties, group_properties)
+      decide_data = get_all_flags_and_payloads(distinct_id, groups, person_properties, group_properties)
       if !decide_data.key?(:featureFlagPayloads)
-        logger.error "Missing feature flag payloads key: #{decide_data.to_json}"
+        logger.debug "Missing feature flag payloads key: #{decide_data.to_json}"
+        return {}
       else
         stringify_keys(decide_data[:featureFlagPayloads] || {})
       end
@@ -115,22 +109,22 @@ def get_feature_flag(key, distinct_id, groups = {}, person_properties = {}, grou
         rescue InconclusiveMatchError => e
           logger.debug "Failed to compute flag #{key} locally: #{e}"
         rescue StandardError => e
-          logger.error "Error computing flag locally: #{e}. #{e.backtrace.join("\n")}"
+          @on_error.call(-1, "Error computing flag locally: #{e}. #{e.backtrace.join("\n")}")
         end
       end
 
       flag_was_locally_evaluated = !response.nil?
 
       if !flag_was_locally_evaluated && !only_evaluate_locally
         begin
-          flags = get_feature_variants(distinct_id, groups, person_properties, group_properties)
+          flags = get_feature_variants(distinct_id, groups, person_properties, group_properties, true)
           response = flags[key]
           if response.nil?
             response = false
           end
           logger.debug "Successfully computed flag remotely: #{key} -> #{response}"
         rescue StandardError => e
-          logger.error "Error computing flag remotely: #{e}. #{e.backtrace.join("\n")}"
+          @on_error.call(-1, "Error computing flag remotely: #{e}. #{e.backtrace.join("\n")}")
         end
       end
 
@@ -143,7 +137,7 @@ def get_all_flags(distinct_id, groups = {}, person_properties = {}, group_proper
       flags = response[:featureFlags]
     end
 
-    def get_all_flags_and_payloads(distinct_id, groups = {}, person_properties = {}, group_properties = {}, only_evaluate_locally = false)
+    def get_all_flags_and_payloads(distinct_id, groups = {}, person_properties = {}, group_properties = {}, only_evaluate_locally = false, raise_on_error = false)
       load_feature_flags
 
       flags = {}
@@ -161,17 +155,24 @@ def get_all_flags_and_payloads(distinct_id, groups = {}, person_properties = {},
         rescue InconclusiveMatchError => e
           fallback_to_decide = true
         rescue StandardError => e
-          logger.error "Error computing flag locally: #{e}."
+          @on_error.call(-1, "Error computing flag locally: #{e}. #{e.backtrace.join("\n")} ")
           fallback_to_decide = true
         end
       end
       if fallback_to_decide && !only_evaluate_locally
         begin
           flags_and_payloads = get_decide(distinct_id, groups, person_properties, group_properties)
+
+          if !flags_and_payloads.key?(:featureFlags)
+            raise StandardError.new("Error flags response: #{flags_and_payloads}")
+          end
           flags = stringify_keys(flags_and_payloads[:featureFlags] || {})
           payloads = stringify_keys(flags_and_payloads[:featureFlagPayloads] || {})
         rescue StandardError => e
-          logger.error "Error computing flag remotely: #{e}"
+          @on_error.call(-1, "Error computing flag remotely: #{e}")
+          if raise_on_error
+            raise e
+          end
         end
       end
       {"featureFlags": flags, "featureFlagPayloads": payloads}
@@ -372,6 +373,10 @@ def _compute_flag_locally(flag, distinct_id, groups = {}, person_properties = {}
     def _compute_flag_payload_locally(key, match_value)
       response = nil
 
+      if @feature_flags_by_key.nil?
+        return response
+      end
+
       if [true, false].include? match_value
         response = @feature_flags_by_key.dig(key, :filters, :payloads, match_value.to_s.to_sym)
       elsif match_value.is_a? String
@@ -472,10 +477,15 @@ def variant_lookup_table(flag)
     end
 
     def _load_feature_flags()
-      res = _request_feature_flag_definitions
+      begin
+        res = _request_feature_flag_definitions
+      rescue StandardError => e
+        @on_error.call(-1, e.to_s)
+        return
+      end
 
       if !res.key?(:flags)
-        logger.error "Failed to load feature flags: #{res}"
+        logger.debug "Failed to load feature flags: #{res}"
       else
         @feature_flags = res[:flags] || []
         @feature_flags_by_key = {}
@@ -508,16 +518,18 @@ def _request_feature_flag_evaluation(data={})
       data['token'] = @project_api_key
       req.body = data.to_json
 
-      _request(uri, req)
+      _request(uri, req, @feature_flag_request_timeout_seconds)
     end
 
-    def _request(uri, request_object)
+    def _request(uri, request_object, timeout = nil)
 
       request_object['User-Agent'] = "posthog-ruby#{PostHog::VERSION}"
 
+      request_timeout = timeout || 10
+
       begin
         res_body = nil
-        Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == 'https') do |http|
+        Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == 'https', :read_timeout => request_timeout) do |http|
           res = http.request(request_object)
           JSON.parse(res.body, {symbolize_names: true})
         end
@@ -527,9 +539,11 @@ def _request(uri, request_object)
              EOFError,
              Net::HTTPBadResponse,
              Net::HTTPHeaderSyntaxError,
+             Net::ReadTimeout,
+             Net::WriteTimeout,
              Net::ProtocolError => e
         logger.debug("Unable to complete request to #{uri}")
-        throw e
+        raise e
       end
     end
   end

lib/posthog/field_parser.rb

@@ -137,10 +137,14 @@ def parse_common_fields(fields)
 
         if send_feature_flags
           feature_variants = fields[:feature_variants]
+          active_feature_variants = {}
           feature_variants.each do |key, value|
             parsed[:properties]["$feature/#{key}"] = value
+            if value != false
+              active_feature_variants[key] = value
+            end
           end
-          parsed[:properties]["$active_feature_flags"] = feature_variants.keys
+          parsed[:properties]["$active_feature_flags"] = active_feature_variants.keys
         end
         parsed
       end

posthog-ruby.gemspec

@@ -31,5 +31,5 @@ Gem::Specification.new do |spec|
   if RUBY_VERSION >= '2.1'
     spec.add_development_dependency 'rubocop', '~> 0.51.0'
   end
-  spec.add_development_dependency 'codecov', '~> 0.1.4'
+  spec.add_development_dependency 'codecov', '~> 0.6.0'
 end

spec/posthog/client_spec.rb

@@ -126,6 +126,49 @@ class PostHog
           .to_return(status: 200, body: decide_res.to_json)
         c = Client.new(api_key: API_KEY, personal_api_key: API_KEY, test_mode: true)
 
+        c.capture(
+          {
+            distinct_id: "distinct_id",
+            event: "ruby test event",
+            send_feature_flags: true,
+          }
+        )
+        properties = c.dequeue_last_message[:properties]
+        expect(properties["$feature/beta-feature"]).to eq(false)
+        expect(properties["$active_feature_flags"]).to eq([])
+      end
+
+      it 'captures feature flags with fallback to decide when needed' do
+        decide_res = {"featureFlags": {"beta-feature": "random-variant", "alpha-feature": true, "off-feature": false}}
+        # Mock response for decide
+        api_feature_flag_res = {
+          "flags": [
+            {
+              "id": 1,
+              "name": '',
+              "key": 'beta-feature',
+              "active": true,
+              "is_simple_flag": false,
+              "rollout_percentage": 100,
+              "filters": {
+                  "groups": [
+                      {
+                          "properties": [{"key": "regionXXX", "value": "USA", "type": "person"}],
+                          "rollout_percentage": 100,
+                      }
+                  ],
+              },
+            },]
+          }
+
+        stub_request(
+          :get,
+          'https://app.posthog.com/api/feature_flag/local_evaluation?token=testsecret'
+        ).to_return(status: 200, body: api_feature_flag_res.to_json)
+        stub_request(:post, decide_endpoint)
+          .to_return(status: 200, body: decide_res.to_json)
+        c = Client.new(api_key: API_KEY, personal_api_key: API_KEY, test_mode: true)
+
         c.capture(
           {
             distinct_id: "distinct_id",
@@ -135,7 +178,9 @@ class PostHog
         )
         properties = c.dequeue_last_message[:properties]
         expect(properties["$feature/beta-feature"]).to eq("random-variant")
-        expect(properties["$active_feature_flags"]).to eq(["beta-feature"])
+        expect(properties["$feature/alpha-feature"]).to eq(true)
+        expect(properties["$feature/off-feature"]).to eq(false)
+        expect(properties["$active_feature_flags"]).to eq(["beta-feature", "alpha-feature"])
       end
 
       it 'captures active feature flags only' do
@@ -254,6 +299,20 @@ class PostHog
                   ],
               },
             },
+            {
+              "id": 2,
+              "name": "Beta Feature",
+              "key": "decide-flag",
+              "active": true,
+              "filters": {
+                  "groups": [
+                      {
+                          "properties": [{"key": "region?????", "value": "USA"}],
+                          "rollout_percentage": 100,
+                      }
+                  ],
+              },
+            },
           ]
         }
 
@@ -282,11 +341,15 @@ class PostHog
         }).exactly(1).times
         expect(c.get_feature_flag('beta-feature', 'some-distinct-id', person_properties: {"region": "USA", "name": "Aloha"})).to eq(true)
 
-
+        # reset capture mock
+        RSpec::Mocks.space.proxy_for(c).reset
+        allow(c).to receive(:capture)
         # called again for same user, shouldn't call capture again
-        # expect(c).not_to receive(:capture)
+        expect(c).not_to receive(:capture)
         expect(c.get_feature_flag('beta-feature', 'some-distinct-id', person_properties: {"region": "USA", "name": "Aloha"})).to eq(true)
 
+        RSpec::Mocks.space.proxy_for(c).reset
+        allow(c).to receive(:capture)
         # called for different user, should call capture again
         expect(c).to receive(:capture).with({
           distinct_id: "some-distinct-id2",