Refactor calls to debug messages in all services by javuto · Pull Request #632 · jmpsec/osctrl

javuto · 2025-03-28T16:12:58Z

Refactor in osctrl-tls, osctrl-admin and osctrl-api to all calls of debug messages to avoid getting the setting value from the backend.
Added three new flags to all services to set debug HTTP settings.

--enable-http-debug
--http-debug-file
--http-debug-show-body

When enabled, the debug HTTP logs will go into a separate file.

Copilot

Pull Request Overview

This PR refactors calls to debug messages across API and Admin handlers, replacing dynamic backend settings retrieval with static boolean flags. Key changes include:

Removing dependency on settings by replacing h.Settings.DebugHTTP(...) with fixed true/false values.
Cleaning up unused imports related to the settings package.
Standardizing the debug flag usage across services (osctrl-api, osctrl-admin, and osctrl-tls).

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated no comments.

File	Description
cmd/api/handlers/*.go	Replaced dynamic debug flag call with static booleans.
cmd/admin/handlers/*.go	Updated all debug calls to use fixed values and removed settings imports.

javuto · 2025-03-28T16:40:58Z

@zhuoyuan-liu please take a look, thanks!

zhuoyuan-liu

Good job! I left a small comment. However, I do have some suggestions if you're interested in further modernizing osctrl.

Reducing Global Variables and Direct Dependencies
Relying too much on global variables and direct dependencies can make the code messy and hard to manage. It can lead to unexpected bugs, tricky testing, and parts of the code depending on each other in ways that aren’t obvious. Using dependency injection helps by making it clear what each part of the code needs, making testing easier, and allowing components to be reused more easily.
Separating Handler Responsibilities
When handlers handle too much—like HTTP requests, business logic, and database work—it leads to messy and repetitive code. Splitting things into layers, where handlers only deal with HTTP, services handle logic, and repositories manage data, makes the code easier to test, reuse, and maintain. It also keeps errors and responses consistent across the application.

zhuoyuan-liu · 2025-03-28T20:40:46Z

pkg/utils/http-utils.go

-		log.Debug().Msg(DebugHTTP(r, debugCheck, showBody))
-	}
+func DebugHTTPDump(r *http.Request, showBody bool) {
+	log.Debug().Msg(DebugHTTP(r, showBody))


In this case, the whole DebugHTTP function will be executed. This can lead to unnecessary overhead. To avoid this overhead, you should conditionally check the log level before executing the operations:

if log.Debug().Enabled() { // something }

Just pushed these changes, let me know what you think! And if you have more examples of how to implement the two points mentioned above, feel free to point them out, I am always happy to improve osctrl!

I would suggest putting the check in this function to make the code more readable.

func DebugHTTPDump(r *http.Request, showBody bool) { if log.Debug().Enabled() { log.Debug().Msg(DebugHTTP(r, showBody)) } }

zhuoyuan-liu · 2025-03-29T17:26:48Z

pkg/utils/http-utils.go

-		log.Debug().Msg(DebugHTTP(r, debugCheck, showBody))
-	}
+func DebugHTTPDump(r *http.Request, showBody bool) {
+	log.Debug().Msg(DebugHTTP(r, showBody))


I would suggest putting the check in this function to make the code more readable.

func DebugHTTPDump(r *http.Request, showBody bool) { if log.Debug().Enabled() { log.Debug().Msg(DebugHTTP(r, showBody)) } }

zhuoyuan-liu · 2025-03-29T17:49:52Z

pkg/utils/http-utils.go

-		}
-		debug += fmt.Sprintf("%s\n", "---------------- end")
+	debug = fmt.Sprintf("%s\n", "---------------- request")
+	requestDump, err := httputil.DumpRequest(r, showBody)


I just realized that it logs everything from the request, which seems excessive and comes with several drawbacks:

Log Volume – It generates a large number of logs, making search and analysis more difficult.

Security Concerns – Some headers may contain sensitive information that should not be logged.

Formatting – I'm unsure how this would look when integrated with zerolog. Do you have an example?

In production, these logs might add too much noise, making it harder to diagnose issues when something goes wrong. I’d love to hear more about how you use these logs and whether there are ways to refine them.

This is a good point, I believe this was a function I used a lot in the early stages of the osctrl development, but now it would make more sense to:

Have the ability to only dump HTTP traffic for certain nodes and not every received request.

Format the output to follow zerolog output, so it will be easier to utilize, even log to a different log file.

Does it make sense to remove it completely?

If these logs have been rarely used recently, I’d suggest removing them. In our use case, osctrl handles 100+ requests per second, and these logs would likely add unnecessary pressure.

I understand they might be useful in certain cases, especially during development. If we need to keep them, we could use a feature flag to control logging, enabling it only in the development environment. So, it would not be mixed with other debug log messages.

E.g. --enable-http-dump=true

I just added a new commit with three new flags in all services:

--enable-http-debug

--http-debug-file

--http-debug-show-body

So if needed, this must be enabled with a flag, which will reduce the risk of logs taking up too much space. And the logs will go to a different file and not stdout.

Let me know what you think! Thanks!

zhuoyuan-liu

LGTM. However, I believe that as osctrl matures, we will eventually remove these debug logs since they reduce code readability and provide limited value.

zhuoyuan-liu · 2025-04-02T08:23:09Z

cmd/api/handlers/handlers.go


+func WithDebugHTTP(logger *zerolog.Logger, cfg *config.DebugHTTPConfiguration) HandlersOption {
+	return func(h *HandlersApi) {
+		h.DebugHTTP = logger


It could be a good example of "Reducing Global Variables and Direct Dependencies". Instead of relying on the global log variable/package, you can assign the logger to your main server(strcut).

zhuoyuan-liu · 2025-04-02T08:26:36Z

cmd/tls/main.go

+	// If enabled, prepare debug HTTP logger
+	if debugHTTP.Enabled {
+		// Open or create the debug HTTP log file (append mode)
+		debugFile, err := os.OpenFile(


Do you have any idea about the log file truncate or rotation? If it is deployed in k8s cluster or docker, you may need extra tools/steps for handling these log files. Since we would never enable them in Prod, it should be fine.

I can probably do something like https://github.com/jmpsec/osctrl/blob/main/pkg/logging/file.go to handle logs rotation. But for now, since this should be enabled only in dev, and it will require the flag and service restart, there should not be any mistake and it ends up turned on. I will send a PR next to cleanup all the DebugHTTP in osctrl-admin and settings. Thanks!

Refactor calls to debug messages

eb20cf1

javuto added osctrl-tls osctrl-tls related changes osctrl-api osctrl-api related changes osctrl-admin osctrl-admin related changes labels Mar 28, 2025

javuto requested a review from Copilot March 28, 2025 16:12

Copilot AI reviewed Mar 28, 2025

View reviewed changes

zhuoyuan-liu reviewed Mar 28, 2025

View reviewed changes

Check if debug is enabled before call function DebugHTTPDump

ce63e26

zhuoyuan-liu reviewed Mar 29, 2025

View reviewed changes

Using flag to enable Debug HTTP logging

ff48ebf

zhuoyuan-liu reviewed Apr 2, 2025

View reviewed changes

javuto added 3 commits April 2, 2025 23:16

Using lumberjack to handle log rotation for debug http

3c845b3

Better output for dumped HTTP requests

e847cdb

Better output for dumped HTTP requests

06d2234

javuto merged commit 091fd7b into main Apr 2, 2025
53 checks passed

javuto deleted the refactor-debug-tls branch April 2, 2025 21:52

BrewTestBot mentioned this pull request Aug 9, 2025

osctrl-cli 0.4.5 Homebrew/homebrew-core#232922

Merged

Comments

Conversation

javuto commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

javuto commented Mar 28, 2025

Uh oh!

zhuoyuan-liu left a comment

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!

javuto Mar 29, 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!

zhuoyuan-liu left a comment

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

javuto commented Mar 28, 2025 •

edited

Loading

javuto Mar 29, 2025 •

edited

Loading