From b85ec09cf0f1c07f04ed767791ad841b34f092f1 Mon Sep 17 00:00:00 2001
From: "Marc A. Paradise" <marc.paradise@gmail.com>
Date: Wed, 16 May 2018 11:54:59 -0400
Subject: [PATCH 1/6] Use acceptance endpoint when telemetry dev mode is set.

* when telemetry.dev is true, use the acceptance endpoint for telemetry
* when telemetry is disabled, just clean up the session files and don't
send.  Even though the telemetry gem would not actually submit them
when disabled, seeing workstation logs stating that we're sending
telemetry files when it's disabled could be easily misunderstood.

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>
---
 .../chef-cli/lib/chef-cli/telemeter/sender.rb | 15 +++++-
 .../spec/unit/telemeter/sender_spec.rb        | 46 +++++++++++++++++--
 2 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/components/chef-cli/lib/chef-cli/telemeter/sender.rb b/components/chef-cli/lib/chef-cli/telemeter/sender.rb
index d9286779a..f7f5274b3 100644
--- a/components/chef-cli/lib/chef-cli/telemeter/sender.rb
+++ b/components/chef-cli/lib/chef-cli/telemeter/sender.rb
@@ -7,7 +7,20 @@ module ChefCLI
   class Telemeter
     class Sender
       def run
-        session_files.each { |path| process_session(path) }
+        if ChefCLI::Telemeter.enabled?
+          # dev mode telemetry gets sent to a different location
+          if ChefCLI::Config.telemetry.dev
+            ENV["CHEF_TELEMETRY_ENDPOINT"] = "https://telemetry-acceptance.chef.io"
+          end
+          session_files.each { |path| process_session(path) }
+        else
+          # If telemetry is not enabled, just clean up and return. Even though
+          # the telemetry gem will not send if disabled, log output saying that we're submitting
+          # it when it has been disabled can be alarming.
+          ChefCLI::Log.info("Telemetry disabled, clearing any existing session captures without sending them.")
+          session_files.each { |path| FileUtils.rm_rf(path) }
+        end
+        FileUtils.rm_rf(ChefCLI::Config.telemetry_session_file)
         ChefCLI::Log.info("Terminating, nothing more to do.")
       end
 
diff --git a/components/chef-cli/spec/unit/telemeter/sender_spec.rb b/components/chef-cli/spec/unit/telemeter/sender_spec.rb
index 1a95ba637..515767329 100644
--- a/components/chef-cli/spec/unit/telemeter/sender_spec.rb
+++ b/components/chef-cli/spec/unit/telemeter/sender_spec.rb
@@ -20,15 +20,52 @@
 
 RSpec.describe ChefCLI::Telemeter::Sender do
   let(:subject) { ChefCLI::Telemeter::Sender.new }
+  let(:enabled_flag) { true }
+  let(:dev_mode) { false }
+  let(:config) { double("config") }
+
+  before do
+    allow(config).to receive(:dev).and_return dev_mode
+    allow(ChefCLI::Config).to receive(:telemetry).and_return config
+    allow(ChefCLI::Telemeter).to receive(:enabled?).and_return enabled_flag
+    # Ensure this is not set for each test:
+    ENV.delete("CHEF_TELEMETRY_ENDPOINT")
+  end
+
   describe "#run" do
     let(:session_files) { %w{file1 file2} }
-    it "submits the session capture for each session file found" do
+    before do
       expect(subject).to receive(:session_files).and_return session_files
-      expect(subject).to receive(:process_session).with("file1")
-      expect(subject).to receive(:process_session).with("file2")
-      subject.run
     end
 
