Fixed Consistent Log Levelling #1490

PalmPalm7 · 2024-06-04T15:28:33Z

Edit: Have finished consistent log leveling issue.

Made changes to exisisting codebase with structured logs from format logs. Github issue ref.

Added logging kepler with klog section on dev/README.md, see here.

github-actions · 2024-06-04T15:29:59Z

🤖 SeineSailor

Here is a concise summary of the pull request changes:

Summary: This pull request primarily focuses on improving logging and error handling in the codebase. The key modifications include:

Logging updates: Replacing klog.Infof with klog.InfoS and updating log verbosity levels to provide more context for logged messages, enhancing structured logging.
Error handling: Introducing error handling for resolving container and VM IDs, making the code more robust.
Debugging flags: Adding flags for log debugging, allowing for more granular control over logging.

Impact: These changes improve the overall logging experience, providing more informative messages without affecting the external interface or behavior of the code.

Observations:

The changes are mostly related to logging and error handling, indicating a focus on improving the codebase's reliability and maintainability.
The introduction of InfoS and ErrorS functions in the golangci-lint configuration suggests a move towards more structured and informative logging.
The lack of changes in pkg/collector/stats/utils.go might indicate that this file was not affected by the logging updates or error handling changes.

Suggestions for improvement:

Consider adding more context or comments to explain the reasoning behind the logging updates and error handling changes, making it easier for future maintainers to understand the code.
Verify that the added debugging flags do not introduce any performance or security concerns.
Review the golangci-lint configuration to ensure it aligns with the project's coding standards and best practices.

dave-tucker

A few things in the rebase to fix up.
I've left some comments for small changes.
I think it's mostly when we're using InfoS (not preceeded by V(n)) with err as one of the values that we'd be better off using ErrorS.
That and some small changes to grammar or messages.

dave-tucker · 2024-06-20T15:12:38Z

cmd/exporter/exporter.go

+func finalizing() {
+	stack := "exit stack: \n" + string(debug.Stack())
+	klog.InfoS(stack)
+	exitCode := 10
+	klog.Infoln(finishingMsg)
+	klog.FlushAndExit(klog.ExitFlushTimeout, exitCode)
+}
+


This can go. It was removed in #1503

dave-tucker · 2024-06-20T15:14:11Z

cmd/exporter/exporter.go

