-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SHACK-188] telemetry cleanup tasks #134
Conversation
fe38dbe
to
b24385b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All about those improvements!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of questions and suggestions.
run_timestamp: run_timestamp, | ||
host_platform: host_platform, | ||
event_data: data | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this sort of user output something that should be defined in the i18n data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, just a log entry. The user need not know or care about this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OOOOOH, yea. LOG. Right.
if ChefCLI::Telemeter.enabled? | ||
# dev mode telemetry gets sent to a different location | ||
if ChefCLI::Config.telemetry.dev && !ENV.has_key?("CHEF_TELEMETRY_ENDPOINT") | ||
ENV["CHEF_TELEMETRY_ENDPOINT"] = "https://telemetry-acceptance.chef.io" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about an or-equals here instead of the longer if &&
statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also should test the case for when the env var is set and doesn't get overriden:
Maybe:
it "uses an endpoint the user has set in the environment" do
allow(ENV).to receive(:[]).with("CHEF_TELEMETRY_ENDPOINT").and_return("http://localhost:5432")
subject.run
expect(ENV["CHEF_TELEMETRY_ENDPOINT"]).to eq "http://localhost:5432"
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
3761cb6
to
4c8603e
Compare
* 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>
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>
Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>
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>
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>
…sent Signed-off-by: Marc A. Paradise <marc.paradise@gmail.com>
4c8603e
to
af340d8
Compare
Updated with review comment fixes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several cleanup tasks:
Telemeter::enabled?
Individual commit messages have some more details.