Skip to content

fix: More robust connections to telemetry backend #1289

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.

Sign up for GitHub

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

Merged
sanathkr merged 1 commit into from
Jul 25, 2019
Merged

fix: More robust connections to telemetry backend #1289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 4 additions & 3 deletions samcli/lib/telemetry/telemetry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,16 +78,17 @@ def _send(self, metric, wait_for_response=False):
payload = {"metrics": [metric]}
LOG.debug("Sending Telemetry: %s", payload)

timeout_ms = 2000 if wait_for_response else 1 # 2 seconds to wait for response or 1ms
timeout_ms = 2000 if wait_for_response else 100 # 2 seconds to wait for response or 100ms

timeout = (2, # connection timeout. Always set to 2 seconds
timeout_ms / 1000.0 # Read timeout. Tweaked based on input.
)
try:
r = requests.post(self._url, json=payload, timeout=timeout)
LOG.debug("Telemetry response: %d", r.status_code)
except requests.exceptions.Timeout as ex:
# Expected if request times out. Just print debug log and ignore the exception.
except (requests.exceptions.Timeout, requests.exceptions.ConnectionError) as ex:
# Expected if request times out OR cannot connect to the backend (offline).
# Just print debug log and ignore the exception.
LOG.debug(str(ex))

def _add_common_metric_attributes(self, attrs):
Expand Down
7 changes: 5 additions & 2 deletions tests/integration/telemetry/integ_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,17 @@ def base_command(cls):

return command

def run_cmd(self, stdin_data=""):
def run_cmd(self, stdin_data="", optout_envvar_value=None):
# Any command will work for this test suite
cmd_list = [self.cmd, "local", "generate-event", "s3", "put"]

env = os.environ.copy()

# remove the envvar which usually is set in Travis. This interferes with tests.
# remove the envvar which usually is set in Travis. This interferes with tests
env.pop("SAM_CLI_TELEMETRY", None)
if optout_envvar_value:
# But if the caller explicitly asked us to opt-out via EnvVar, then set it here
env["SAM_CLI_TELEMETRY"] = optout_envvar_value

env["__SAM_CLI_APP_DIR"] = self.config_dir
env["__SAM_CLI_TELEMETRY_ENDPOINT_URL"] = "{}/metrics".format(TELEMETRY_ENDPOINT_URL)
Expand Down
73 changes: 73 additions & 0 deletions tests/integration/telemetry/test_telemetry_contract.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@

from .integ_base import IntegBase, TelemetryServer


class TestTelemetryContract(IntegBase):
"""
Validates the basic tenets/contract Telemetry module needs to adhere to
"""

def test_must_not_send_metrics_if_disabled_using_envvar(self):
"""
No metrics should be sent if "Enabled via Config file but Disabled via Envvar"
"""
# Enable it via configuration file
self.set_config(telemetry_enabled=True)
Copy link
Contributor

@jfuss jfuss Jul 25, 2019

Choose a reason for hiding this comment

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

Personally, I am not crazy about setting this config through code, as these are integ tests and should be set how customers would enable/disable. I am ok with it, just would prefer always going through customer paths in our integ tests.

Copy link
Contributor Author

@sanathkr sanathkr Jul 25, 2019

Choose a reason for hiding this comment

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

This is how customers would enable telemetry. Thru the config.

Copy link
Contributor

@jfuss jfuss Jul 25, 2019

Choose a reason for hiding this comment

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

There is a sight nuance to me though. This function gets called through command executions for the customers. The customer would never explicitly call this method. I know it’s what happens for them but since a customer never directly interacts with it, calling the method in an integ test looks off to me. Not blocking on this, just something I noticed

Copy link
Contributor Author

@sanathkr sanathkr Jul 25, 2019

Choose a reason for hiding this comment

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

Sure, but we need some way of mimicing customer's environment. Calling this method is a way to do that