+    context "when telemetry is disabled" do
+      let(:enabled_flag) { false }
+      it "deletes session files without sending" do
+        expect(FileUtils).to receive(:rm_rf).with("file1")
+        expect(FileUtils).to receive(:rm_rf).with("file2")
+        expect(FileUtils).to receive(:rm_rf).with(ChefCLI::Config.telemetry_session_file)
+        expect(subject).to_not receive(:process_session)
+        subject.run
+      end
+    end
+
+    context "when telemetry is enabled" do
+      context "and telemetry dev mode is true" do
+        let(:dev_mode) { true }
+        let(:session_files) { [] } # Ensure we don't send anything without mocking :allthecalls:
+        it "configures the environment to submit to the Acceptance telemetry endpoint" do
+          subject.run
+          expect(ENV["CHEF_TELEMETRY_ENDPOINT"]).to eq "https://telemetry-acceptance.chef.io"
+        end
+      end
+
+      it "submits the session capture for each session file found" do
+        expect(subject).to receive(:process_session).with("file1")
+        expect(subject).to receive(:process_session).with("file2")
+        expect(FileUtils).to receive(:rm_rf).with(ChefCLI::Config.telemetry_session_file)
+        subject.run
+      end
+    end
   end
 
   describe "#session_files" do
@@ -66,5 +103,4 @@
       subject.submit_entry(telemetry, { "test" => "this" }, 1, 1)
     end
   end
-
 end

From 23a589fe15b26a3273da0cd2b4d51e91aac02a14 Mon Sep 17 00:00:00 2001
From: "Marc A. Paradise" <marc.paradise@gmail.com>
Date: Wed, 16 May 2018 12:01:01 -0400
Subject: [PATCH 2/6] Remove dev-mode tracking

Since we're now using a different endpoint to submit to when in dev
mode, we don't need to capture telemetry_mode = dev|prod in the
telemetry data.

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>
---
 components/chef-cli/lib/chef-cli/telemeter.rb   | 3 ---
 components/chef-cli/spec/unit/telemeter_spec.rb | 5 -----
 2 files changed, 8 deletions(-)

diff --git a/components/chef-cli/lib/chef-cli/telemeter.rb b/components/chef-cli/lib/chef-cli/telemeter.rb
index b4704a76e..df59d7554 100644
--- a/components/chef-cli/lib/chef-cli/telemeter.rb
+++ b/components/chef-cli/lib/chef-cli/telemeter.rb
@@ -96,9 +96,6 @@ def make_event_payload(name, data)
       properties = {
         # We will submit this payload in a future run, so capture the time of actual execution:
         run_timestamp: run_timestamp,
-        # This lets us filter out testing/dev actions, which may not
-        # follow customer usage patterns:
-        telemetry_mode:  ChefCLI::Config.telemetry.dev ? "dev" : "prod",
         host_platform: host_platform,
       }
       { event: name, properties: properties.merge(data) }
diff --git a/components/chef-cli/spec/unit/telemeter_spec.rb b/components/chef-cli/spec/unit/telemeter_spec.rb
index f9a3ff8bf..63f999161 100644
--- a/components/chef-cli/spec/unit/telemeter_spec.rb
+++ b/components/chef-cli/spec/unit/telemeter_spec.rb
@@ -19,13 +19,9 @@
 
 RSpec.describe ChefCLI::Telemeter do
   subject { ChefCLI::Telemeter.instance }
-  let(:dev_mode) { true }
-  let(:config) { double("config") }
   let(:host_platform) { "linux" }
 
   before do
-    allow(config).to receive(:dev).and_return dev_mode
-    allow(ChefCLI::Config).to receive(:telemetry).and_return config
     allow(subject).to receive(:host_platform).and_return host_platform
   end
 
@@ -146,7 +142,6 @@
         payload = subject.make_event_payload(:run, { hello: "world" })
         expect(payload[:event]).to eq :run
         props = payload[:properties]
-        expect(props[:telemetry_mode]).to eq "dev"
         expect(props[:host_platform]).to eq host_platform
         expect(props[:run_timestamp]).to_not eq nil
         expect(props[:hello]).to eq "world"

