Skip to content

feat: Improve error handling in host information retrieval#1980

Closed
Dylan-M wants to merge 3 commits intoshirou:masterfrom
Dylan-M:dylanmyers/aix_host
Closed

feat: Improve error handling in host information retrieval#1980
Dylan-M wants to merge 3 commits intoshirou:masterfrom
Dylan-M:dylanmyers/aix_host

Conversation

@Dylan-M
Copy link
Copy Markdown
Contributor

@Dylan-M Dylan-M commented Dec 23, 2025

Description

This PR enhances AIX platform support in the host package with improved error handling and better test coverage through dependency injection.

Part 2 of splitting #1969: host package
Depends on #1979

Changes

host/host.go

  • Refactored InfoWithContext() to collect all errors during information gathering instead of returning early on first error
  • Now returns nil with errors.Join() when any field fails to retrieve
  • Enables graceful degradation by continuing to attempt gathering all host information

host/host_aix.go

  • Added dependency injection support with testInvoker variable and getInvoker() function
  • Replaced all direct invoke calls with getInvoker() to enable test mocking
  • Updated functions: HostIDWithContext(), BootTimeWithContext(), UptimeWithContext(), UsersWithContext(), PlatformInformationWithContext(), KernelVersionWithContext(), and KernelArch()

host/host_aix_test.go

  • Added TestBootTimeWithContext() for boot time functionality

Benefits

  • Better error resilience in host information gathering
  • Improved testability through dependency injection
  • All existing host functions now have AIX support with injectable dependencies
  • Foundation for easier testing and mocking of AIX host operations

@shirou
Copy link
Copy Markdown
Owner

shirou commented Jan 24, 2026

@Dylan-M Since #1979 has been merged, could you rebase this PR?

@Dylan-M
Copy link
Copy Markdown
Contributor Author

Dylan-M commented Jan 24, 2026

@Dylan-M Since #1979 has been merged, could you rebase this PR?

Done, same for the rest of the follow on PRs

Copy link
Copy Markdown
Owner

@shirou shirou left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution. However, I have some concerns about this PR.

This PR includes two separate changes: modifying the return value behavior of host.InfoWithContext() and introducing host.FDLimitsWithContext() for AIX only.

Regarding FDLimitsWithContext(): We cannot add a function that only runs on a single platform. Any new function must be supported on at least Windows and Linux.

Regarding the return value change in InfoWithContext(): This PR changes the behavior of return values. Previously, InfoWithContext() returned nil as the first value on error, but this PR returns a non-nil (partial) value instead. While I understand the rationale—return values on error are not strictly defined—we would prefer not to change the existing behavior as it could break existing code that relies on the current contract.

Could you please split this into separate PRs, each containing a single change? When a PR contains multiple changes, we may have to reject it entirely even if some parts are acceptable. Smaller, focused PRs are also much easier to review.

@Dylan-M Dylan-M force-pushed the dylanmyers/aix_host branch 2 times, most recently from 931b06b to ccfa854 Compare January 26, 2026 14:24
@Dylan-M Dylan-M changed the title feat: Add AIX file descriptor limits support and improve error handling in host information retrieval feat: Improve error handling in host information retrieval Jan 26, 2026
@Dylan-M
Copy link
Copy Markdown
Contributor Author

Dylan-M commented Jan 26, 2026

Thank you for the contribution. However, I have some concerns about this PR.

This PR includes two separate changes: modifying the return value behavior of host.InfoWithContext() and introducing host.FDLimitsWithContext() for AIX only.

Regarding FDLimitsWithContext(): We cannot add a function that only runs on a single platform. Any new function must be supported on at least Windows and Linux.

Regarding the return value change in InfoWithContext(): This PR changes the behavior of return values. Previously, InfoWithContext() returned nil as the first value on error, but this PR returns a non-nil (partial) value instead. While I understand the rationale—return values on error are not strictly defined—we would prefer not to change the existing behavior as it could break existing code that relies on the current contract.

Could you please split this into separate PRs, each containing a single change? When a PR contains multiple changes, we may have to reject it entirely even if some parts are acceptable. Smaller, focused PRs are also much easier to review.

Done. I'll create a separate follow on PR with the file descriptor work.

@Dylan-M Dylan-M requested a review from shirou January 26, 2026 14:28
@Dylan-M Dylan-M force-pushed the dylanmyers/aix_host branch from ccfa854 to 36846c2 Compare February 16, 2026 14:33
@Dylan-M Dylan-M force-pushed the dylanmyers/aix_host branch from 36846c2 to af6ff2e Compare February 27, 2026 13:52
@Dylan-M Dylan-M force-pushed the dylanmyers/aix_host branch from 4bd2913 to 00a05f9 Compare March 20, 2026 21:19
@Dylan-M Dylan-M force-pushed the dylanmyers/aix_host branch from 00a05f9 to 2b648c6 Compare March 20, 2026 21:33
@Dylan-M
Copy link
Copy Markdown
Contributor Author

Dylan-M commented Mar 20, 2026

@shirou I've refactored all of my branches to account for the cgo compatible changes you merged in from others.

I've now been made one of the platform owners for AIX on OpenTelemetry, and I'd really like to pull all of these changes in so that OTel's hostmetricsreceiver will work correctly. Is there anything I can provide for you to make testing these changes easier?

I have a working OTel collector with these changes baked in, and we can probably provide a 1 week rental of an AIX LPAR so you can test in whatever fashion you want. I'm also available to pair if we can find overlapping hours.

@shirou
Copy link
Copy Markdown
Owner

shirou commented Mar 21, 2026

Hi @Dylan-M, thank you for your work on improving AIX support — the WPAR/LPAR virtualization detection and the who-based user parsing are great additions.

However, this PR still contains several independent changes bundled together, which makes it difficult to review properly:

  1. Error handling change in InfoWithContext() — This affects all platforms, not just AIX, and changes the existing behavior. This needs careful, focused review on its own.
  2. Dependency injection for testability — A structural change to how the invoker is used in AIX code.
  3. New/rewritten functionality — UsersWithContext rewrite (w → who), VirtualizationWithContext implementation, parseWhoTimestamp addition.

Each of these has a different scope and risk profile. When they're combined in a single PR, it's hard to evaluate each change on its own merits, and a concern with one part blocks everything else.

I'm going to close this PR along with your other open PRs for now, as they share the same issue — the scope of each PR is too broad for us to review with confidence. I want to be upfront: as a maintainer with limited time, I simply cannot give large, multi-concern PRs the thorough review they deserve. Merging changes I haven't fully reviewed wouldn't be fair to you or to the project's users.

This is not a rejection of your contributions — I genuinely appreciate the effort you've put in. I'd welcome you to resubmit as smaller, focused PRs. As a general guideline:

  • One logical change per PR (e.g., one new feature, one refactor, or one bug fix)
  • Keep cross-platform behavioral changes separate from platform-specific additions
  • Smaller PRs get reviewed faster and are more likely to be merged

I'm happy to discuss how to best split things up if that would be helpful. Thanks for your understanding, and I hope to see your contributions again in a more reviewable form!

@shirou shirou closed this Mar 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants