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

[system/process] add support for mutlierr #166

Merged
merged 28 commits into from
Jul 23, 2024
Merged

[system/process] add support for mutlierr #166

merged 28 commits into from
Jul 23, 2024

Conversation

VihasMakwana
Copy link
Contributor

@VihasMakwana VihasMakwana commented Jul 18, 2024 •
edited by ycombinator
Loading

What does this PR do?

  • Previously, we weren't passing errors to the caller while monitoring set of processes.
    • With the recent introduction of the status reporter for metricsets, it is impossible to change the status to degraded if such errors are not passed to the caller.
  • Fix this by passing errors to the caller. We also populate the process related information to our best-effort.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.md

@VihasMakwana VihasMakwana mentioned this pull request Jul 18, 2024
Closed
4 tasks
@VihasMakwana VihasMakwana marked this pull request as ready for review July 18, 2024 14:12
@VihasMakwana VihasMakwana requested a review from a team as a code owner July 18, 2024 14:12
@VihasMakwana VihasMakwana requested review from belimawr, leehinman, cmacknz, a team and faec and removed request for a team, belimawr and leehinman July 18, 2024 14:12
@VihasMakwana VihasMakwana added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Jul 18, 2024
@pierrehilbert pierrehilbert requested a review from rdner July 18, 2024 21:17
@pierrehilbert pierrehilbert removed the request for review from faec July 18, 2024 21:17
ycombinator
ycombinator reviewed Jul 18, 2024
View reviewed changes
metric/system/process/helpers.go Outdated
@@ -98,3 +100,10 @@ func GetProcCPUPercentage(s0, s1 ProcState) ProcState {
return s1

}

func CanIgnore(err error) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be exported, as it seems to be used from within the same package?

Suggested change
func CanIgnore(err error) bool {
func canIgnore(err error) bool {

Same goes for CanDegrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi,
Regarding CanDegrade, we use this helper in metricbeat here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, @ycombinator we will also use CanDegrade here to ignore non-fatal errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made CanIgnore local to this package.

@VihasMakwana VihasMakwana added enhancement New feature or request breaking-change labels Jul 19, 2024
cmacknz
cmacknz reviewed Jul 22, 2024
View reviewed changes
Copy link
Member

@cmacknz cmacknz left a comment

Choose a reason for hiding this comment

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

Couple of small things but besides those looks good.

metric/system/process/process.go Outdated Show resolved Hide resolved
metric/system/process/process.go Outdated Show resolved Hide resolved
metric/system/process/process.go Show resolved Hide resolved
metric/system/process/process.go Show resolved Hide resolved
metric/system/process/process_aix.go Show resolved Hide resolved
cmacknz
cmacknz approved these changes Jul 22, 2024
View reviewed changes
Copy link
Member

@cmacknz cmacknz left a comment

Choose a reason for hiding this comment

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

Thanks!

@VihasMakwana VihasMakwana merged commit 3abd5c0 into elastic:main Jul 23, 2024
5 checks passed
VihasMakwana added a commit to VihasMakwana/elastic-agent-system-metrics that referenced this pull request Jul 26, 2024
@VihasMakwana
Revert "[system/process] add support for mutlierr (elastic#166)"
698e979 
This reverts commit 3abd5c0.
@VihasMakwana VihasMakwana mentioned this pull request Jul 26, 2024
Merged
VihasMakwana added a commit that referenced this pull request Jul 26, 2024
@VihasMakwana
Revert non fatal error changes for now. (#169)
50700ed 
- This PR reverts
#166 and
#166
- This is due the agent CI getting blocked due to metricbeat status
reporter.

- We will merge these changes as we fix the root cause of above.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdner rdner Awaiting requested review from rdner

@ycombinator ycombinator Awaiting requested review from ycombinator

@cmacknz cmacknz Awaiting requested review from cmacknz

Assignees
No one assigned
Labels
breaking-change enhancement New feature or request Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[metric/system/process] - return errors encountered while monitoring set of processes
3 participants
@VihasMakwana @ycombinator @cmacknz