From be4139909a1af69969b4a245035847fd4a55eca8 Mon Sep 17 00:00:00 2001
From: "Marc A. Paradise" <marc.paradise@gmail.com>
Date: Wed, 16 May 2018 12:04:31 -0400
Subject: [PATCH 3/6] Add test coverage for Telemeter::enabled?

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>
---
 .../chef-cli/spec/unit/telemeter_spec.rb      | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/components/chef-cli/spec/unit/telemeter_spec.rb b/components/chef-cli/spec/unit/telemeter_spec.rb
index 63f999161..26523dc28 100644
--- a/components/chef-cli/spec/unit/telemeter_spec.rb
+++ b/components/chef-cli/spec/unit/telemeter_spec.rb
@@ -99,6 +99,38 @@
     end
   end
 
+  context "::enabled?" do
+    let(:enabled_flag) { false }
+    let(:config) { double("config") }
+    before do
+      allow(ChefCLI::Config).to receive(:telemetry).and_return(config)
+      allow(config).to receive(:enable).and_return(enabled_flag)
+    end
+
+    context "when config value is enabled" do
+      let(:enabled_flag) { true }
+      context "and CHEF_TELEMETRY_OPT_OUT is not present in env vars" do
+        it "returns false" do
+          ENV.delete("CHEF_TELEMETRY_OPT_OUT")
+          expect(subject.enabled?).to eq true
+        end
+      end
+      context "and CHEF_TELEMETRY_OPT_OUT is present in env vars" do
+        it "returns false" do
+          ENV["CHEF_TELEMETRY_OPT_OUT"] = ""
+          expect(subject.enabled?).to eq false
+        end
+      end
+    end
+
+    context "when config value is disabled" do
+      let(:enabled_flag) { false }
+      it "returns false" do
+        expect(subject.enabled?).to eq false
+      end
+    end
+  end
+
   context "#timed_run_capture" do
     it "invokes timed_capture with run data" do
       expected_data = { arguments: [ "arg1" ] }

From ca739f2b088dc0fd9ac6e30cb42da69f47f541a8 Mon Sep 17 00:00:00 2001
From: "Marc A. Paradise" <marc.paradise@gmail.com>
Date: Wed, 16 May 2018 12:07:55 -0400
Subject: [PATCH 4/6] Send the correct version to Telemetry

Capture the version of chef-cli in the telemetry session data,
so that we can be sure we're sending the right version to telemetry

This prevents the following scenario:
* customer runs command under version 1.0.1
* customer upgrades
* customer runs command under version 1.0.2
  * telemetry submits previous 1.0.1 session as version 1.0.2

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>
---
 components/chef-cli/lib/chef-cli/telemeter.rb          | 3 ++-
 components/chef-cli/lib/chef-cli/telemeter/sender.rb   | 7 +++++--
 components/chef-cli/spec/unit/telemeter/sender_spec.rb | 7 ++++---
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/components/chef-cli/lib/chef-cli/telemeter.rb b/components/chef-cli/lib/chef-cli/telemeter.rb
index df59d7554..e94008e45 100644
--- a/components/chef-cli/lib/chef-cli/telemeter.rb
+++ b/components/chef-cli/lib/chef-cli/telemeter.rb
@@ -122,7 +122,8 @@ def host_platform
     end
 
     def convert_events_to_session
-      YAML.dump({ "entries" => @events_to_send })
+      YAML.dump({ "version" => ChefCLI::VERSION,
+                  "entries" => @events_to_send })
     end
 
     def write_session(session)
diff --git a/components/chef-cli/lib/chef-cli/telemeter/sender.rb b/components/chef-cli/lib/chef-cli/telemeter/sender.rb
index f7f5274b3..98191d091 100644
--- a/components/chef-cli/lib/chef-cli/telemeter/sender.rb
+++ b/components/chef-cli/lib/chef-cli/telemeter/sender.rb
@@ -43,11 +43,14 @@ def submit_session(content)
         # Each run is one session, so we'll first remove remove the session file
         # to force creating a new one.
         FileUtils.rm_rf(ChefCLI::Config.telemetry_session_file)
