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.

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

chore: tidy use of bpfSupportedMetrics for NewStats() #1629

Conversation

maryamtahhan
Copy link
Collaborator

@maryamtahhan maryamtahhan commented Jul 23, 2024

Tidy up the universal passing around of bpfSupportedMetrics for NewStats()

  • Introduce a reasonable default set of bpfSupportedMetrics in the stats package so that these can be used for NewStats() whenever it's invoked.
  • Remove the InitAvailableParamAndMetrics() function and simply initialise the variables in the package at declaration.
  • Remove dependency between bpf and stats packages entirely

Copy link
Contributor

github-actions bot commented Jul 23, 2024

🤖 SeineSailor

Here is a concise summary of the pull request changes:

Summary: The "chore: tidy universal use of bpfSupportedMetrics" pull request aims to standardize the use of bpfSupportedMetrics across the codebase. It introduces a contained lookup function, replacing the redundant !stats.AvailableBPFMetrics.HardwareCounters.Has(config.CPUTime) checks.

Key Modifications:

  1. Simplified code: The changes reduce code redundancy and improve organization.
  2. Improved maintainability: The consistent use of bpfSupportedMetrics makes the code easier to maintain.
  3. Potential behavioral impact: The changes might affect the initialization of available parameters and metrics.

Observations and Suggestions:

  • The pull request is still a work-in-progress (WIP), indicating that further refinements are needed.
  • It would be beneficial to include unit tests to ensure the new lookup function behaves as expected.
  • A thorough review of the affected code paths is recommended to ensure the changes do not introduce unintended consequences.

Overall, this pull request takes a step towards improving code consistency and maintainability. However, it requires further attention to ensure the changes do not impact the code's behavior.

@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch 2 times, most recently from 94c4ceb to 4716a13 Compare July 25, 2024 11:14
@maryamtahhan maryamtahhan marked this pull request as ready for review July 25, 2024 11:21
@maryamtahhan
Copy link
Collaborator Author

Hola @dave-tucker @sthaha @rootfs

Would really appreciate a review if you get the chance.

Thanks in advance
Maryam

Copy link
Collaborator

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

Following up from my earlier comment I feel like we're treating a symptom (bpfSupportedMetrics is being passed everywhere) and not the disease (why does bpfSupportMetrics NEED to be plumbed in so many places).

The bigger problem is that pkg/stats depending on pkg/bpf...
However, pkg/stats is regularly being called from parts of the codebase where a bpfExporter object isn't in scope.

I think the pkg/stats -> pkg/bpf dependency can be removed entirely by simply populating the hashmap with all the possible eBPF features. We're already filtering them before being posted to prometheus - see: CollectResUtilizationMetrics. That should clean up most of the mess.

I don't feel like the cleanup to make pkg/model depend on a maybe-unititialized value in pkg/stats is a good idea - depending on being provided a bpfSupportedStats struct seems fine.

cmd/exporter/exporter.go Outdated Show resolved Hide resolved
@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from 4716a13 to 6079d67 Compare July 31, 2024 11:13
pkg/bpf/exporter.go Outdated Show resolved Hide resolved
@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from 6079d67 to 862315a Compare July 31, 2024 11:31
@maryamtahhan maryamtahhan changed the title chore: tidy universal use of bpfSupportedMetrics chore: tidy use of bpfSupportedMetrics for NewStats() Jul 31, 2024
@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch 4 times, most recently from d2e2cc2 to 6d72d65 Compare August 2, 2024 11:40
@maryamtahhan
Copy link
Collaborator Author

maryamtahhan commented Aug 2, 2024

I've moved the stats globals cleanup to #1659 ... and i think this ready for another round of review

@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from 6d72d65 to 8c2565e Compare August 7, 2024 10:42
@rootfs
Copy link
Contributor

rootfs commented Aug 7, 2024

@maryamtahhan can you rebase to pick the e2e test diagnostics? thanks

@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from 8c2565e to 1fda657 Compare August 7, 2024 14:49
@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from 1fda657 to f77516b Compare August 19, 2024 09:09
@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch 2 times, most recently from 7e46b15 to b11f1f7 Compare September 2, 2024 09:57
@maryamtahhan
Copy link
Collaborator Author

Hi @dave-tucker
I think this is ready for review again. Please let me know if there are any other issues

Thanks in advance

pkg/bpf/exporter.go Outdated Show resolved Hide resolved
@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch 3 times, most recently from 4510ec0 to 718d171 Compare September 9, 2024 09:10
@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from 718d171 to ce31e90 Compare September 9, 2024 09:18
@maryamtahhan
Copy link
Collaborator Author

I will rebase this again

@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch 4 times, most recently from 698b1f8 to 1c223d2 Compare September 11, 2024 11:36
@maryamtahhan
Copy link
Collaborator Author

This is rebase now :) \o/ yay!

Copy link
Collaborator

@dave-tucker dave-tucker left a comment

Choose a reason for hiding this comment

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

LGTM pending resolution of merge conflict

@maryamtahhan maryamtahhan force-pushed the tidy-bpfSupportedMetrics branch from f95df6d to 7ea948d Compare October 17, 2024 10:11
Remove the need to pass bpfSupportedMetrics through
n layers of function calls that lead to NewStats()
by adding a reasonable default set of bpfmetrics
that the exporter will/can then override when it's
started.

Signed-off-by: Maryam Tahhan <[email protected]>
@maryamtahhan maryamtahhan merged commit 6de3263 into sustainable-computing-io:main Oct 18, 2024
20 checks passed
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.

3 participants