[fix]: add test case with darwin OS #1856

SamYuan1990 · 2024-11-21T07:15:18Z

add test case for config package.
remove unused function.
refactor function name to keep same naming convention.
refactor function behavior to same with function name.

github-actions · 2024-11-21T07:16:01Z

🤖 SeineSailor

Here is a concise summary of the pull request changes:

Summary:

This pull request primarily focuses on adding support for Darwin (MacOS) by removing platform-specific build restrictions, introducing test cases, and refactoring functions for consistency. The key modifications include:

Removing //go:build !darwin comments to allow building on Darwin.
Adding test cases for Darwin OS, including new files for handling eBPF programs.
Updating functions related to enabling/disabling metrics, refactoring function names and behavior for consistency, and removing unused functions.
Introducing new structs and functions for managing perf events and CPU cores.

Impact:

These changes may affect the external interface or behavior of the code, particularly:

Building on Darwin might result in changes to the external interface or behavior, depending on platform-specific functionality.
The updated functions for enabling/disabling metrics might impact other parts of the codebase that rely on the previous behavior.
Thorough testing on Darwin is essential to ensure compatibility and avoid unexpected behavior.

Suggestions:

Ensure comprehensive testing on Darwin to validate the changes and identify any potential issues.
Review the updated functions for enabling/disabling metrics to ensure they do not break existing functionality.
Consider adding documentation or comments to explain the changes and their implications for future maintainers.

SamYuan1990 · 2024-11-21T07:16:49Z

pkg/config/config.go

@@ -454,15 +454,6 @@ func isCGroupV2(c Client) bool {
 	return !os.IsNotExist(err)
 }

-// Get cgroup version, return 1 or 2


this function is no longer used.

SamYuan1990 · 2024-11-21T07:17:26Z

pkg/config/config.go

@@ -340,7 +340,7 @@ func SetEnabledIdlePower(enabled bool) {
 // SetEnabledGPU enables the exposure of gpu metrics
 func SetEnabledGPU(enabled bool) {
 	// set to true if any config source set it to true
-	instance.Kepler.EnabledGPU = enabled || instance.Kepler.EnabledGPU


if the original instance.Kepler.EnabledGPU is true, which seems the function will not set the GPU enable from true to false.

sthaha · 2024-11-24T23:59:38Z

pkg/bpf/fake_mac.go

+//go:build darwin
+// +build darwin


Kepler won't run on osx, so IMHO, we shouldn't spend time on it. WDYT?

well, just because I am a mac user.
as a mac user, when I notice/make some nit fix.
I want to test those locally.
so, I made some build tag on files to make it happen.
any other suggestions?

I am completely fine with supporting darwin as long as we don't block it in CI as OSX isn't a supported platform.
I also feel it is best to focus on linux as the platform kepler support and hence all development on OSX is expected to use linux box (lima may work well)

yep, I agree currently darwin is not major platform to run kepler.
so, @sthaha , can we make an agreement as following:

kepler currently focus on linux platform.
for other platforms, to make kepler is easy for anyone contributes from any platform, we are welcome any benefits(PRs) for kepler including parts as compilable on other platform.
before the specific platform is supported, we just running CI on linux as PR merge standard and official support.

as instance, this PR, increasing test coverage on config package(benefits), with parts as compilable on darwin.

sthaha · 2024-11-25T03:23:02Z

cmd/exporter/exporter.go

@@ -152,7 +152,7 @@ func main() {
 	platform.InitPowerImpl()
 	defer platform.StopPower()

-	if config.EnabledGPU() {
+	if config.IsEnabledGPU() {


If you want to rename this, could you please rename it as IsGPUEnabled() ?

sthaha · 2024-11-25T03:51:35Z

pkg/bpf/exporter.go

@@ -111,7 +114,7 @@ func (e *exporter) attach() error {
 		return fmt.Errorf("error attaching sched_switch tracepoint: %v", err)
 	}

-	if config.ExposeIRQCounterMetrics() {
+	if config.IsExposeIRQCounterMetrics() {


My preference is to leave it as is ..
IsExposeIRQCounterMetrics does not seem right. If we have to use Is for bool functions, I think the better option is IsIRQCounterMetricsExposed just like an English sentence.

sthaha · 2024-11-25T03:52:50Z

pkg/bpf/exporter.go

@@ -142,7 +145,7 @@ func (e *exporter) attach() error {
 	}

 	// Return early if hardware counters are not enabled
-	if !config.ExposeHardwareCounterMetrics() {
+	if !config.IsExposeHardwareCounterMetrics() {


Same as above, lets not change it.
config.ExposeHwCounterMetrics() reads better (proper English) than config.IsExposeHardwareCounterMetrics

SamYuan1990 · 2024-11-25T08:41:17Z

markdownlint-cli2-rules-docker...........................................Failed
- hook id: markdownlint-cli2-rules-docker
- exit code: 1
Unable to find image 'davidanson/markdownlint-cli2-rules:latest' locally

ref https://github.com/sustainable-computing-io/kepler/actions/runs/12005981954/job/33464291051#step:4:75
@maryamtahhan could you please check the lint hook works or we need update the link hook?

CONTRIBUTING.md

Signed-off-by: Sam Yuan <[email protected]>

SamYuan1990 · 2024-11-26T08:40:26Z

I think our commit msg check can't support Co-authored-by, as I added two commits from @maryamtahhan as suggestion change, as the suggestion LGTM.

CONTRIBUTING.md

Signed-off-by: Sam Yuan <[email protected]>

SamYuan1990 · 2024-11-27T06:01:37Z

@maryamtahhan , sorry, for short, I use force push to overwrite the approval for change suggestion, as webiny/action-conventional-commits#17

Signed-off-by: Sam Yuan <[email protected]>

SamYuan1990 · 2024-11-27T10:28:55Z

@rootfs , @sthaha , @vprashar2929 please help review this PR, if it looks good, let's merge it.

SamYuan1990 commented Nov 21, 2024

View reviewed changes

rootfs approved these changes Nov 21, 2024

View reviewed changes

SamYuan1990 marked this pull request as ready for review November 22, 2024 06:14

SamYuan1990 requested review from sthaha, vimalk78, dave-tucker, maryamtahhan, KaiyiLiu1234, vprashar2929 and rootfs November 22, 2024 06:14

rootfs assigned rootfs and vprashar2929 and unassigned rootfs Nov 22, 2024

sthaha reviewed Nov 24, 2024

View reviewed changes

sthaha reviewed Nov 25, 2024

View reviewed changes

SamYuan1990 requested a review from sthaha November 25, 2024 08:17

SamYuan1990 force-pushed the addTestCase branch from bb44a64 to 6474232 Compare November 25, 2024 08:23

maryamtahhan reviewed Nov 25, 2024

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

maryamtahhan reviewed Nov 25, 2024

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

maryamtahhan reviewed Nov 25, 2024

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

SamYuan1990 added 3 commits November 25, 2024 18:23

[fix]: add test case with darwin OS

1939d82

Signed-off-by: Sam Yuan <[email protected]>

[fix]: use same function naming conventions and behavior

19b8a94

Signed-off-by: Sam Yuan <[email protected]>

[fix]: update with review comments

7c374a2

Signed-off-by: Sam Yuan <[email protected]>

SamYuan1990 force-pushed the addTestCase branch from 6474232 to 7c374a2 Compare November 25, 2024 10:23

maryamtahhan reviewed Nov 26, 2024

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

maryamtahhan reviewed Nov 26, 2024

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

[fix]: update contributing.md as PR review comments

5576287

Signed-off-by: Sam Yuan <[email protected]>

SamYuan1990 force-pushed the addTestCase branch from 8d1b2f9 to 5576287 Compare November 27, 2024 05:59

SamYuan1990 requested a review from maryamtahhan November 27, 2024 06:05

[fix]: remove unused package

cb73095

Signed-off-by: Sam Yuan <[email protected]>

maryamtahhan approved these changes Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix]: add test case with darwin OS #1856

[fix]: add test case with darwin OS #1856

SamYuan1990 commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

SamYuan1990 Nov 21, 2024

SamYuan1990 Nov 21, 2024

sthaha Nov 24, 2024 •

edited

Loading

SamYuan1990 Nov 25, 2024

sthaha Nov 25, 2024

SamYuan1990 Nov 25, 2024

sthaha Nov 25, 2024

sthaha Nov 25, 2024

sthaha Nov 25, 2024

SamYuan1990 commented Nov 25, 2024

SamYuan1990 commented Nov 26, 2024

SamYuan1990 commented Nov 27, 2024

SamYuan1990 commented Nov 27, 2024

[fix]: add test case with darwin OS #1856

Are you sure you want to change the base?

[fix]: add test case with darwin OS #1856

Conversation

SamYuan1990 commented Nov 21, 2024 • edited Loading

github-actions bot commented Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamYuan1990 commented Nov 25, 2024

SamYuan1990 commented Nov 26, 2024

SamYuan1990 commented Nov 27, 2024

SamYuan1990 commented Nov 27, 2024

SamYuan1990 commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

sthaha Nov 24, 2024 •

edited

Loading