From ab7bf8787c83873d75ed62d7a15d6eff25824b53 Mon Sep 17 00:00:00 2001 From: Akshay Gupta Date: Tue, 4 Feb 2025 12:50:15 +0530 Subject: [PATCH] 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 | 25 ++++++---- app/models/app.rb | 4 ++ app/models/bugsnag_integration.rb | 2 +- app/models/crashlytics_integration.rb | 6 ++- app/models/production_release.rb | 26 +++++----- spec/factories/integrations.rb | 5 ++ spec/factories/play_store_rollout.rb | 0 spec/factories/release_platform_runs.rb | 4 +- .../installations/crashlytics/api_spec.rb | 6 +-- spec/models/crashlytics_integration_spec.rb | 6 ++- spec/models/production_release_spec.rb | 49 +++++++++++++++++++ 11 files changed, 103 insertions(+), 30 deletions(-) delete mode 100644 spec/factories/play_store_rollout.rb diff --git a/app/libs/installations/crashlytics/api.rb b/app/libs/installations/crashlytics/api.rb index 9266614e4..3bfe5960c 100644 --- a/app/libs/installations/crashlytics/api.rb +++ b/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 diff --git a/app/models/app.rb b/app/models/app.rb index 2da207277..6e0dc3783 100644 --- a/app/models/app.rb +++ b/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] } diff --git a/app/models/bugsnag_integration.rb b/app/models/bugsnag_integration.rb index 8556d22d8..d1e3702ab 100644 --- a/app/models/bugsnag_integration.rb +++ b/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 diff --git a/app/models/crashlytics_integration.rb b/app/models/crashlytics_integration.rb index 6a8d5d754..2d3e41ea7 100644 --- a/app/models/crashlytics_integration.rb +++ b/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. diff --git a/app/models/production_release.rb b/app/models/production_release.rb index d7eaeb302..d2a38d10f 100644 --- a/app/models/production_release.rb +++ b/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 diff --git a/spec/factories/integrations.rb b/spec/factories/integrations.rb index 9cbd5b06e..87b25d1cc 100644 --- a/spec/factories/integrations.rb +++ b/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 diff --git a/spec/factories/play_store_rollout.rb b/spec/factories/play_store_rollout.rb deleted file mode 100644 index e69de29bb..000000000 diff --git a/spec/factories/release_platform_runs.rb b/spec/factories/release_platform_runs.rb index 4d82c87f0..b813a858a 100644 --- a/spec/factories/release_platform_runs.rb +++ b/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 diff --git a/spec/libs/installations/crashlytics/api_spec.rb b/spec/libs/installations/crashlytics/api_spec.rb index 69ce38423..6ad088989 100644 --- a/spec/libs/installations/crashlytics/api_spec.rb +++ b/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 diff --git a/spec/models/crashlytics_integration_spec.rb b/spec/models/crashlytics_integration_spec.rb index e9fd33855..63c592991 100644 --- a/spec/models/crashlytics_integration_spec.rb +++ b/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 diff --git a/spec/models/production_release_spec.rb b/spec/models/production_release_spec.rb index f81ed5102..db45e1124 100644 --- a/spec/models/production_release_spec.rb +++ b/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