Skip to content

Bug: Fix error formatting #206

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

0xThiebaut
Copy link

What does this PR do?

Fix error formatting.

Why is it important?

To avoid funky error reporting such as:

2025-03-12T16:38:11.740Z ERROR [metrics] map[file.line:40 file.name:report/metrics_file_descriptors.go function:github.com/elastic/elastic-agent-system-metrics/report.SetupLinuxBSDFDMetrics.FDUsageReporter.func1] Error while retrieving FD information: %verror fetching PID 38644: GetInfoForPid: no such process {"ecs.version": "1.6.0"}

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

Author's Checklist

  • [ ]

Related issues

@0xThiebaut 0xThiebaut requested a review from a team as a code owner March 12, 2025 16:50
@0xThiebaut 0xThiebaut requested review from belimawr and mauri870 and removed request for a team March 12, 2025 16:50
Copy link

cla-checker-service bot commented Mar 12, 2025

💚 CLA has been signed

@0xThiebaut
Copy link
Author

Signed the CLA.

@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Mar 12, 2025
mauri870
mauri870 previously approved these changes Mar 13, 2025
Copy link
Member

@mauri870 mauri870 left a comment

Choose a reason for hiding this comment

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

LGTM! I left some minor feedback. Thanks for your contribution!

@0xThiebaut
Copy link
Author

Thanks for the review! I went ahead and applied your suggested change in a couple more places.

@0xThiebaut 0xThiebaut requested a review from mauri870 March 13, 2025 15:00
mauri870
mauri870 previously approved these changes Mar 14, 2025
Copy link
Member

@mauri870 mauri870 left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@belimawr belimawr left a comment

Choose a reason for hiding this comment

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

It looks great, thanks for the contribution!

There is just one %v you missed.

@0xThiebaut
Copy link
Author

Added another couple more. Looking for %v in the code base highlights quite some other locations that might benefit from a quick refactor but this goes beyond the error formatting I was focusing on.

@0xThiebaut 0xThiebaut requested a review from belimawr March 14, 2025 15:49
belimawr
belimawr previously approved these changes Mar 17, 2025
@0xThiebaut
Copy link
Author

I'll review the failed linting, seems one Errorf is not known.

@belimawr
Copy link
Contributor

buildkite test this

@belimawr
Copy link
Contributor

The failing test seems unrelated, I created a flaky test issue: #212

@belimawr
Copy link
Contributor

buildkite test this

@0xThiebaut
Copy link
Author

@belimawr As the buildkite results are not public, can you let me know if the failures are caused by the PR?

For the linting errors, I don't believe these are caused by this PR.

@belimawr
Copy link
Contributor

@belimawr As the buildkite results are not public, can you let me know if the failures are caused by the PR?

For the linting errors, I don't believe these are caused by this PR.

TestContainerProcess/nsmode:private_priv:false_user is failing on CI. I've run the tests on my machine and they're passing.

The failure does not look related to the PR.

Today or tomorrow I'll take a deeper look at it.

@belimawr
Copy link
Contributor

This flaky test is very OS dependant, on CI it is failing on Rhel 9, we'll get someone that knows this test better to take a look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants