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.

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

Update elastic-agent-system-metrics to v0.8.1 #37027

Merged
merged 6 commits into from
Nov 20, 2023

Conversation

belimawr
Copy link
Contributor

@belimawr belimawr commented Nov 3, 2023

Proposed commit message

Update elastic-agent-system-metrics to v0.8.1 to enable collecting
memory and CPU metrics from privileged process on Windows.

Fix python test to ensure the cmdLine is found in at least one
process instead of them all because we cannot fetch the cmdLine from
privileged process on Windows.

Checklist

  • My code follows the style guidelines of this project
    - [ ] I have commented my code, particularly in hard-to-understand areas
    - [ ] I have made corresponding changes to the documentation
    - [ ] I have made corresponding change to the default configuration files
    - [ ] I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

How to test this PR locally

  1. Install Elastic-Agent with Elastic-Defend integration enabled
  2. Run Metricbeat (from this PR) as administrator with process metrics enabled
  3. Ensure events from elastic-endpoint.exe contain CPU and memory metrics.

Related issues

## Use cases

Screenshots

Screenshot 2023-11-02 150329

## Logs

@belimawr belimawr requested a review from a team as a code owner November 3, 2023 16:11
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Nov 3, 2023
Copy link
Contributor

mergify bot commented Nov 3, 2023

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @belimawr? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@belimawr belimawr added the Team:Elastic-Agent Label for the Agent team label Nov 3, 2023
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Nov 3, 2023
@elasticmachine
Copy link
Collaborator

elasticmachine commented Nov 3, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-11-15T09:18:15.946+0000

  • Duration: 178 min 26 sec

Test stats 🧪

Test Results
Failed 0
Passed 28172
Skipped 1882
Total 30054

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@belimawr belimawr requested a review from a team as a code owner November 6, 2023 15:06
@belimawr belimawr changed the title Update elastic-agent-system-metrics to v0.8.0 Update elastic-agent-system-metrics to v0.8.1 Nov 8, 2023
Update elastic-agent-system-metrics to v0.8.0 to enable collecting
memory and CPU metrics from privileged process on Windows.
Fix system tests by removing metrics that are not present for all
process.
Update elastic-agent-system-metrics to v0.8.1 to enable collecting
memory and CPU metrics from privileged process on Windows.
@botelastic botelastic bot added the Team:Automation Label for the Observability productivity team label Nov 13, 2023
Fix the python test to ensure the cmdLine is found in at least one
process instead of them all because we cannot fetch the cmdLine from
privileged process.
Comment on lines +440 to +443
# After iterating over all process, make sure at least one of them had
# the 'cmdline' set.
self.assertTrue(
found_cmdline, "cmdline not found in any process events")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fearful-symmetry, could you do a sanity check on this last change? I believe it is ok to change, but I'd like a second opinion before merging.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the assertTrue() line was just moved out of the loop block to check for the cmdline globally? That seems fine, but wouldn't it require the last event to set found_cmdline |= "cmdline" in process in order to pass? Maybe a counter that needs to be > 1 would work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like the assertTrue() line was just moved out of the loop block to check for the cmdline globally?

Yes, that was my intent because it already does an OR operation on itself. Being honest I was very confused by the assertTrue() being inside the loop and having found_cmdline |= "cmdline" in process.

found_cmdline = False
for evt in output:
process = evt["system"]["process"]
# Not all process will have 'cmdline' due to permission issues,
# especially on Windows. Therefore we ensure at least some of
# them will have it.
found_cmdline |= "cmdline" in process

but wouldn't it require the last event to set found_cmdline |= "cmdline" in process in order to pass?

That's an OR operator, so as long as "cmdline" in process returns True at least once, then found_cmdline will always be True.

@belimawr belimawr merged commit 9ad511b into elastic:main Nov 20, 2023
8 checks passed
Scholar-Li pushed a commit to Scholar-Li/beats that referenced this pull request Feb 5, 2024
Update elastic-agent-system-metrics to v0.8.1 to enable collecting
memory and CPU metrics from privileged process on Windows.

Fix the python test to ensure the cmdLine is found in at least one
process instead of them all because we cannot fetch the cmdLine from
privileged process on Windows.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Automation Label for the Observability productivity team Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metricbeat fails to get information for protected processes under Windows
4 participants