with TelemetryServer() as server:
# Start the CLI, but opt-out of Telemetry using env var
process = self.run_cmd(optout_envvar_value="0")
(_, stderrdata) = process.communicate()
retcode = process.poll()
self.assertEquals(retcode, 0, "Command should successfully complete")
all_requests = server.get_all_requests()
self.assertEquals(0, len(all_requests), "No metrics should be sent")

# Now run again without the Env Var Opt out
process = self.run_cmd()
(_, stderrdata) = process.communicate()
retcode = process.poll()
self.assertEquals(retcode, 0, "Command should successfully complete")
all_requests = server.get_all_requests()
self.assertEquals(1, len(all_requests), "Command run metric should be sent")

def test_must_send_metrics_if_enabled_via_envvar(self):
"""
Metrics should be sent if "Disabled via config file but Enabled via Envvar"
"""
# Disable it via configuration file
self.set_config(telemetry_enabled=False)

with TelemetryServer() as server:
# Run without any envvar.Should not publish metrics
process = self.run_cmd()
(_, stderrdata) = process.communicate()
retcode = process.poll()
self.assertEquals(retcode, 0, "Command should successfully complete")
all_requests = server.get_all_requests()
self.assertEquals(0, len(all_requests), "No metric should be sent")

# Opt-in via env var
process = self.run_cmd(optout_envvar_value="1")
(_, stderrdata) = process.communicate()
retcode = process.poll()
self.assertEquals(retcode, 0, "Command should successfully complete")
all_requests = server.get_all_requests()
self.assertEquals(1, len(all_requests), "Command run metric must be sent")

def test_must_not_crash_when_offline(self):
"""
Must not crash the process if internet is not available
"""
self.set_config(telemetry_enabled=True)

# DO NOT START Telemetry Server here.
# Try to run the command without it.

# Start the CLI
process = self.run_cmd()

(_, stderrdata) = process.communicate()

retcode = process.poll()
self.assertEquals(retcode, 0, "Command should successfully complete")
14 changes: 13 additions & 1 deletion tests/unit/lib/telemetry/test_telemetry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ def test_default_request_should_be_fire_and_forget(self, requests_mock):
telemetry = Telemetry(url=self.url)

telemetry.emit("metric_name", {})
requests_mock.post.assert_called_once_with(ANY, json=ANY, timeout=(2, 0.001)) # 1ms response timeout
requests_mock.post.assert_called_once_with(ANY, json=ANY, timeout=(2, 0.1)) # 100ms response timeout

@patch("samcli.lib.telemetry.telemetry.requests")
def test_request_must_wait_for_2_seconds_for_response(self, requests_mock):
Expand All @@ -121,15 +121,27 @@ def test_must_swallow_timeout_exception(self, requests_mock):
#

requests_mock.exceptions.Timeout = requests.exceptions.Timeout
requests_mock.exceptions.ConnectionError = requests.exceptions.ConnectionError
Copy link
Contributor

@jfuss jfuss Jul 25, 2019

Choose a reason for hiding this comment

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

Is this needed here if the this test is only for Timeout exception?

Copy link
Contributor Author

@sanathkr sanathkr Jul 25, 2019

Choose a reason for hiding this comment

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

Yeah lbecause of how Python exception mocking works

requests_mock.post.side_effect = requests.exceptions.Timeout()

telemetry.emit("metric_name", {})

@patch("samcli.lib.telemetry.telemetry.requests")
def test_must_swallow_connection_error_exception(self, requests_mock):
telemetry = Telemetry(url=self.url)

requests_mock.exceptions.Timeout = requests.exceptions.Timeout
requests_mock.exceptions.ConnectionError = requests.exceptions.ConnectionError
requests_mock.post.side_effect = requests.exceptions.ConnectionError()

telemetry.emit("metric_name", {})

@patch("samcli.lib.telemetry.telemetry.requests")
def test_must_raise_on_other_requests_exception(self, requests_mock):
telemetry = Telemetry(url=self.url)

requests_mock.exceptions.Timeout = requests.exceptions.Timeout
requests_mock.exceptions.ConnectionError = requests.exceptions.ConnectionError
requests_mock.post.side_effect = IOError()

with self.assertRaises(IOError):
Expand Down