Extension telemetry pipeline #1918

larohra · 2020-06-24T00:56:05Z

Description

This PR contains the major changes for enabling Extension Telemetry pipeline for Extensions.
It adds a new thread to the Agent which comes alive every 5 mins and reads the events directory per extension and sends those extensions to Wireserver.

Issue #

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which has an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes in the description.
New Unit tests were added for the changes made and Travis.CI is passing.

Quality of Code and Contribution Guidelines

I have read the contribution guidelines.

…f VM

…n-telemetry-pipeline # Conflicts: # azurelinuxagent/ga/exthandlers.py

…n-telemetry-pipeline # Conflicts: # azurelinuxagent/common/event.py # azurelinuxagent/common/telemetryevent.py # tests/common/test_event.py

codecov · 2020-06-24T00:58:10Z

Codecov Report

Merging #1918 into develop will increase coverage by 0.45%.
The diff coverage is 92.88%.

@@             Coverage Diff             @@
##           develop    #1918      +/-   ##
===========================================
+ Coverage    70.69%   71.14%   +0.45%     
===========================================
  Files           85       86       +1     
  Lines        12055    12285     +230     
  Branches      1685     1728      +43     
===========================================
+ Hits          8522     8740     +218     
- Misses        3152     3159       +7     
- Partials       381      386       +5

