[system/process] Fix windows protected process data collection #191
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Enhancement
We can ignore the error in two cases:
- While reading the process executable name.
- For pid 4, this call fails as we can't access the executable name via
the system call. Same for other kernel-level processes.
- While finding the owner for a particular process.
- We try to open the process token via `syscall.OpenProcessToken`and we
can't access the token for protected processes , even as an
administrator.
it's okay to ignore these errors and move forward as we can access few
other metrics (memory, cpu).
More context
[here](elastic/beats#40484 (comment))
Relates elastic/beats#40484
This PR does the following: - It integrates `getIdleMemory` and `getProcessMemory` introduced in #181. This is used to capture stats for PID 0. - `FillMetricsRequiringMoreAccess` adds some extra information about a process. It's failure is expected for protected processes and if a non-root user accesses elevated processes. We can just log the error rather than returning an error. This will not pollute the status message on fleet. - @cmacknz Please let me know how you feel about this. This is final PR in this series and once this PR gets merged, we can merge `windows-protected-processes` into `main`.
VihasMakwana
added
the
Team:Elastic-Agent-Data-Plane
VihasMakwana
changed the title
VihasMakwana
requested review from
AndersonQ,
rdner and
leehinman
and removed request for
a team
VihasMakwana
changed the title
leehinman
approved these changes
Nov 14, 2024
| return state, err | ||
| } | ||
| // sleep is crucial to calculate the idle percentage | ||
| time.Sleep(50 * time.Millisecond) |
There was a problem hiding this comment.
Can you add to the docstring for FillPidMetrics, that it will take 50msec to complete because of this. I just want to be sure that the developer using FillPidMetrics understands this call has a "long" runtime overhead and isn't just reading counters.
There was a problem hiding this comment.
@leehinman I'll remove this part.
We don't show percentage for a process (for windows). I don't see any reason to include percentage for idle process.
In future, we can make use of performance counters for it.
What do you think?
AndersonQ
requested changes
Nov 19, 2024
| state.Memory.Rss.Bytes = opt.UintWith(processInfo.WorkingSetSize) | ||
| state.Memory.Size = opt.UintWith(processInfo.PrivatePageCount) | ||
| state.NumThreads = opt.IntWith(int(processInfo.NumberOfThreads)) | ||
| break |
There was a problem hiding this comment.
[Blocker]
break or set offset+continue?
Isn't it possible to have other process in the list?
There was a problem hiding this comment.
Yes. But we're only interested in one with pid=0. So, no need to check other processes once we come across the pid 0 process
Co-authored-by: Anderson Queiroz <[email protected]>
Co-authored-by: Anderson Queiroz <[email protected]>
AndersonQ
approved these changes
Nov 19, 2024
|
/test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR merges work done on windows-protected-processes into
main.Fixes elastic/beats#40484