+        # We'll use the version captured in the sesion file
+        entries = content["entries"]
+        cli_version = content["version"]
+        total = entries.length
         telemetry = Telemetry.new(product: "chef-workstation-cli",
                                   origin: "command-line",
-                                  product_version: ChefCLI::VERSION,
+                                  product_version: cli_version,
                                   install_context: "omnibus")
-        entries = content["entries"]
         total = entries.length
         entries.each_with_index do |entry, x|
           submit_entry(telemetry, entry, x + 1, total)
diff --git a/components/chef-cli/spec/unit/telemeter/sender_spec.rb b/components/chef-cli/spec/unit/telemeter/sender_spec.rb
index 515767329..5d15aa405 100644
--- a/components/chef-cli/spec/unit/telemeter/sender_spec.rb
+++ b/components/chef-cli/spec/unit/telemeter/sender_spec.rb
@@ -78,8 +78,8 @@
 
   describe "process_session" do
     it "loads the sesion and submits it" do
-      expect(subject).to receive(:load_and_clear_session).with("file1").and_return({ "entries" => [] })
-      expect(subject).to receive(:submit_session).with({ "entries" => [] })
+      expect(subject).to receive(:load_and_clear_session).with("file1").and_return({ "version" => "1.0.0", "entries" => [] })
+      expect(subject).to receive(:submit_session).with({ "version" => "1.0.0", "entries" => [] })
       subject.process_session("file1")
     end
   end
@@ -92,7 +92,8 @@
       expect(Telemetry).to receive(:new).and_return telemetry
       expect(subject).to receive(:submit_entry).with(telemetry, { "event" => "action1" }, 1, 2)
       expect(subject).to receive(:submit_entry).with(telemetry, { "event" => "action2" }, 2, 2)
-      subject.submit_session( { "entries" => [ { "event" => "action1" }, { "event" => "action2" } ] } )
+      subject.submit_session( { "version" => "1.0.0",
+                                "entries" => [ { "event" => "action1" }, { "event" => "action2" } ] } )
     end
   end
 

From c2e307a7c40eaf9ccc060c24d46bee370d9dde56 Mon Sep 17 00:00:00 2001
From: "Marc A. Paradise" <marc.paradise@gmail.com>
Date: Wed, 16 May 2018 14:25:39 -0400
Subject: [PATCH 5/6] Add a unique installation_id to telemetry

Every event will now include a unique installation_id, which is a GUID
we generate at the same time as other first-run tasks. We store it in
CHefCLI::Config.telemetry_installation_identiifer_file ->
.chef-workstation/installation_id.

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>
---
 components/chef-cli/lib/chef-cli/cli.rb       | 28 +++++++++++++------
 components/chef-cli/lib/chef-cli/config.rb    |  4 +++
 components/chef-cli/lib/chef-cli/telemeter.rb | 25 +++++++++++++----
 .../chef-cli/spec/unit/telemeter_spec.rb      | 24 ++++++++++------
 4 files changed, 59 insertions(+), 22 deletions(-)

diff --git a/components/chef-cli/lib/chef-cli/cli.rb b/components/chef-cli/lib/chef-cli/cli.rb
index 5233a7f43..3755fd114 100644
--- a/components/chef-cli/lib/chef-cli/cli.rb
+++ b/components/chef-cli/lib/chef-cli/cli.rb
@@ -79,22 +79,34 @@ def setup_cli
       # Creates the tree we need under ~/.chef-workstation
       # based on config settings:
       Config.create_directory_tree
-      # TODO because we have not loaded a command, we wil lalways be using
+
+      # TODO because we have not loaded a command, we will always be using
       #      the default location at this step.
       if Config.using_default_location? && !Config.exist?
