Skip to content

[system/process] add support for mutlierr #166

Merged
VihasMakwana merged 28 commits intoelastic:mainfrom
VihasMakwana:multierror-enhancement
Jul 23, 2024
Merged

[system/process] add support for mutlierr #166
VihasMakwana merged 28 commits intoelastic:mainfrom
VihasMakwana:multierror-enhancement

Conversation

@VihasMakwana
Copy link
Copy Markdown
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 a team, belimawr, cmacknz, faec and leehinman 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

}

func CanIgnore(err error) bool {
Copy link
Copy Markdown
Contributor

@ycombinator ycombinator Jul 18, 2024

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
Copy Markdown
Contributor Author

@VihasMakwana VihasMakwana Jul 19, 2024

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
Copy Markdown
Contributor Author

@VihasMakwana VihasMakwana Jul 19, 2024

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
Copy Markdown
Contributor Author

@VihasMakwana VihasMakwana Jul 19, 2024

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.

ycombinator
ycombinator reviewed Jul 18, 2024
View reviewed changes
@VihasMakwana VihasMakwana added enhancement New feature or request breaking-change labels Jul 19, 2024
@VihasMakwana VihasMakwana requested a review from ycombinator July 19, 2024 08:37
cmacknz
cmacknz reviewed Jul 22, 2024
View reviewed changes
cmacknz
cmacknz reviewed Jul 22, 2024
View reviewed changes
@VihasMakwana VihasMakwana requested a review from cmacknz July 22, 2024 19:15
cmacknz
cmacknz reviewed Jul 22, 2024
View reviewed changes
Copy link
Copy Markdown
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.

cmacknz
cmacknz approved these changes Jul 22, 2024
View reviewed changes
Copy link
Copy Markdown
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 requested a review from cmacknz July 22, 2024 20:36
@VihasMakwana VihasMakwana merged commit 3abd5c0 into elastic:main Jul 23, 2024
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