Merge pull request #16846 from Homebrew/analytics_cleanup

utils/analytics: general cleanup.

Library/Homebrew/cask/installer.rb

@@ -112,7 +112,7 @@ module Cask
       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
 

Library/Homebrew/formula_installer.rb

@@ -419,7 +419,7 @@ class FormulaInstaller
     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
 

Library/Homebrew/test/utils/analytics_spec.rb

@@ -8,47 +8,47 @@ RSpec.describe Utils::Analytics do
     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_package_tags).to have_key(:prefix)
-      expect(described_class.default_tags_influx[:prefix]).to eq "custom-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_package_tags).to have_key(:arch)
-      expect(described_class.default_tags_influx[:arch]).to eq HOMEBREW_PHYSICAL_PROCESSOR
+      expect(described_class.default_package_tags[:arch]).to eq HOMEBREW_PHYSICAL_PROCESSOR
-      expect(described_class.default_tags_influx).to have_key(:prefix)
+      expect(described_class.default_package_tags).to have_key(:prefix)
-      expect(described_class.default_tags_influx[:prefix]).to eq "custom-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_package_tags).to have_key(:prefix)
-      expect(described_class.default_tags_influx[:prefix]).to eq "custom-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_package_tags).to have_key(:prefix)
-      expect(described_class.default_tags_influx[:prefix]).to eq HOMEBREW_PREFIX.to_s
+      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 @@ RSpec.describe Utils::Analytics do
       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 @@ RSpec.describe Utils::Analytics do
         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 @@ RSpec.describe Utils::Analytics do
       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,
+      expect(described_class).to receive(:report_influx).with(:install, hash_including(on_request:),
-                                                                                       on_request:   on_request)).once
+                                                              hash_including(package: package_name)).once
-      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
 
   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 @@ RSpec.describe Utils::Analytics do
       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,
+      described_class.report_influx(:install, { on_request: }, { package:, tap_name: })
-options: options)
     end
   end
 
@@ -116,13 +115,13 @@ options: options)
 
       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 @@ options: options)
       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 @@ options: options)
 
       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

Library/Homebrew/utils/analytics.rb

@@ -20,33 +20,28 @@ module Utils
       include Context
 
       sig {
-        params(measurement: Symbol, package_name: String, tap_name: String, on_request: T::Boolean,
+        params(measurement: Symbol,
-               options: String).void
+               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:)
+      def report_influx(measurement, tags, fields)
-        # ensure on_request is a boolean
+        return if not_this_run? || disabled?
-        on_request = on_request ? true : false
-
-        # ensure options are removed (by `.compact` below) if empty
-        options = nil if options.blank?
 
         # Tags are always implicitly strings and must have low cardinality.
-        tags = default_tags_influx.merge(on_request: on_request)
+        tags_string = tags.map { |k, v| "#{k}=#{v}" }
-                                  .map { |k, v| "#{k}=#{v}" }
+                          .join(",")
-                                  .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)
+        fields_string = fields.compact
-                                      .compact
+                              .map { |k, v| %Q(#{k}="#{v}") }
-                                      .map { |k, v| %Q(#{k}="#{v}") }
+                              .join(",")
-                                      .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 @@ module Utils
         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: "")
+      def report_package_event(measurement, package_name:, tap_name:, on_request: false, 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: "")
         return if not_this_run? || disabled?
 
-        report_influx(measurement, package_name: package_name, tap_name: tap_name, on_request: on_request,
+        # ensure on_request is a boolean
-options: options)
+        on_request = on_request ? true : false
+
+        # 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_build_error(exception)
-        report_influx_error(exception)
-      end
-
-      sig { params(exception: BuildError).void }
-      def report_influx_error(exception)
         return if not_this_run? || disabled?
 
         formula = exception.formula
@@ -105,7 +98,7 @@ options: options)
         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 @@ options: options)
       end
 
       def clear_cache
-        remove_instance_variable(:@default_tags_influx) if instance_variable_defined?(:@default_tags_influx)
+        remove_instance_variable(:@default_package_tags) if instance_variable_defined?(:@default_package_tags)
-        remove_instance_variable(:@default_fields_influx) if instance_variable_defined?(:@default_fields_influx)
+        remove_instance_variable(:@default_package_fields) if instance_variable_defined?(:@default_package_fields)
       end
 
       sig { returns(T::Hash[Symbol, String]) }
-      def default_tags_influx
+      def default_package_tags
-        @default_tags_influx ||= begin
+        @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 @@ options: options)
       # remove os_version starting with " or number
       # remove macOS patch release
       sig { returns(T::Hash[Symbol, String]) }
-      def default_fields_influx
+      def default_package_fields
-        @default_fields_influx ||= begin
+        @default_package_fields ||= begin
           version = if (match_data = HOMEBREW_VERSION.match(/^[\d.]+/))
             suffix = "-dev" if HOMEBREW_VERSION.include?("-")
             match_data[0] + suffix.to_s