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.

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

Jump to bottom

Changing add_event to start sending missing fields #1663

Merged
merged 14 commits into from
Nov 6, 2019
Merged

Changing add_event to start sending missing fields #1663

merged 14 commits into from
Nov 6, 2019

Conversation

vrdmr
Copy link
Member

@vrdmr vrdmr commented Oct 10, 2019 •
edited
Loading

Description

This PR makes changes to the base implementation methods used for sending events to telemetry, to add newer telemetry fields which weren't sent earlier. Mainly TaskName, EventTid, EventPid, OpCodeName, KeywordName. The ExecutionMode has been added in #1657.

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

This change is Reviewable

@codecov
Copy link

codecov bot commented Oct 10, 2019 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (develop@2229b7b). Click here to learn what that means.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #1663   +/-   ##
==========================================
  Coverage           ?   67.65%           
==========================================
  Files              ?       80           
  Lines              ?    11515           
  Branches           ?     1616           
==========================================
  Hits               ?     7790           
  Misses             ?     3383           
  Partials           ?      342
Impacted Files Coverage Δ
azurelinuxagent/ga/env.py 51.88% <0%> (ø)
azurelinuxagent/ga/monitor.py 88.31% <100%> (ø)
azurelinuxagent/common/event.py 84.28% <100%> (ø)
azurelinuxagent/common/telemetryevent.py 100% <100%> (ø)
azurelinuxagent/agent.py 45.22% <66.66%> (ø)

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 2229b7b...4a55c62. Read the comment docs.

@vrdmr vrdmr mentioned this pull request Oct 10, 2019
Merged
6 tasks
larohra
larohra reviewed Oct 10, 2019
View reviewed changes
tests/common/test_event.py Outdated Show resolved Hide resolved
larohra
larohra reviewed Oct 10, 2019
View reviewed changes
Copy link
Contributor

@larohra larohra left a comment

Choose a reason for hiding this comment

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

Some minor comments and feedback

narrieta
narrieta reviewed Oct 15, 2019
View reviewed changes
Copy link
Member

@narrieta narrieta left a comment

Choose a reason for hiding this comment

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

posted a few comments

larohra
larohra previously approved these changes Oct 16, 2019
View reviewed changes
Copy link
Contributor

@larohra larohra left a comment

Choose a reason for hiding this comment

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

LGTM

pgombar
pgombar previously approved these changes Oct 16, 2019
View reviewed changes
Copy link
Contributor

@pgombar pgombar left a comment

Choose a reason for hiding this comment

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

LGTM with two questions

tests/common/test_event.py Outdated Show resolved Hide resolved
tests/ga/test_monitor.py Show resolved Hide resolved
narrieta
narrieta reviewed Oct 17, 2019
View reviewed changes
azurelinuxagent/common/event.py Outdated Show resolved Hide resolved
tests/common/test_event.py Outdated Show resolved Hide resolved
@vrdmr vrdmr mentioned this pull request Oct 17, 2019
Merged
6 tasks
@vrdmr vrdmr requested a review from narrieta October 18, 2019 00:38
narrieta
narrieta reviewed Oct 18, 2019
View reviewed changes
Copy link
Member

@narrieta narrieta left a comment

Choose a reason for hiding this comment

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

LGTM. Please ping me once end-to-end tests are completed.

narrieta
narrieta previously approved these changes Nov 5, 2019
View reviewed changes
azurelinuxagent/common/event.py Outdated Show resolved Hide resolved
azurelinuxagent/common/event.py Outdated Show resolved Hide resolved
@vrdmr vrdmr force-pushed the vameru/making-schema-consistent branch from c2175e5 to 196d9aa Compare November 5, 2019 23:30
@vrdmr
Adding common params into all the events if not present & other changes
bcc5536 
1. Added "in" support for TelemetryEvent for easier code check. Also added complementary tests for the same.
2. Updated the tests in test_monitor to reflect newer sysinfo fields added.
3. Added comments to describe the decisions behind the field decisions.
@vrdmr vrdmr dismissed stale reviews from narrieta, pgombar, and larohra via bcc5536 November 6, 2019 01:20
narrieta
narrieta reviewed Nov 6, 2019
View reviewed changes
Copy link
Member

@narrieta narrieta left a comment

Choose a reason for hiding this comment

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

Added a few comments, let me know what you think

pgombar
pgombar reviewed Nov 6, 2019
View reviewed changes
azurelinuxagent/common/event.py Outdated Show resolved Hide resolved
azurelinuxagent/common/event.py Outdated Show resolved Hide resolved
azurelinuxagent/ga/monitor.py Outdated Show resolved Hide resolved
tests/data/ext/event_from_agent.xml Outdated Show resolved Hide resolved
tests/ga/test_monitor.py Outdated Show resolved Hide resolved
tests/ga/test_monitor.py Outdated Show resolved Hide resolved
tests/ga/test_monitor.py Show resolved Hide resolved
narrieta
narrieta previously approved these changes Nov 6, 2019
View reviewed changes
@vrdmr vrdmr requested a review from larohra November 6, 2019 21:35
pgombar
pgombar previously approved these changes Nov 6, 2019
View reviewed changes
Copy link
Contributor

@pgombar pgombar left a comment

Choose a reason for hiding this comment

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

LGTM

tests/ga/test_monitor.py Outdated
@@ -174,6 +170,7 @@ def test_add_sysinfo_should_honor_sysinfo_values_from_agent_for_agent_events(sel

counter = 0
for p in event.parameters:
print(p.name, p.value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

@vrdmr vrdmr dismissed stale reviews from pgombar and narrieta via 4a55c62 November 6, 2019 22:52
@pgombar pgombar self-requested a review November 6, 2019 22:53
@vrdmr vrdmr requested a review from narrieta November 6, 2019 22:54
@pgombar pgombar merged commit df98f00 into Azure:develop Nov 6, 2019
@vrdmr vrdmr mentioned this pull request Nov 16, 2019
Merged
6 tasks
@vrdmr vrdmr deleted the vameru/making-schema-consistent branch November 22, 2019 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@narrieta narrieta narrieta approved these changes

@pgombar pgombar pgombar approved these changes

@ZhidongPeng ZhidongPeng Awaiting requested review from ZhidongPeng ZhidongPeng is a code owner

@larohra larohra Awaiting requested review from larohra

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vrdmr @narrieta @pgombar @larohra