CSL-10969: Add the -capture flag to proxy log command. #4788

LordAbhishek · 2025-09-22T07:42:02Z

Changes proposed in this PR

Added -capture flag to proxy log command.
This will enable user to capture log for given duration and write them to a file in current working dir.
If -update-level flag is provided along with -capture flag, it will set given new log level, capture the log for given duration and reset the log level to existing one post log capture.
consul-k8s proxy log <podname> -capture 1m -update-level debug

Signal interrupt handling:

Here, with existing base command setup, if signal interrupt is received during log capture, it would exit the terminal at an instant, so log levels won't be reset back to existing levels.
So, to handle the signal interrupt while capturing log, I have modified main.go, commands.go, common/base.go which will handle signal interrupts with the help of dual channel for any cli command in consul-k8s.
With the new setup of common/base.go and main.go, it will reset back the log levels back to previous one, basically main will wait unitll reset.

* feat: `proxy log <podname> -capture 30s` cmd will capture the log and write logs to a file * feat: `proxy log <podname> -capture 30s -update-level warning` cmd will capture the log with provided envoy log level (warning), write logs to a file and reset envoy log level back to existing levels * feat: modified cli/main.go, cli/command.go and cli/common/base.go to support graceful handle signal interrupts for any commands.

hashicorp-cla-app · 2025-09-22T07:42:16Z

All committers have signed the CLA.

hashicorp-cla-app · 2025-09-22T07:42:16Z

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

_{Have you signed the CLA already but the status is still pending? Recheck it.}

This reverts commit 7a57d43.

sriramr98 · 2025-09-26T06:14:59Z

.changelog/4788.txt