Impacted Files	Coverage Δ
azurelinuxagent/ga/extension_telemetry.py	`91.42% <91.42%> (ø)`
azurelinuxagent/common/event.py	`86.61% <100.00%> (+0.08%)`	⬆️
azurelinuxagent/common/exception.py	`98.91% <100.00%> (+0.07%)`	⬆️
azurelinuxagent/ga/env.py	`65.00% <100.00%> (+1.20%)`	⬆️
azurelinuxagent/ga/exthandlers.py	`87.72% <100.00%> (+0.22%)`	⬆️
azurelinuxagent/ga/monitor.py	`78.45% <100.00%> (+0.48%)`	⬆️
azurelinuxagent/ga/update.py	`88.43% <100.00%> (+0.30%)`	⬆️
azurelinuxagent/pa/deprovision/default.py	`67.85% <100.00%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4d6404...79a47d3. Read the comment docs.

narrieta

A few initial comments. I still need to look at extension_telemetry.py and tests.

narrieta · 2020-06-30T16:12:58Z

azurelinuxagent/ga/exthandlers.py

@@ -60,7 +61,8 @@

 _VALID_HANDLER_STATUS = ['Ready', 'NotReady', "Installing", "Unresponsive"]

-HANDLER_NAME_PATTERN = re.compile(_HANDLER_PATTERN + r'$', re.IGNORECASE)
+HANDLER_NAME_PATTERN = re.compile(_HANDLER_NAME_PATTERN, re.IGNORECASE)
+HANDLER_COMPLETE_NAME_PATTERN = re.compile(_HANDLER_PATTERN + r'$', re.IGNORECASE)


I missed this in the previous PR, but is_extension_telemetry_pipeline_enabled should be defined elsewhere (extension_telemetry.py?)

It does make more sense for this flag to be in extension_telemetry.py but I didnt put it there because that creates a circular dependency (because I need to read the HANDLER_NAME from exthandlers.py) and moving these globals (HANDLER_NAME, etc) outside doesn't make logical sense either since they're defined for the Extension Handlers.

To avoid all this confusion I just left the is_extension_telemetry_pipeline_enabled in exthandlers because that's where we are reading the flag. If you feel strongly about it I can move it to a common place though (maybe AgentGlobals.py or version.py) to avoid the circular dependency.

azurelinuxagent/ga/update.py

azurelinuxagent/common/event.py

azurelinuxagent/ga/update.py

azurelinuxagent/ga/extension_telemetry.py

narrieta

a few more comments, still need to look at the tests

azurelinuxagent/ga/extension_telemetry.py

narrieta · 2020-06-30T20:43:35Z

azurelinuxagent/ga/extension_telemetry.py

+
+    # Limits
+    MAX_NUMBER_OF_EVENTS_PER_EXTENSION_PER_PERIOD = 300
+    EXTENSION_EVENT_FILE_MAX_SIZE = 4 * 1024 * 1024  # 4 MB = 4 * 1,048,576 Bytes


do we have similar limits for the agent? should they be the same?

We have some limiters for the Agent events but they're more relaxed as compared to these. These are mainly needed to ensure that the extensions dont abuse the pipeline. I dont think these limiters are needed for the agent events.

azurelinuxagent/ga/extension_telemetry.py

narrieta · 2020-06-30T23:09:43Z

azurelinuxagent/ga/extension_telemetry.py

+                            event_file_path, convert_to_mb(event_file_size),
+                            convert_to_mb(self.EXTENSION_EVENT_FILE_MAX_SIZE))
+                    logger.warn(msg)
+                    add_log_event(level=logger.LogLevel.WARNING, message=msg, forced=True)


why add_log_event? (instead of add_event)

add_event goes to the ExtensionEvents table and add_log_event goes to the GenericLogs table. I figured for the extension related stuff, it would make sense if all the data (including errors) are in 1 table so that its easier for debugging.

But above you are using add_event(op=WALAEventOperation.ExtensionTelemetryEventProcessing, message=msg, is_success=False)

The idea is basically that any error thrown by the agent while processing Extension Telemetry will be logged in ExtensionEvents table and any errors thrown due to the telemetry events (malformed, limit exceeds, etc) will be logged in the GenericLogs table for easier lookup

tests/ga/test_extension_telemetry.py

tests/ga/test_update.py

azurelinuxagent/ga/extension_telemetry.py

narrieta · 2020-08-03T17:11:16Z

azurelinuxagent/common/exception.py

@@ -210,6 +210,28 @@ def __init__(self, msg=None, inner=None):
        super(ResourceGoneError, self).__init__(msg, inner)


+class RemoteAccessError(AgentError):


seems like you need to update your branch

There's still something funny with your branch. This exception was removed by #1935

Yeah I dont think the merge happened properly/correctly. Even Travis is failing with some weird error. I'll try fixing it today properly if I get time. Will let you know once its ready for review!

Not sure what went wrong here, I tried the merge again but somehow this is still there. I'll just cherry-pick that commit into my branch to get this change in. Apart from this I dont see anything out of ordinary

…try-pipeline # Conflicts: # tests/ga/test_extension.py

pgombar

Left some comments, still haven't looked at the test code.

azurelinuxagent/common/exception.py

azurelinuxagent/ga/extension_telemetry.py

pgombar · 2020-08-05T20:38:48Z

azurelinuxagent/ga/extension_telemetry.py

+                            event_file_path, convert_to_mb(event_file_size),
+                            convert_to_mb(self.EXTENSION_EVENT_FILE_MAX_SIZE))
+                    logger.warn(msg)
+                    add_log_event(level=logger.LogLevel.WARNING, message=msg, forced=True)


But above you are using add_event(op=WALAEventOperation.ExtensionTelemetryEventProcessing, message=msg, is_success=False)

azurelinuxagent/ga/extension_telemetry.py

…try-pipeline

tests/ga/test_update.py

tests/ga/test_extension_telemetry.py

pgombar · 2020-08-14T22:43:19Z

tests/ga/test_extension_telemetry.py

+
+        # EventName maps to HandlerName + '-' + Version from event file
+        expected_mapping = {
+            GuestAgentGenericLogsSchema.EventName: ExtensionEventSchema.Version,


Confused between this line and the comment above, the definitions don't seem to match?

Ohh yeah sorry for the confusion, in the code below I'm actually mapping it to "Name-Version". This dict is supposed to just be a placeholder

Its happening in Line 433 of this file

pgombar

Minor comments on the tests, but otherwise LGTM. I think it's important to re-enable the special characters test since that scenario has been problematic so far.

…try-pipeline # Conflicts: # azurelinuxagent/ga/exthandlers.py # azurelinuxagent/ga/update.py # tests/ga/test_extension.py

larohra added 17 commits June 8, 2020 11:12

Added HandlerName regex and renamed the current one

24fe79f

Added helper functions and fixed GAGenericLogs Context2 table

1b8aa12

Create Events directory and add it to HandlerEnvironment.json

64a53e2

Add ExtensionTelemetryPipeline tool to send extension telemetry out o…

78b6f09

…f VM

Handle keyerrors gracefully with verbose msgs

c67c30a

Added new tests for extension telemetry pipeline

8db5766

Merge remote-tracking branch 'remotes/upstream/develop' into extensio…

746dcb9

…n-telemetry-pipeline # Conflicts: # azurelinuxagent/ga/exthandlers.py

Merge new schema with current code

908ed50

cherry picking 1911

e3e09bd

Complete cherry pick 1911

84aa75f

Fixed failing py3 test

d18bda1

Fixed failing tests

612a1e3

fix failing test for py2.6, no dict comprehension < 2.7

5391a65

Fix failing py2.6 tests

e0342c8

Merge remote-tracking branch 'remotes/upstream/develop' into extensio…

dea25fa

…n-telemetry-pipeline # Conflicts: # azurelinuxagent/common/event.py # azurelinuxagent/common/telemetryevent.py # tests/common/test_event.py

Code cleanup, added more logging

b80fec4

Fix failing tests

85f54e3

larohra requested review from kevinclark19a, narrieta, pgombar and ZhidongPeng as code owners June 24, 2020 00:56

larohra added 2 commits June 25, 2020 11:55

Minor log improvements

e7ce246

minor code cleanup

1accf1e

narrieta reviewed Jun 30, 2020

View reviewed changes

azurelinuxagent/ga/extension_telemetry.py Outdated Show resolved Hide resolved

narrieta reviewed Jun 30, 2020

View reviewed changes

narrieta reviewed Jul 2, 2020

View reviewed changes

tests/ga/test_extension_telemetry.py Outdated Show resolved Hide resolved

larohra added 2 commits July 15, 2020 17:19

Addressed PR comments and code cleanup

0a25d51

Merge branch 'develop' into extension-telemetry-pipeline

bd922cf

larohra commented Jul 18, 2020

View reviewed changes

tests/ga/test_update.py Show resolved Hide resolved

kevinclark19a reviewed Jul 21, 2020

View reviewed changes

azurelinuxagent/ga/extension_telemetry.py Show resolved Hide resolved

kevinclark19a reviewed Jul 21, 2020

View reviewed changes

azurelinuxagent/ga/extension_telemetry.py Show resolved Hide resolved

narrieta reviewed Aug 3, 2020

View reviewed changes

narrieta previously approved these changes Aug 3, 2020

View reviewed changes

Merge branch 'develop' into extension-telemetry-pipeline

1d7b694

larohra dismissed narrieta’s stale review via 1d7b694 August 4, 2020 00:38

larohra added 4 commits August 4, 2020 17:02

Merge remote-tracking branch 'upstream/develop' into extension-teleme…

09d4fd9

…try-pipeline # Conflicts: # tests/ga/test_extension.py

Removed extra code and test fix

1a75393

Readded wrongfully deleted class

87f18da

Fixed failing tests

88c20b3

pgombar reviewed Aug 5, 2020

View reviewed changes

Code beautification

4b2558a

larohra mentioned this pull request Aug 12, 2020

Recreate handler environment file on service startup #1960

Merged

6 tasks

Merge remote-tracking branch 'upstream/develop' into extension-teleme…

e6c7f81

…try-pipeline

pgombar reviewed Aug 14, 2020

View reviewed changes

tests/ga/test_update.py Show resolved Hide resolved

pgombar reviewed Aug 14, 2020

View reviewed changes

tests/ga/test_extension_telemetry.py Outdated Show resolved Hide resolved

pgombar reviewed Aug 14, 2020

View reviewed changes

tests/ga/test_extension_telemetry.py Outdated Show resolved Hide resolved

pgombar reviewed Aug 14, 2020

View reviewed changes

pgombar previously approved these changes Aug 14, 2020

View reviewed changes

larohra added 2 commits August 14, 2020 15:51

Merge remote-tracking branch 'upstream/develop' into extension-teleme…

e76111c

…try-pipeline # Conflicts: # azurelinuxagent/ga/exthandlers.py # azurelinuxagent/ga/update.py # tests/ga/test_extension.py

Enabled skipped test since bug fixed

2978df9

larohra dismissed pgombar’s stale review via 2978df9 August 14, 2020 22:52

pgombar previously approved these changes Aug 14, 2020

View reviewed changes

PR comments

79a47d3

larohra dismissed pgombar’s stale review via 79a47d3 August 14, 2020 23:02

pgombar approved these changes Aug 17, 2020

View reviewed changes

narrieta approved these changes Aug 17, 2020

View reviewed changes

larohra merged commit 5d2ac9f into Azure:develop Aug 17, 2020

larohra deleted the extension-telemetry-pipeline branch August 17, 2020 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extension telemetry pipeline #1918

Extension telemetry pipeline #1918

larohra commented Jun 24, 2020

codecov bot commented Jun 24, 2020 •

edited

Loading

narrieta left a comment

narrieta Jun 30, 2020

larohra Jul 18, 2020

narrieta left a comment

narrieta Jun 30, 2020

larohra Jul 16, 2020

narrieta Jun 30, 2020

larohra Jul 16, 2020

pgombar Aug 5, 2020

larohra Aug 12, 2020

narrieta Aug 3, 2020

narrieta Aug 4, 2020

larohra Aug 4, 2020

larohra Aug 5, 2020

pgombar left a comment

pgombar Aug 5, 2020

pgombar Aug 14, 2020

larohra Aug 14, 2020

larohra Aug 14, 2020

pgombar left a comment

		@@ -210,6 +210,28 @@ def __init__(self, msg=None, inner=None):
		super(ResourceGoneError, self).__init__(msg, inner)


		class RemoteAccessError(AgentError):

Extension telemetry pipeline #1918

Extension telemetry pipeline #1918

Conversation

larohra commented Jun 24, 2020

Description

PR information

Quality of Code and Contribution Guidelines

codecov bot commented Jun 24, 2020 • edited Loading

Codecov Report

narrieta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narrieta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgombar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgombar left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 24, 2020 •

edited

Loading