-        UI::Terminal.output T.creating_config(Config.default_location)
-        Config.create_default_config_file
-        # Tell the user we're anonymously tracking, give brief opt-out
-        # and a link to detailed information.
-        UI::Terminal.output ""
-        UI::Terminal.output T.telemetry_enabled(Config.default_location)
-        UI::Terminal.output ""
+        setup_workstation
       end
+
       Config.load
       ChefCLI::Log.setup(Config.log.location, Config.log.level.to_sym)
       ChefCLI::Log.info("Initialized logger")
     end
 
+    # This setup command is run if ".chef-workstation" is missing prior to
+    # the run.  It will set up default configuration, generated an installation id
+    # for telemetry, and report telemetry & config info to the operator.
+    def setup_workstation
+      require "securerandom"
+      installation_id = SecureRandom.uuid
+      File.write(Config.telemetry_installation_identifier_file, installation_id)
+      UI::Terminal.output T.creating_config(Config.default_location)
+      Config.create_default_config_file
+      # Tell the user we're anonymously tracking, give brief opt-out
+      # and a link to detailed information.
+      UI::Terminal.output ""
+      UI::Terminal.output T.telemetry_enabled(Config.default_location)
+      UI::Terminal.output ""
+    end
+
     def perform_command
       update_args_for_help
       update_args_for_version
diff --git a/components/chef-cli/lib/chef-cli/config.rb b/components/chef-cli/lib/chef-cli/config.rb
index 6caf13c3b..bff3c1533 100644
--- a/components/chef-cli/lib/chef-cli/config.rb
+++ b/components/chef-cli/lib/chef-cli/config.rb
@@ -35,6 +35,10 @@ def telemetry_session_file
         File.join(telemetry_path, "TELEMETRY_SESSION_ID")
       end
 
+      def telemetry_installation_identifier_file
+        File.join(WS_BASE_PATH, "installation_id")
+      end
+
       def error_output_path
         File.join(File.dirname(log.location), "errors.txt")
       end
diff --git a/components/chef-cli/lib/chef-cli/telemeter.rb b/components/chef-cli/lib/chef-cli/telemeter.rb
index e94008e45..bb27dfe97 100644
--- a/components/chef-cli/lib/chef-cli/telemeter.rb
+++ b/components/chef-cli/lib/chef-cli/telemeter.rb
@@ -32,6 +32,8 @@ module ChefCLI
   # a main 'timed_capture', and it would be good to see ordering within nested calls.
   class Telemeter
     include Singleton
+    DEFAULT_INSTALLATION_GUID = "00000000-0000-0000-0000-000000000000"
+
     class << self
       extend Forwardable
       def_delegators :instance, :timed_capture, :capture, :commit, :timed_action_capture, :timed_run_capture
@@ -93,12 +95,25 @@ def commit
     end
 
     def make_event_payload(name, data)