+	klog.Infoln(finishingMsg)
+	klog.FlushAndExit(klog.ExitFlushTimeout, exitCode)
+}
+
 func main() {
 	start := time.Now()
 	klog.InitFlags(nil)


This registers the flags - you can see this from running kepler -help

dave-tucker · 2024-06-20T15:14:57Z

cmd/exporter/exporter.go

+	// for log debugging issues
+	flag.Set("logtostderr", "false")
+	flag.Set("alsologtostderr", "true")
+	flag.Set("log_dir", "/root/kepler/kepler_prime/kepler_debug/logs1.txt")


I don't think it's safe to assume this directory exists everywhere.
I'd probably remove it.

dave-tucker · 2024-06-20T15:15:12Z

cmd/exporter/exporter.go

 func main() {
 	start := time.Now()
 	klog.InitFlags(nil)
+
+	// for log debugging issues


Is this setting defaults that are different to what klog provides?

dave-tucker · 2024-06-20T15:15:38Z

cmd/exporter/exporter.go

+	flag.Set("alsologtostderr", "true")
+	flag.Set("log_dir", "/root/kepler/kepler_prime/kepler_debug/logs1.txt")
+	flag.Set("v", "2")
+


Have you tested that the log level can still be correctly overridden?

dave-tucker · 2024-06-20T15:31:26Z

pkg/collector/resourceutilization/cgroup/container_cgroup_collector.go

@@ -0,0 +1,59 @@
+/*


I believe this file was also deleted

dave-tucker · 2024-06-20T15:31:36Z

pkg/collector/stats/container_stats.go

@@ -66,3 +66,15 @@ func (c *ContainerStats) String() string {
 		c.ContainerID,
 	) + c.Stats.String()
 }
+


As was this function

dave-tucker · 2024-06-20T15:59:56Z

pkg/collector/stats/utils.go

 	}

+	// cgroup metric are deprecated and will be removed later


This was also removed in a recent PR

dave-tucker · 2024-06-20T16:00:37Z

pkg/collector/stats/utils.go

@@ -104,7 +110,7 @@ func GetNodeName() string {
 func getCPUArch() string {
 	arch, err := getCPUArchitecture()
 	if err == nil {
-		klog.V(3).Infof("Current CPU architecture: %s", arch)
+		klog.V(3).InfoS("Current CPU architecture:", "arch", arch)


Suggested change

klog.V(3).InfoS("Current CPU architecture:", "arch", arch)

klog.V(3).InfoS("Discovered CPU architecture", "arch", arch)

dave-tucker · 2024-06-20T16:02:32Z

pkg/config/config.go

+		klog.V(5).InfoS("Configuration value", "ENABLE_EBPF_CGROUPID", EnabledEBPFCgroupID)
+		klog.V(5).InfoS("Configuration value", "ENABLE_GPU", EnabledGPU)
+		klog.V(5).InfoS("Configuration value", "ENABLE_QAT", EnabledQAT)
+		klog.V(5).InfoS("Configuration value", "ENABLE_PROCESS_METRICS", EnableProcessStats)
+		klog.V(5).InfoS("Configuration value", "EXPOSE_HW_COUNTER_METRICS", ExposeHardwareCounterMetrics)
+		klog.V(5).InfoS("Configuration value", "EXPOSE_CGROUP_METRICS", ExposeCgroupMetrics)
+		klog.V(5).InfoS("Configuration value", "EXPOSE_IRQ_COUNTER_METRICS", ExposeIRQCounterMetrics)


You can make this all one line:

Suggested change

klog.V(5).InfoS("Configuration value", "ENABLE_EBPF_CGROUPID", EnabledEBPFCgroupID)

klog.V(5).InfoS("Configuration value", "ENABLE_GPU", EnabledGPU)

klog.V(5).InfoS("Configuration value", "ENABLE_QAT", EnabledQAT)

klog.V(5).InfoS("Configuration value", "ENABLE_PROCESS_METRICS", EnableProcessStats)

klog.V(5).InfoS("Configuration value", "EXPOSE_HW_COUNTER_METRICS", ExposeHardwareCounterMetrics)

klog.V(5).InfoS("Configuration value", "EXPOSE_CGROUP_METRICS", ExposeCgroupMetrics)

klog.V(5).InfoS("Configuration value", "EXPOSE_IRQ_COUNTER_METRICS", ExposeIRQCounterMetrics)

klog.V(5).InfoS("Configuration values", "ENABLE_EBPF_CGROUPID", EnabledEBPFCgroupID, "ENABLE_GPU", EnabledGPU, "ENABLE_QAT", EnabledQAT,"ENABLE_PROCESS_METRICS", EnableProcessStats, "EXPOSE_HW_COUNTER_METRICS", ExposeHardwareCounterMetrics, "EXPOSE_CGROUP_METRICS", ExposeCgroupMetrics, "EXPOSE_IRQ_COUNTER_METRICS", ExposeIRQCounterMetrics)

dave-tucker · 2024-06-20T17:05:41Z

@PalmPalm7 when you've fixed the issues with the rebase, you might want to run make format to ensure all your Go code is properly formatted, and make golint also to catch any issues.

PalmPalm7 · 2024-06-20T17:29:27Z

@PalmPalm7 when you've fixed the issues with the rebase, you might want to run make format to ensure all your Go code is properly formatted, and make golint also to catch any issues.

Thank you for the Review Dave, I'll make changes to ensure they are properly formatted.

PalmPalm7 force-pushed the main branch 2 times, most recently from d8849d7 to 14c8f70 Compare June 20, 2024 12:31

PalmPalm7 marked this pull request as ready for review June 20, 2024 12:33

PalmPalm7 changed the title ~~"place-holder commit/PR"~~ Fixed Consistant Log Levelling Jun 20, 2024

PalmPalm7 changed the title ~~Fixed Consistant Log Levelling~~ Fixed Consistent Log Levelling Jun 20, 2024

PalmPalm7 force-pushed the main branch 2 times, most recently from 1400dc5 to 77d4005 Compare June 20, 2024 14:41

PalmPalm7 mentioned this pull request Jun 20, 2024

Consistent Log Levelling #1469

Open

dave-tucker requested changes Jun 20, 2024

View reviewed changes

PalmPalm7 closed this Jul 11, 2024

PalmPalm7 force-pushed the main branch from 77d4005 to 39223f7 Compare July 11, 2024 14:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Consistent Log Levelling #1490

Fixed Consistent Log Levelling #1490

PalmPalm7 commented Jun 4, 2024 •

edited

Loading

github-actions bot commented Jun 4, 2024 •

edited

Loading

dave-tucker left a comment

dave-tucker Jun 20, 2024

dave-tucker Jun 20, 2024

dave-tucker Jun 20, 2024

dave-tucker Jun 20, 2024

dave-tucker Jun 20, 2024

dave-tucker Jun 20, 2024

dave-tucker Jun 20, 2024

dave-tucker Jun 20, 2024

dave-tucker Jun 20, 2024

dave-tucker Jun 20, 2024

dave-tucker commented Jun 20, 2024

PalmPalm7 commented Jun 20, 2024

	klog.V(3).InfoS("Current CPU architecture:", "arch", arch)
	klog.V(3).InfoS("Discovered CPU architecture", "arch", arch)

Fixed Consistent Log Levelling #1490

Fixed Consistent Log Levelling #1490

Conversation

PalmPalm7 commented Jun 4, 2024 • edited Loading

github-actions bot commented Jun 4, 2024 • edited Loading

dave-tucker left a 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

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

dave-tucker commented Jun 20, 2024

PalmPalm7 commented Jun 20, 2024

PalmPalm7 commented Jun 4, 2024 •

edited

Loading

github-actions bot commented Jun 4, 2024 •

edited

Loading