Telemetry pipeline improvements (iteration 1) #3133
Conversation
nagworld9
requested review from
narrieta,
ZhidongPeng and
maddieford
as code owners
tests/ga/test_update.py
Outdated
| @@ -106,17 +106,20 @@ def _get_update_handler(iterations=1, test_data=None, protocol=None, autoupdate_ | |||
| This function returns a mocked version of the UpdateHandler object to be used for testing. It will only run the | |||
| main loop [iterations] no of times. | |||
| """ | |||
| # Added try-finally block to fix false positive pylint warning contextmanager-generator-missing-cleanup | |||
There was a problem hiding this comment.
pylint has new warning even on develop branch https://pylint.readthedocs.io/en/latest/user_guide/messages/warning/contextmanager-generator-missing-cleanup.html
azurelinuxagent/common/event.py
Outdated
| logger.periodic_error(logger.EVERY_FIFTEEN_MINUTES, "[PERIODIC] {0}".format(ustr(e))) | ||
| self.flush_or_save_event(event, message, immediate_flush) | ||
|
|
||
| def flush_or_save_event(self, event, message, immediate_flush): |
There was a problem hiding this comment.
I'd rename this to report_or_save_event
|
|
||
| class EventLogger(object): | ||
| def __init__(self): | ||
| self.event_dir = None | ||
| self.periodic_events = {} | ||
| self.protocol = None |
There was a problem hiding this comment.
it is not a good idea to have this uninitialized property. let's look for alternatives... maybe get the protocol on demand when sending an important event?
There was a problem hiding this comment.
Sending on demand may add other compilations, wherever we send imp event, we need reference for protocol object as well plus we need to add protocol as optional property to add_event method with None.
or if you have something else in mind for on demand, lets have quick call to discuss
(cherry picked from commit 55ea21f)
| @@ -468,7 +481,10 @@ def http_request(method, | |||
| # retry attempts | |||
| if _is_throttle_status(resp.status): | |||
| was_throttled = True | |||
| max_retry = max(max_retry, THROTTLE_RETRIES) | |||
| # Today, THROTTLE_RETRIES set to big number as opposite to slow down the retry attempts | |||
There was a problem hiding this comment.
I don't follow this comment, could you please explain:
Today, THROTTLE_RETRIES set to big number as opposite to slow down the retry attempts
There was a problem hiding this comment.
Today when any api return throttle error, we make 26 attempts for retying as opposed to back off and make fewer attempts with good delay.
My fix will change this behavior only for telemetry calls and keeping same for non-telemetry calls as I wasn't sure why it was implemented that way.
I'll improve the comment
Description
This pr address below issues to improve data reliability
I added necessary comments, still questions/concerns happy to go over a call.
Issue #
PR information
Quality of Code and Contribution Guidelines