-      properties = {
-        # We will submit this payload in a future run, so capture the time of actual execution:
-        run_timestamp: run_timestamp,
-        host_platform: host_platform,
+      {
+        event: name,
+        properties: {
+          installation_id: installation_id,
+          run_timestamp: run_timestamp,
+          host_platform: host_platform,
+          event_data: data
+        }
       }
-      { event: name, properties: properties.merge(data) }
+    end
+
+    def installation_id
+      @installation_id ||=
+        begin
+          File.read(ChefCLI::Config.telemetry_installation_identifier_file).chomp
+        rescue
+          ChefCLI::Log.info "could not read #{ChefCLI::Config.telemetry_installation_identifier_file} - using default id"
+          DEFAULT_INSTALLATION_GUID
+        end
     end
 
     # For testing.
diff --git a/components/chef-cli/spec/unit/telemeter_spec.rb b/components/chef-cli/spec/unit/telemeter_spec.rb
index 26523dc28..3286158fc 100644
--- a/components/chef-cli/spec/unit/telemeter_spec.rb
+++ b/components/chef-cli/spec/unit/telemeter_spec.rb
@@ -169,16 +169,22 @@
   end
 
   context "#make_event_payload" do
-    context "when event is ':run'" do
-      it "adds expected properties" do
-        payload = subject.make_event_payload(:run, { hello: "world" })
-        expect(payload[:event]).to eq :run
-        props = payload[:properties]
-        expect(props[:host_platform]).to eq host_platform
-        expect(props[:run_timestamp]).to_not eq nil
-        expect(props[:hello]).to eq "world"
-      end
+    before do
+      allow(subject).to receive(:installation_id).and_return "0000"
     end
 
+    it "adds expected properties" do
+      payload = subject.make_event_payload(:run, { hello: "world" })
+      expected_payload = {
+        event: :run,
+        properties: {
+          installation_id: "0000",
+          run_timestamp: subject.run_timestamp,
+          host_platform: host_platform,
+          event_data:  { hello: "world" }
+        }
+      }
+      expect(payload).to eq expected_payload
+    end
   end
 end

From af340d82121c583fe4bef3b4d9bbf0c24be3e2e1 Mon Sep 17 00:00:00 2001
From: "Marc A. Paradise" <marc.paradise@gmail.com>
Date: Wed, 16 May 2018 14:29:32 -0400
Subject: [PATCH 6/6] Do not override existing CHEF_TELEMETRY_ENDPOINT env var
 if it is present

Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>
---
 .../chef-cli/lib/chef-cli/telemeter/sender.rb  |  2 +-
 .../spec/unit/telemeter/sender_spec.rb         | 18 +++++++++++++++---
 2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/components/chef-cli/lib/chef-cli/telemeter/sender.rb b/components/chef-cli/lib/chef-cli/telemeter/sender.rb
index 98191d091..c272e290a 100644
--- a/components/chef-cli/lib/chef-cli/telemeter/sender.rb
+++ b/components/chef-cli/lib/chef-cli/telemeter/sender.rb
@@ -10,7 +10,7 @@ def run
         if ChefCLI::Telemeter.enabled?
           # dev mode telemetry gets sent to a different location
           if ChefCLI::Config.telemetry.dev
-            ENV["CHEF_TELEMETRY_ENDPOINT"] = "https://telemetry-acceptance.chef.io"
+            ENV["CHEF_TELEMETRY_ENDPOINT"] ||= "https://telemetry-acceptance.chef.io"
           end
           session_files.each { |path| process_session(path) }
         else
diff --git a/components/chef-cli/spec/unit/telemeter/sender_spec.rb b/components/chef-cli/spec/unit/telemeter/sender_spec.rb
index 5d15aa405..9e673af5a 100644
--- a/components/chef-cli/spec/unit/telemeter/sender_spec.rb
+++ b/components/chef-cli/spec/unit/telemeter/sender_spec.rb
@@ -53,9 +53,21 @@
       context "and telemetry dev mode is true" do
         let(:dev_mode) { true }
         let(:session_files) { [] } # Ensure we don't send anything without mocking :allthecalls:
-        it "configures the environment to submit to the Acceptance telemetry endpoint" do
-          subject.run
-          expect(ENV["CHEF_TELEMETRY_ENDPOINT"]).to eq "https://telemetry-acceptance.chef.io"
+        context "and a custom telemetry endpoint is not set" do
+          it "configures the environment to submit to the Acceptance telemetry endpoint" do
+            subject.run
+            expect(ENV["CHEF_TELEMETRY_ENDPOINT"]).to eq "https://telemetry-acceptance.chef.io"
+          end
+        end
+
+        context "and a custom telemetry endpoint is already set" do
+          before do
+            ENV["CHEF_TELEMETRY_ENDPOINT"] = "https://localhost"
+          end
+          it "should not overwrite the custom value" do
+            subject.run
+            expect(ENV["CHEF_TELEMETRY_ENDPOINT"]).to eq "https://localhost"
+          end
         end
       end