Skip to content
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

Prompt telemetry #170

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/README.rst
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ To get started, after installation run the following:
Change Log
==========

7.0.2
----------
- Prompt on first use for telemetry. Update code for 7.0.2 release (#170)

7.0.1
----------
- Fix bug where an empty directory is generated in the current location. Update code for 7.0.1 release (#167)
Expand Down
2 changes: 1 addition & 1 deletion src/setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def read(fname):

setup(
name='sfctl',
version='7.0.1',
version='7.0.2',
description='Azure Service Fabric command line',
long_description=read('README.rst'),
url='https://github.com/Azure/service-fabric-cli',
Expand Down
14 changes: 9 additions & 5 deletions src/sfctl/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ def get_config_value(name, fallback=None):


def get_config_bool(name, fallback=False):
"""Checks if a config value is set to a valid bool value."""
"""Checks if a config value is set to a valid bool value.
Exception will be raised if the value is not convertible to True or False."""

cli_config = CLIConfig(SF_CLI_CONFIG_DIR, SF_CLI_ENV_VAR_PREFIX)
return cli_config.getboolean('servicefabric', name, fallback)
Expand Down Expand Up @@ -188,18 +189,21 @@ def set_telemetry_config(telemetry_on):
:return: None
"""
if telemetry_on:
set_config_value('use_telemetry', 'true')
set_config_value('use_telemetry_v2', 'true')
else:
set_config_value('use_telemetry', 'false')
set_config_value('use_telemetry_v2', 'false')


def get_telemetry_config():
"""
Gets whether or not telemetry is turned on
Returns True if no value is set.
:return: bool. True if telemetry is on. False otherwise.
:return: bool. True if telemetry is on. False otherwise. Return None if no values are found.
"""
return get_config_bool('use_telemetry', fallback=True)
if get_config_value('use_telemetry_v2') is None:
return None

return get_config_bool('use_telemetry_v2')


def get_cli_version_from_pkg():
Expand Down
2 changes: 1 addition & 1 deletion src/sfctl/custom_cluster.py
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ def sfctl_cluster_version_matches(cluster_version, sfctl_version):
:return: True if they are a match. False otherwise.
"""

if sfctl_version in ['7.0.0', '7.0.1']:
if sfctl_version in ['7.0.0', '7.0.1', '7.0.2']:

return cluster_version.startswith('6.4')

Expand Down
2 changes: 1 addition & 1 deletion src/sfctl/helps/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
short-summary: Turn off telemetry.
- name: --on
type: bool
short-summary: Turn on telemetry. This is the default value.
short-summary: Turn on telemetry.


examples:
Expand Down
56 changes: 54 additions & 2 deletions src/sfctl/telemetry.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from uuid import uuid4
import portalocker
from knack.log import get_logger
from sfctl.config import get_telemetry_config, get_cli_version_from_pkg
from sfctl.config import get_telemetry_config, set_telemetry_config, get_cli_version_from_pkg
from sfctl.state import increment_telemetry_send_retry_count

# knack CLIConfig has been re-purposed to handle state instead.
Expand Down Expand Up @@ -49,8 +49,10 @@ def check_and_send_telemetry(args_list, invocation_ret_val, exception=None):
:return: None
"""

use_telemetry = get_telemetry_config()

# Only send telemetry if user has configured it to be on
if get_telemetry_config():
if use_telemetry:

try:

Expand Down Expand Up @@ -85,6 +87,56 @@ def check_and_send_telemetry(args_list, invocation_ret_val, exception=None):
logger.info(
str.format('Not sending telemetry because python process due to error: {0}', ex))

# Prompt the user if there is no value set in the config file for whether or not telemetry
# should be sent. If use telemetry is set to False, then do nothing.
elif use_telemetry is None:

_prompt_for_telemetry(args_list, invocation_ret_val, exception)


def _prompt_for_telemetry(args_list, invocation_ret_val, exception):
"""
Ask users for a yes or no answer on collecting telemetry.

If the user says yes, set the value in config, then re-run the check_and_send_telemetry check.
If the user says no, set the value in config and return.

Inputs are the ones passed into check_and_send_telemetry. This just passes the information
along.

:return: None
"""

user_response = None

while user_response not in ['y', 'yes', 'n', 'no']:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does knack have any prompting features we should be using for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. Will look it up before merging this change in - this change will not be part of 7.1.0

Thanks!

if user_response is None: # Means this is the first time asking for the prompt
prompt = str.format('Hello! In order for us to improve the user experience, '
'we would like to collect some data. Sfctl telemetry collects '
'command name without parameters '
'provided or their values, sfctl version, OS type, python version, '
'the success or failure of the command, the error message '
'returned.{0}'
'To give permission, enter "y" or "yes". '
'To decline, enter "n" or "no": ',
os.linesep)
else:
prompt = ('Invalid response provided. Please enter "y" or "yes" to accept, '
'and "n" or "no" to decline: ')

try: # raw_input is for python 2.x
user_response = raw_input(prompt)
except NameError:
user_response = input(prompt)

if user_response.lower() in ['y', 'yes']:
set_telemetry_config(True)
# Re-run this function
check_and_send_telemetry(args_list, invocation_ret_val, exception)

elif user_response.lower() in ['n', 'no']:
set_telemetry_config(False)


def batch_or_send_telemetry(command_as_str, command_return):
"""
Expand Down
27 changes: 23 additions & 4 deletions src/sfctl/tests/request_generation_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

from __future__ import print_function
from os import (remove, path)
from random import choice
import json
import logging
from shutil import rmtree
Expand Down Expand Up @@ -215,18 +216,36 @@ def validate_command(self, command, method, url_path, query, body=None, # pylin
# If this test reaches here, then this test is successful.
remove(generated_file_path) # Clean up

@staticmethod
def _mock_raw_input():
random_yes_or_no = choice([True, False])
if random_yes_or_no:
return 'yes'

return 'no'

def test_paths_generation(self):
""" This test calls all commands that exist within sfctl.
The commands are routed to a mock cluster which always returns
success.We then read the values of the URL and other request
features to determine that the command is working as expected
(generating the correct URL). """

# Set test URL path to that of our mock server
set_mock_endpoint('http://localhost:' + str(self.port))
try: # Python 3
with patch('builtins.input', return_value=ServiceFabricRequestTests._mock_raw_input()):
# Set test URL path to that of our mock server
set_mock_endpoint('http://localhost:' + str(self.port))

# Call test
self.paths_generation_helper()

except NameError: # Python 2
with patch('__builtin__.raw_input', return_value=ServiceFabricRequestTests._mock_raw_input()):
# Set test URL path to that of our mock server
set_mock_endpoint('http://localhost:' + str(self.port))

# Call test
self.paths_generation_helper()
# Call test
self.paths_generation_helper()

def paths_generation_helper(self): # pylint: disable=too-many-statements, too-many-locals
""" Lists all the commands to be tested and their expected values.
Expand Down
2 changes: 1 addition & 1 deletion src/sfctl/tests/version_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def test_valid_current_version(self):

note: this will require changing the sfctl_version on releases
"""
sfctl_version = '7.0.1'
sfctl_version = '7.0.2'

pipe = Popen('sfctl --version', shell=True, stdout=PIPE, stderr=PIPE)
# returned_string and err are returned as bytes
Expand Down