Skip to content

Conversation

@paragao
Copy link

@paragao paragao commented Nov 11, 2025

Review Findings for PR #885

Hi @rpovelik! I reviewed the merged PR #885 "EFA Node exporter updates" and found several areas for improvement that I've addressed in this follow-up PR.

Issues Found & Fixed:

🔧 Syntax Improvements:

  • Fixed missing case statements in parseAmazonEfaCounters function for existing counters
  • Added parsing logic for all 28 existing counters that were missing

📝 Code Quality Enhancements:

  • Standardized indentation and formatting throughout both files
  • Fixed typos in metric descriptions ("droped" → "dropped", "reponses" → "responses")
  • Improved error handling with more descriptive error messages including counter name and value

✅ Verification:

  • Confirmed all 5 new counters from your PR are properly integrated:
    • impaired_remote_conn_events
    • retrans_bytes
    • retrans_pkts
    • retrans_timeout_events
    • unresponsive_remote_events

Request for Review:

Could you please review these improvements and test them to ensure the EFA node exporter functionality works as expected? The changes maintain all your original functionality while fixing compilation issues and improving code maintainability.

Thanks for the great work on adding the new EFA counters! 🚀

… (PR #885 follow-up)

- Fix missing case statements in parseAmazonEfaCounters function
- Add all missing counter parsing logic for existing counters
- Standardize indentation and formatting consistency
- Improve error handling with more descriptive messages
- Fix typos in metric descriptions (dropped, responses)
- Ensure all 5 new counters from PR #885 are properly integrated

This addresses compilation issues and improves maintainability of the EFA node exporter code.
@paragao paragao requested a review from rpovelik November 11, 2025 10:58
@rpovelik
Copy link
Contributor

Hey Paulo,

Could you please explain why we shall add the counters in the list below?
They are not mentioned for the users in official doc.

alloc_pd_err
alloc_ucontext_err
cmds_err
completed_cmds
create_ah_err
create_cq_err
create_qp_err
keep_alive_rcvd
lifespan
mmap_err
no_completion_cmds
reg_mr_err
submitted_cmds

Otherwise, good looking PR. I need to run the tests on a cluster, but before that want to sort things out on the list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants