From 9259c345cc38ffae84fd34cd6f4cad24bef5f31e Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Thu, 7 Mar 2024 15:19:04 +0000 Subject: [PATCH] utils/analytics: general cleanup. We have plans to add analytics for commands and `brew test-bot` This requires a certain amount of refactoring which I've done here. There was also a bunch of legacy `*_influx_?` usage from when we used both InfluxDB and Google Analytics that made sense to clean up and excessive indirection. --- Library/Homebrew/cask/installer.rb | 2 +- Library/Homebrew/formula_installer.rb | 2 +- Library/Homebrew/test/utils/analytics_spec.rb | 53 +++++++------- Library/Homebrew/utils/analytics.rb | 73 +++++++++---------- 4 files changed, 61 insertions(+), 69 deletions(-) diff --git a/Library/Homebrew/cask/installer.rb b/Library/Homebrew/cask/installer.rb index 8786d187b0c32..b03a4032521b8 100644 --- a/Library/Homebrew/cask/installer.rb +++ b/Library/Homebrew/cask/installer.rb @@ -112,7 +112,7 @@ def install install_artifacts(predecessor: predecessor) if (tap = @cask.tap) && tap.should_report_analytics? - ::Utils::Analytics.report_event(:cask_install, package_name: @cask.token, tap_name: tap.name, + ::Utils::Analytics.report_package_event(:cask_install, package_name: @cask.token, tap_name: tap.name, on_request: true) end diff --git a/Library/Homebrew/formula_installer.rb b/Library/Homebrew/formula_installer.rb index 5fb23489e2bc6..83d621498b444 100644 --- a/Library/Homebrew/formula_installer.rb +++ b/Library/Homebrew/formula_installer.rb @@ -419,7 +419,7 @@ def install oh1 "Installing #{Formatter.identifier(formula.full_name)} #{options}".strip if show_header? if (tap = formula.tap) && tap.should_report_analytics? - Utils::Analytics.report_event(:formula_install, package_name: formula.name, tap_name: tap.name, + Utils::Analytics.report_package_event(:formula_install, package_name: formula.name, tap_name: tap.name, on_request: installed_on_request?, options: options) end diff --git a/Library/Homebrew/test/utils/analytics_spec.rb b/Library/Homebrew/test/utils/analytics_spec.rb index dc2100336f387..3867fe79de0df 100644 --- a/Library/Homebrew/test/utils/analytics_spec.rb +++ b/Library/Homebrew/test/utils/analytics_spec.rb @@ -8,47 +8,47 @@ described_class.clear_cache end - describe "::default_tags_influx" do + describe "::default_package_tags" do let(:ci) { ", CI" if ENV["CI"] } it "returns OS_VERSION and prefix when HOMEBREW_PREFIX is a custom prefix on intel" do expect(Homebrew).to receive(:default_prefix?).and_return(false).at_least(:once) - expect(described_class.default_tags_influx).to have_key(:prefix) - expect(described_class.default_tags_influx[:prefix]).to eq "custom-prefix" + expect(described_class.default_package_tags).to have_key(:prefix) + expect(described_class.default_package_tags[:prefix]).to eq "custom-prefix" end it "returns OS_VERSION, ARM and prefix when HOMEBREW_PREFIX is a custom prefix on arm" do expect(Homebrew).to receive(:default_prefix?).and_return(false).at_least(:once) - expect(described_class.default_tags_influx).to have_key(:arch) - expect(described_class.default_tags_influx[:arch]).to eq HOMEBREW_PHYSICAL_PROCESSOR - expect(described_class.default_tags_influx).to have_key(:prefix) - expect(described_class.default_tags_influx[:prefix]).to eq "custom-prefix" + expect(described_class.default_package_tags).to have_key(:arch) + expect(described_class.default_package_tags[:arch]).to eq HOMEBREW_PHYSICAL_PROCESSOR + expect(described_class.default_package_tags).to have_key(:prefix) + expect(described_class.default_package_tags[:prefix]).to eq "custom-prefix" end it "returns OS_VERSION, Rosetta and prefix when HOMEBREW_PREFIX is a custom prefix on Rosetta", :needs_macos do expect(Homebrew).to receive(:default_prefix?).and_return(false).at_least(:once) - expect(described_class.default_tags_influx).to have_key(:prefix) - expect(described_class.default_tags_influx[:prefix]).to eq "custom-prefix" + expect(described_class.default_package_tags).to have_key(:prefix) + expect(described_class.default_package_tags[:prefix]).to eq "custom-prefix" end it "does not include prefix when HOMEBREW_PREFIX is the default prefix" do expect(Homebrew).to receive(:default_prefix?).and_return(true).at_least(:once) - expect(described_class.default_tags_influx).to have_key(:prefix) - expect(described_class.default_tags_influx[:prefix]).to eq HOMEBREW_PREFIX.to_s + expect(described_class.default_package_tags).to have_key(:prefix) + expect(described_class.default_package_tags[:prefix]).to eq HOMEBREW_PREFIX.to_s end it "includes CI when ENV['CI'] is set" do ENV["CI"] = "1" - expect(described_class.default_tags_influx).to have_key(:ci) + expect(described_class.default_package_tags).to have_key(:ci) end it "includes developer when ENV['HOMEBREW_DEVELOPER'] is set" do expect(Homebrew::EnvConfig).to receive(:developer?).and_return(true) - expect(described_class.default_tags_influx).to have_key(:developer) + expect(described_class.default_package_tags).to have_key(:developer) end end - describe "::report_event" do + describe "::report_package_event" do let(:f) { formula { url "foo-1.0" } } let(:package_name) { f.name } let(:tap_name) { f.tap.name } @@ -59,14 +59,14 @@ it "returns nil when HOMEBREW_NO_ANALYTICS is true" do ENV["HOMEBREW_NO_ANALYTICS"] = "true" expect(described_class).not_to receive(:report_influx) - described_class.report_event(:install, package_name: package_name, tap_name: tap_name, + described_class.report_package_event(:install, package_name: package_name, tap_name: tap_name, on_request: on_request, options: options) end it "returns nil when HOMEBREW_NO_ANALYTICS_THIS_RUN is true" do ENV["HOMEBREW_NO_ANALYTICS_THIS_RUN"] = "true" expect(described_class).not_to receive(:report_influx) - described_class.report_event(:install, package_name: package_name, tap_name: tap_name, + described_class.report_package_event(:install, package_name: package_name, tap_name: tap_name, on_request: on_request, options: options) end @@ -76,7 +76,7 @@ ENV["HOMEBREW_ANALYTICS_DEBUG"] = "true" expect(described_class).to receive(:report_influx) - described_class.report_event(:install, package_name: package_name, tap_name: tap_name, + described_class.report_package_event(:install, package_name: package_name, tap_name: tap_name, on_request: on_request, options: options) end end @@ -85,16 +85,16 @@ ENV.delete("HOMEBREW_NO_ANALYTICS_THIS_RUN") ENV.delete("HOMEBREW_NO_ANALYTICS") ENV["HOMEBREW_ANALYTICS_DEBUG"] = "true" - expect(described_class).to receive(:report_influx).with(:install, hash_including(package_name: package_name, - on_request: on_request)).once - described_class.report_event(:install, package_name: package_name, tap_name: tap_name, + expect(described_class).to receive(:report_influx).with(:install, hash_including(on_request:), + hash_including(package: package_name)).once + described_class.report_package_event(:install, package_name: package_name, tap_name: tap_name, on_request: on_request, options: options) end end describe "::report_influx" do let(:f) { formula { url "foo-1.0" } } - let(:package_name) { f.name } + let(:package) { f.name } let(:tap_name) { f.tap.name } let(:on_request) { false } let(:options) { "--HEAD" } @@ -104,8 +104,7 @@ ENV.delete("HOMEBREW_NO_ANALYTICS") ENV["HOMEBREW_ANALYTICS_DEBUG"] = "true" expect(described_class).to receive(:deferred_curl).once - described_class.report_influx(:install, package_name: package_name, tap_name: tap_name, on_request: on_request, -options: options) + described_class.report_influx(:install, { on_request: }, { package:, tap_name: }) end end @@ -116,13 +115,13 @@ it "reports event if BuildError raised for a formula with a public remote repository" do allow_any_instance_of(Tap).to receive(:custom_remote?).and_return(false) - expect(described_class).to respond_to(:report_event) + expect(described_class).to respond_to(:report_package_event) described_class.report_build_error(err) end it "does not report event if BuildError raised for a formula with a private remote repository" do allow_any_instance_of(Tap).to receive(:private?).and_return(true) - expect(described_class).not_to receive(:report_event) + expect(described_class).not_to receive(:report_package_event) described_class.report_build_error(err) end end @@ -132,7 +131,7 @@ let(:f) { instance_double(Formula, name: "foo", path: "blah", tap: nil) } it "does not report event if BuildError is raised" do - expect(described_class).not_to receive(:report_event) + expect(described_class).not_to receive(:report_package_event) described_class.report_build_error(err) end end @@ -143,7 +142,7 @@ it "does not report event if BuildError is raised" do allow_any_instance_of(Pathname).to receive(:directory?).and_return(false) - expect(described_class).not_to receive(:report_event) + expect(described_class).not_to receive(:report_package_event) described_class.report_build_error(err) end end diff --git a/Library/Homebrew/utils/analytics.rb b/Library/Homebrew/utils/analytics.rb index 35c7f80a0de94..2e7d7b323a3a6 100644 --- a/Library/Homebrew/utils/analytics.rb +++ b/Library/Homebrew/utils/analytics.rb @@ -20,33 +20,28 @@ class << self include Context sig { - params(measurement: Symbol, package_name: String, tap_name: String, on_request: T::Boolean, - options: String).void + params(measurement: Symbol, + tags: T::Hash[Symbol, T.any(T::Boolean, String)], + fields: T::Hash[Symbol, T.any(T::Boolean, String)]).void } - def report_influx(measurement, package_name:, tap_name:, on_request:, options:) - # ensure on_request is a boolean - on_request = on_request ? true : false - - # ensure options are removed (by `.compact` below) if empty - options = nil if options.blank? + def report_influx(measurement, tags, fields) + return if not_this_run? || disabled? # Tags are always implicitly strings and must have low cardinality. - tags = default_tags_influx.merge(on_request: on_request) - .map { |k, v| "#{k}=#{v}" } - .join(",") + tags_string = tags.map { |k, v| "#{k}=#{v}" } + .join(",") # Fields need explicitly wrapped with quotes and can have high cardinality. - fields = default_fields_influx.merge(package: package_name, tap_name: tap_name, options: options) - .compact - .map { |k, v| %Q(#{k}="#{v}") } - .join(",") + fields_string = fields.compact + .map { |k, v| %Q(#{k}="#{v}") } + .join(",") args = [ "--max-time", "3", "--header", "Authorization: Token #{INFLUX_TOKEN}", "--header", "Content-Type: text/plain; charset=utf-8", "--header", "Accept: application/json", - "--data-binary", "#{measurement},#{tags} #{fields} #{Time.now.to_i}" + "--data-binary", "#{measurement},#{tags_string} #{fields_string} #{Time.now.to_i}" ] # Second precision is highest we can do and has the lowest performance cost. @@ -72,29 +67,27 @@ def deferred_curl(url, args) params(measurement: Symbol, package_name: String, tap_name: String, on_request: T::Boolean, options: String).void } - def report_event(measurement, package_name:, tap_name:, on_request:, options: "") - report_influx_event(measurement, package_name: package_name, tap_name: tap_name, on_request: on_request, -options: options) - end - - sig { - params(measurement: Symbol, package_name: String, tap_name: String, on_request: T::Boolean, - options: String).void - } - def report_influx_event(measurement, package_name:, tap_name:, on_request: false, options: "") + def report_package_event(measurement, package_name:, tap_name:, on_request: false, options: "") return if not_this_run? || disabled? - report_influx(measurement, package_name: package_name, tap_name: tap_name, on_request: on_request, -options: options) - end + # ensure on_request is a boolean + on_request = on_request ? true : false - sig { params(exception: BuildError).void } - def report_build_error(exception) - report_influx_error(exception) + # ensure options are removed (by `.compact` below) if empty + options = nil if options.blank? + + # Tags must have low cardinality. + tags = default_package_tags.merge(on_request: on_request) + + # Fields can have high cardinality. + fields = default_package_fields.merge(package: package_name, tap_name: tap_name, options: options) + .compact + + report_influx(measurement, tags, fields) end sig { params(exception: BuildError).void } - def report_influx_error(exception) + def report_build_error(exception) return if not_this_run? || disabled? formula = exception.formula @@ -105,7 +98,7 @@ def report_influx_error(exception) return unless tap.should_report_analytics? options = exception.options.to_a.map(&:to_s).join(" ") - report_influx_event(:build_error, package_name: formula.name, tap_name: tap.name, options: options) + report_package_event(:build_error, package_name: formula.name, tap_name: tap.name, options: options) end def influx_message_displayed? @@ -285,13 +278,13 @@ def cask_output(cask, args:) end def clear_cache - remove_instance_variable(:@default_tags_influx) if instance_variable_defined?(:@default_tags_influx) - remove_instance_variable(:@default_fields_influx) if instance_variable_defined?(:@default_fields_influx) + remove_instance_variable(:@default_package_tags) if instance_variable_defined?(:@default_package_tags) + remove_instance_variable(:@default_package_fields) if instance_variable_defined?(:@default_package_fields) end sig { returns(T::Hash[Symbol, String]) } - def default_tags_influx - @default_tags_influx ||= begin + def default_package_tags + @default_package_tags ||= begin # Only display default prefixes to reduce cardinality and improve privacy prefix = Homebrew.default_prefix? ? HOMEBREW_PREFIX.to_s : "custom-prefix" @@ -311,8 +304,8 @@ def default_tags_influx # remove os_version starting with " or number # remove macOS patch release sig { returns(T::Hash[Symbol, String]) } - def default_fields_influx - @default_fields_influx ||= begin + def default_package_fields + @default_package_fields ||= begin version = if (match_data = HOMEBREW_VERSION.match(/^[\d.]+/)) suffix = "-dev" if HOMEBREW_VERSION.include?("-") match_data[0] + suffix.to_s