Optimize Crashlytics fetches (#730)

- Include a start date before querying for crashlytics/analytics data from BigQuery
- Flag off apps if the monitoring_disabled feature flag is enabled for them

app/libs/installations/crashlytics/api.rb

@@ -12,9 +12,9 @@ def initialize(project_number, json_key)
       @json_key = json_key
     end
 
-    def find_release(bundle_identifier, platform, app_version, app_version_code, transforms)
+    def find_release(bundle_identifier, platform, app_version, app_version_code, start_date, transforms)
       execute do
-        crash_data = fetch_crash_data(bundle_identifier, platform.upcase, app_version, app_version_code)
+        crash_data = fetch_crash_data(bundle_identifier, platform.upcase, app_version, app_version_code, start_date)
         return if crash_data.blank?
         Installations::Response::Keys.transform([crash_data], transforms).first
       end
@@ -37,9 +37,11 @@ def datasets
 
     private
 
-    def fetch_crash_data(bundle_identifier, platform, app_version, app_version_code)
-      analytics_data = get_data(analytics_query(datasets[:ga4], bundle_identifier, platform, app_version)).find { |a| a[:version_name] == app_version } || {}
-      crashlytics_data = get_data(crashlytics_query(datasets[:crashlytics], bundle_identifier, platform, app_version, app_version_code)).find { |a| a[:version_name] == app_version } || {}
+    def fetch_crash_data(bundle_identifier, platform, app_version, app_version_code, start_date)
+      aq = analytics_query(datasets[:ga4], bundle_identifier, platform, app_version, start_date)
+      analytics_data = get_data(aq).find { |a| a[:version_name] == app_version } || {}
+      cq = crashlytics_query(datasets[:crashlytics], bundle_identifier, platform, app_version, app_version_code, start_date)
+      crashlytics_data = get_data(cq).find { |a| a[:version_name] == app_version } || {}
       analytics_data.merge(crashlytics_data)
     end
 
@@ -55,8 +57,9 @@ def get_data(query)
       execute { bigquery_client.query(query) }
     end
 
-    def crashlytics_query(dataset_name, bundle_identifier, platform, version_name, version_code)
+    def crashlytics_query(dataset_name, bundle_identifier, platform, version_name, version_code, start_date)
       table_name = dataset_name.sub("*", "") + bundle_identifier.tr(".", "_") + "_" + platform + "*"
+      start_timestamp = date_to_bq_date(start_date)
       <<-SQL.squish
         WITH combined_events AS (
           SELECT
@@ -66,7 +69,7 @@ def crashlytics_query(dataset_name, bundle_identifier, platform, version_name, v
             event_timestamp,
             bundle_identifier
           FROM `#{table_name}`
-          WHERE event_timestamp >= TIMESTAMP_SUB(CURRENT_TIMESTAMP(), INTERVAL 30 DAY)
+          WHERE event_timestamp >= '#{start_timestamp}'
           AND error_type = 'FATAL'
         ),
         errors_with_version AS (
@@ -103,9 +106,9 @@ def crashlytics_query(dataset_name, bundle_identifier, platform, version_name, v
       SQL
     end
 
-    def analytics_query(dataset_name, bundle_identifier, platform, version_name)
+    def analytics_query(dataset_name, bundle_identifier, platform, version_name, start_date)
       table_name = dataset_name.sub("*", "events_*")
-      events_table_suffix = (Date.current - 30.days).strftime("%Y%m%d")
+      events_table_suffix = start_date.strftime("%Y%m%d")
       <<-SQL.squish
         WITH combined_events AS (
           SELECT
@@ -161,5 +164,9 @@ def execute
     rescue ::Google::Cloud::PermissionDeniedError => e
       raise Installations::Crashlytics::Error.new("Authentication failed: #{e.message}")
     end
+
+    def date_to_bq_date(date)
+      date.iso8601
+    end
   end
 end

app/models/app.rb

@@ -85,6 +85,10 @@ def deploy_action_enabled?
     Flipper.enabled?(:deploy_action_enabled, self)
   end
 
+  def monitoring_disabled?
+    Flipper.enabled?(:monitoring_disabled, self)
+  end
+
   def variant_options
     opts = {"Default (#{bundle_identifier})" => nil}
     opts.merge variants.map.to_h { |v| [v.display_text, v.id] }

app/models/bugsnag_integration.rb

@@ -100,7 +100,7 @@ def metadata
     list_organizations
   end
 
-  def find_release(platform, version, build_number)
+  def find_release(platform, version, build_number, _start_date = nil)
     installation.find_release(project_id(platform), release_stage(platform), version, build_number, RELEASE_TRANSFORMATIONS)
   end
 

app/models/crashlytics_integration.rb

@@ -71,9 +71,11 @@ def metadata
     {}
   end
 
-  def find_release(platform, version, build_number)
+  # Crashlytics specifically accepts a start_date param because the queries to fetch the release data
+  # are expensive (in $$), so we try to minimize the window of fetch to save on costs.
+  def find_release(platform, version, build_number, start_date)
     return nil if version.blank?
-    installation.find_release(integrable.bundle_identifier, platform, version, build_number, RELEASE_TRANSFORMATIONS)
+    installation.find_release(integrable.bundle_identifier, platform, version, build_number, start_date, RELEASE_TRANSFORMATIONS)
   end
 
   # FIXME: This is an incomplete URL. The full URL should contain the project id.

app/models/production_release.rb

@@ -17,7 +17,6 @@ class ProductionRelease < ApplicationRecord
   # include Sandboxable
   include Loggable
   include Passportable
-  RELEASE_MONITORING_PERIOD_IN_DAYS = 15
 
   belongs_to :release_platform_run
   belongs_to :build
@@ -48,8 +47,12 @@ class ProductionRelease < ApplicationRecord
   ACTIONABLE_STATES = [STATES[:inflight], STATES[:active]]
 
   JOB_FREQUENCY = {
-    CrashlyticsIntegration => 120.minutes,
-    BugsnagIntegration => 5.minutes
+    BugsnagIntegration => 5.minutes,
+    CrashlyticsIntegration => 120.minutes
+  }
+  RELEASE_MONITORING_PERIOD_IN_DAYS = {
+    BugsnagIntegration => 15,
+    CrashlyticsIntegration => 5
   }
 
   enum :status, STATES
@@ -120,27 +123,24 @@ def rollout_started!
 
     return if beyond_monitoring_period?
     return if monitoring_provider.blank?
-    # NOTE: Disable all Crashlytics query temporarily
-    return if monitoring_provider.is_a?(CrashlyticsIntegration)
+    return if app.monitoring_disabled?
+
     FetchHealthMetricsJob.perform_async(id, JOB_FREQUENCY[monitoring_provider.class])
   end
 
   def beyond_monitoring_period?
-    finished? && completed_at < RELEASE_MONITORING_PERIOD_IN_DAYS.days.ago
+    finished? && completed_at && completed_at < release_monitoring_period
   end
 
   def fetch_health_data!
     return if store_rollout.blank?
     return if beyond_monitoring_period?
     return if monitoring_provider.blank?
-    # NOTE: Disable all Crashlytics query temporarily
-    return if monitoring_provider.is_a?(CrashlyticsIntegration)
+    return if app.monitoring_disabled?
     return if stale?
 
-    release_data = monitoring_provider.find_release(platform, version_name, build_number)
-
+    release_data = monitoring_provider.find_release(platform, version_name, build_number, store_rollout.created_at)
     return if release_data.blank?
-
     release_health_metrics.create!(fetched_at: Time.current, **release_data)
   end
 
@@ -202,4 +202,8 @@ def commits_since_previous
       changes_since_last_release
     end
   end
+
+  def release_monitoring_period
+    RELEASE_MONITORING_PERIOD_IN_DAYS[monitoring_provider.class].days.ago
+  end
 end

spec/factories/integrations.rb

@@ -46,5 +46,10 @@
       category { "project_management" }
       providable factory: %i[jira_integration with_app_config]
     end
+
+    trait :with_crashlytics do
+      category { "monitoring" }
+      providable factory: %i[crashlytics_integration skip_validate_key]
+    end
   end
 end

spec/factories/release_platform_runs.rb

@@ -117,11 +117,11 @@
   end
 end
 
-def create_production_rollout_tree(train, release_platform, release_traits: [:finished], run_status: :finished, rollout_status: :completed, submission_status: :created, skip_rollout: false)
+def create_production_rollout_tree(train, release_platform, release_traits: [:finished], run_status: :finished, parent_release_status: :finished, rollout_status: :completed, submission_status: :created, skip_rollout: false)
   release = create(:release, *release_traits, train:)
   platform = release_platform.platform
   release_platform_run = create(:release_platform_run, platform.to_sym, run_status, release_platform:, release:)
-  parent_release = create(:production_release, :finished, config: release_platform_run.conf.production_release.as_json, release_platform_run:)
+  parent_release = create(:production_release, parent_release_status, config: release_platform_run.conf.production_release.as_json, release_platform_run:)
   store_submission = create(:play_store_submission, status: submission_status, config: parent_release.conf.submissions.first, parent_release:, release_platform_run:)
 
   unless skip_rollout

spec/libs/installations/crashlytics/api_spec.rb

@@ -1,6 +1,6 @@
 require "rails_helper"
 
-Dataset = Struct.new(:dataset_id, :project_id)
+Dataset = Data.define(:dataset_id, :project_id)
 APP_IDENTIFIER = "com.example.app"
 APP_ONE = "App One"
 APP_TWO = "App Two"
@@ -62,7 +62,7 @@
       end
 
       it "returns the transformed release data" do
-        result = api_instance.find_release(bundle_identifier, platform, version, build_number, transforms)
+        result = api_instance.find_release(bundle_identifier, platform, version, build_number, Time.current, transforms)
         expected_data = {
           "daily_users" => 150,
           "daily_users_with_errors" => 30,
@@ -85,7 +85,7 @@
       end
 
       it "returns nil" do
-        result = api_instance.find_release(bundle_identifier, platform, version, build_number, transforms)
+        result = api_instance.find_release(bundle_identifier, platform, version, build_number, Time.current, transforms)
         expect(result).to be_nil
       end
     end

spec/models/crashlytics_integration_spec.rb

@@ -44,19 +44,21 @@
     end
 
     it "calls the API find_release method with correct arguments" do
-      crashlytics_integration.find_release(platform, version, build_number)
+      start_date = Time.current
+      crashlytics_integration.find_release(platform, version, build_number, start_date)
 
       expect(api_instance).to have_received(:find_release).with(
         bundle_identifier,
         platform,
         version,
         build_number,
+        start_date,
         CrashlyticsIntegration::RELEASE_TRANSFORMATIONS
       )
     end
 
     it "returns nil if version is blank" do
-      expect(crashlytics_integration.find_release("android", nil, build_number)).to be_nil
+      expect(crashlytics_integration.find_release("android", nil, build_number, Time.current)).to be_nil
     end
   end
 

spec/models/production_release_spec.rb

@@ -79,4 +79,53 @@
       expect(ReleasePlatformRuns::CreateTagJob).to have_received(:perform_async).with(release_platform_run.id, production_release.commit.id)
     end
   end
+
+  describe "#fetch_health_data!" do
+    let(:train) { create(:train, tag_all_store_releases: true, tag_platform_releases: true) }
+    let(:release_platform) { create(:release_platform, train:) }
+    let(:monitoring_api_dbl) { instance_double(Installations::Crashlytics::Api) }
+
+    it "fetches health data from the monitoring provider" do
+      integration = create(:integration, :with_crashlytics)
+      monitoring_provider = integration.providable
+      create_production_rollout_tree(
+        train,
+        release_platform,
+        release_traits: [:on_track],
+        run_status: :on_track,
+        parent_release_status: :active,
+        rollout_status: :started,
+        skip_rollout: false
+      ) => {release_platform:, release:, production_release:, store_rollout:}
+      allow(production_release).to receive(:monitoring_provider).and_return(monitoring_provider)
+      allow(monitoring_provider).to receive(:installation).and_return(monitoring_api_dbl)
+      allow(monitoring_provider).to receive(:find_release)
+
+      production_release.fetch_health_data!
+
+      expect(monitoring_provider).to have_received(:find_release).with(release_platform.platform, release.release_version, production_release.build.build_number, store_rollout.created_at)
+    end
+
+    it "does not fetch health data if app has monitoring disabled" do
+      integration = create(:integration, :with_crashlytics)
+      monitoring_provider = integration.providable
+      create_production_rollout_tree(
+        train,
+        release_platform,
+        release_traits: [:on_track],
+        run_status: :on_track,
+        parent_release_status: :active,
+        rollout_status: :started,
+        skip_rollout: false
+      ) => {release_platform:, release:, production_release:, store_rollout:}
+      allow(production_release).to receive(:monitoring_provider).and_return(monitoring_provider)
+      allow(monitoring_provider).to receive(:installation).and_return(monitoring_api_dbl)
+      allow(monitoring_provider).to receive(:find_release)
+
+      Flipper.enable_actor(:monitoring_disabled, train.app)
+      production_release.fetch_health_data!
+
+      expect(monitoring_provider).not_to have_received(:find_release).with(release_platform.platform, release.release_version, production_release.build.build_number, store_rollout.created_at)
+    end
+  end
 end