@@ -0,0 +1,3 @@
+```release-note:feature
+cli: add new `-capture` flag to proxy loglevel cmd. Also updated main.go, command.go and common/base.go to support graceful handle signal interrupts for any commands


This a public changelog which will be published. Need a more professional changelog. This looks more like a commit message than a changelog

cli/cmd/proxy/loglevel/command.go

sriramr98 · 2025-09-26T06:29:33Z

cli/cmd/proxy/loglevel/command.go

+
+func (l *LogLevelCommand) fetchExistingLogLevels(adminPorts map[string]int) (map[string]LoggerConfig, error) {
+	l.UI.Output(fmt.Sprintf("Fetching existing log levels..."))
+	existingLogger := make(map[string]LoggerConfig, 0)


Why are we creating a new map here only to be overriden at line 359?

sriramr98 · 2025-09-26T06:34:24Z

cli/cmd/proxy/loglevel/command.go

+	l.UI.Output(fmt.Sprintf("Fetching existing log levels..."))
+	existingLogger := make(map[string]LoggerConfig, 0)
+	newLogLevels := l.level
+	l.level = ""


Why are we updating it here just to override it again at line 360?

sriramr98 · 2025-09-26T06:38:43Z

cli/cmd/proxy/loglevel/command.go

+	existingLogger := make(map[string]LoggerConfig, 0)
+	newLogLevels := l.level
+	l.level = ""
+	existingLogger, err := l.fetchOrSetLogLevels(adminPorts)


The function fetchExistingLogLevels denotes it's only fetching the existing level, but calling fetchOrSetLogLevels creates a side effects which is not a good coding practise.

Either change the function name fetchExistingLogLevels to something that more describes what the function does or move the set to outside the function

removed fetchExistingLogLevels.
updated fetchOrSetLogLevels to accept 2 parameters - fetchOrSetLogLevels(adminPorts map[string]int, logLevel string).

Previously, fetchOrSetLogLevels was designed to work as both getter and setter & it was controlled by l.level flag.
Currently, we could control the behaviour of it with the help of logLevel parameter, so that we could use it whereever possible without modifying l.level (no side-effects).

Let me know if i need to modify this one.. i also tried to seperate the get and set functions.. that was also working perfectly fine but was it was repeating one. so removed that one and modified fetchOrSetLogLevels.

sriramr98 · 2025-09-26T06:45:24Z

cli/cmd/proxy/loglevel/command.go

+	if err != nil {
+		return fmt.Errorf("error parsing capture's duration")
+	}
+	durationChn := time.After(duration)


Create the durationChn in line 445. This is because we cann k8s api in line 423 which is an API call and will have some delay. This time will be consumed in the duration and the channel will fire earlier.

For example, if the duration is 0.5 seconds, but the api takes 0.6 seconds, then the select statement will immediately execute and won't wait for that 0.5 seconds.

Here's an example code to understand why this isn't an optimal implementation

package main import "time" func main() { start := time.Now() duration := time.Duration(5 * time.Second) durationCh := time.After(duration) println(duration.String()) time.Sleep(3 * time.Second) startSelect := time.Now() select { case <-durationCh: println("Duration elapsed") println("Elapsed time since select:", time.Since(startSelect).String()) println("Total elapsed time:", time.Since(start).String()) } }

cli/cmd/proxy/loglevel/command.go

sriramr98 · 2025-09-26T06:46:43Z

cli/cmd/proxy/loglevel/command.go

+		// Create file path and directory for storing logs
+		// NOTE: currently it is writing log file in cwd /proxy only. Also, log file contents will be overwritten if
+		// the command is run multiple times for the same pod name or if file already exists.
+		if err := os.MkdirAll(filepath.Dir(proxyLogFilePath), 0755); err != nil {


Avoid magic numbers. Move 0755 to a constant with a descriptive variable name

sriramr98 · 2025-09-26T06:46:50Z

cli/cmd/proxy/loglevel/command.go

+		if err := os.MkdirAll(filepath.Dir(proxyLogFilePath), 0755); err != nil {
+			return fmt.Errorf("error creating directory for log file: %w", err)
+		}
+		if err := os.WriteFile(proxyLogFilePath, logs, 0644); err != nil {


Avoid magic numbers. Move 0644 to a constant with a descriptive variable name

sriramr98 · 2025-09-26T06:48:53Z

cli/cmd/proxy/loglevel/command.go

+	return err
+}
+
+func (l *LogLevelCommand) fetchExistingLogLevels(adminPorts map[string]int) (map[string]LoggerConfig, error) {


The logic of this function is very confusing. Can you add unit tests specific to this function to ensure the implementation works as expected?

…s parameter

Copilot

Pull Request Overview

This PR adds the -capture flag to the proxy log command, enabling users to capture logs for a specified duration and write them to a file in the current working directory. The implementation includes signal interrupt handling to ensure proper cleanup of log levels when users interrupt the command.

Adds -capture flag to proxy log command for capturing logs for a specified duration
Implements signal interrupt handling across the CLI to ensure graceful cleanup
Enables log level reset functionality when capturing logs with new log levels

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
cli/main.go	Adds cleanup channels and signal handling infrastructure
cli/common/base.go	Extends BaseCommand with cleanup channel fields
cli/commands.go	Injects cleanup channels into proxy log command
cli/cmd/proxy/loglevel/command.go	Implements log capture functionality and cleanup handling
cli/cmd/proxy/loglevel/command_test.go	Adds comprehensive tests for new capture functionality
.changelog/4788.txt	Documents the new feature

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-30T04:41:40Z

cli/cmd/proxy/loglevel/command.go

+		l.UI.Output("Logs saved to '%s'", proxyLogFilePath, terminal.WithSuccessStyle())
+		return nil
+	case <-l.Ctx.Done():
+		return fmt.Errorf("stopping collection due to shutdown signal recieved")


Misspelled 'received' as 'recieved' in error message.

Suggested change

return fmt.Errorf("stopping collection due to shutdown signal recieved")

return fmt.Errorf("stopping collection due to shutdown signal received")

LordAbhishek added 2 commits September 21, 2025 22:31

adding the changelog

7a57d43

LordAbhishek requested a review from a team as a code owner September 22, 2025 07:42

LordAbhishek added area/cli CLI pr/no-backport signals that a PR will not contain a backport label labels Sep 22, 2025

LordAbhishek added 3 commits September 22, 2025 13:45

Revert "adding the changelog"

e0c36d5

This reverts commit 7a57d43.

adding correct changelog

95be915

minor changes

4696b0c

LordAbhishek mentioned this pull request Sep 26, 2025

CSL-10969: Add consul-k8s debug command to consul-k8s cli. #4800

Open

sriramr98 requested changes Sep 26, 2025

View reviewed changes

LordAbhishek added 2 commits September 27, 2025 14:00

minor code fixes and refactored fetchOrSetLogLevels to accept level a…

96511e1

…s parameter

updated changelog

66da372

LordAbhishek requested a review from Copilot September 30, 2025 04:41

Copilot AI reviewed Sep 30, 2025

View reviewed changes

		@@ -0,0 +1,3 @@
		```release-note:feature
		cli: add new `-capture` flag to proxy loglevel cmd. Also updated main.go, command.go and common/base.go to support graceful handle signal interrupts for any commands

	return fmt.Errorf("stopping collection due to shutdown signal recieved")
	return fmt.Errorf("stopping collection due to shutdown signal received")

CSL-10969: Add the -capture flag to proxy log command. #4788

Are you sure you want to change the base?

CSL-10969: Add the -capture flag to proxy log command. #4788

Conversation

LordAbhishek commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this PR

Signal interrupt handling:

Uh oh!

hashicorp-cla-app bot commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hashicorp-cla-app bot commented Sep 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LordAbhishek Sep 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LordAbhishek commented Sep 22, 2025 •

edited

Loading

hashicorp-cla-app bot commented Sep 22, 2025 •

edited

Loading

LordAbhishek Sep 27, 2025 •

edited

Loading