Skip to content

fix: More robust connections to telemetry backend#1289

Merged
sanathkr merged 1 commit intoaws:developfrom
sanathkr:telemetry_perf_fix
Jul 25, 2019
Merged

fix: More robust connections to telemetry backend#1289
sanathkr merged 1 commit intoaws:developfrom
sanathkr:telemetry_perf_fix

Conversation

@sanathkr
Copy link
Contributor

  • Increase read timeout to 100ms. Backend needs a bit more time than 1ms to reliably receive & process data
  • Don't crash if offline
  • Integration tests to validate offline behavior & opt-in/opt-out behavior

Issue #, if available:

Description of changes:

Checklist:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

- Increase read timeout to 100ms. Backend needs a bit more time than 1ms to reliably receive & process data
- Don't crash if offline
- Integration tests to validate offline behavior & opt-in/opt-out behavior
#

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

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

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

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

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

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

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

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

@sanathkr
Copy link
Contributor Author

Couple of StartApi tests failed in Travis 2.7 only. This should be transient because I have never seen them fail before. Merging the PR anyway

@sanathkr sanathkr merged commit f5a24de into aws:develop Jul 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants