feat: runtime-configurable upstream request logging #37

dannykopping · 2025-10-22T14:39:45Z

Disclaimer: largely implemented by Claude Code.

It's often very useful to see the exact requests made to, and responses received from, upstream providers.

This is both useful for development and for production troubleshooting.

See coder/coder#20414 for the concomitant changes in coder/coder.

Signed-off-by: Danny Kopping <[email protected]>

dannykopping · 2025-10-22T18:55:53Z

chore: set cache control markers on tools #38
feat: runtime-configurable upstream request logging #37 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

deansheather · 2025-10-23T12:24:55Z

config.go


+import "go.uber.org/atomic"
+
 type ProviderConfig struct {


This is a lot especially given that the baseURL and key won't change during a provider's lifetime within Coder.

Have you considered moving the methods for changing upstream logging settings to the provider rather than having the config be mutable? In Coder for example, we don't change the coderd.Options type after passing it to the API.

I'll have a look.

deansheather · 2025-10-23T12:25:09Z

config.go

+	upstreamLoggingDir    atomic.String
+	enableUpstreamLogging atomic.Bool


Does this need to be two values? An empty string could denote that it's disabled

An empty upstreamLoggingDir defaults to os.TempDir. We may want to allow configuration of the path in the future, so this is forward-looking.

deansheather · 2025-10-23T12:29:34Z

request_logger.go

+	resLogger := log.New(resLogFile, "", log.LstdFlags)
+
+	return func(req *http.Request, next func(*http.Request) (*http.Response, error)) (*http.Response, error) {
+		logUpstreamRequest(reqLogger, id, model, req)


They're all going to be appended to the same file, which might make it confusing. I'd argue for moving the OpenFile calls into the middleware itself and appending an integer that is incremented on each request. E.g. ${interception_id}-${i}.req.log

Each interception is generally a single upstream request, so it'll be separate files.

For interceptions with injected tool calls this is not the case, so I'm suggesting that for those requests we have separate files like -1 -2 -3 etc. for each request

Ah yes, good point 👍

deansheather · 2025-10-23T12:30:00Z

request_logger.go

+	reqLogPath := filepath.Join(logDir, fmt.Sprintf("%s.req.log", id))
+	resLogPath := filepath.Join(logDir, fmt.Sprintf("%s.res.log", id))
+
+	reqLogFile, err := os.OpenFile(reqLogPath, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644)


Never closed?

If the OpenFile was moved into the actual middleware function like I suggested in another comment, would make it much easier to defer it.

Doh!

Middleware will make this more difficult; I'll keep it as-is but add closing logic, thanks.

deansheather · 2025-10-23T12:31:03Z

request_logger.go

+	reqLogger := log.New(reqLogFile, "", log.LstdFlags)
+	resLogger := log.New(resLogFile, "", log.LstdFlags)


The log package isn't really doing much for you here, I think you'd pretty much get the same effect by just fmt.Fprintlning to the file directly.

deansheather · 2025-10-23T12:33:18Z

request_logger.go

+		baseDir = os.TempDir()
+	}
+
+	logDir := filepath.Join(baseDir, provider, safeModel)


I could see the argument for subdividing directories based on provider (or endpoint), but don't fully understand what splitting by model does.

Claude Code does this ugly thing where it sends a bunch of requests to the Haiku class of models, so I wanted to split that out from normal prompts (presumably using Sonnet or Opus).

deansheather · 2025-10-23T12:33:55Z

request_logger.go

+		"\"", repl,
+		"<", repl,
+		">", repl,
+		"|", repl,


^.$ or ^..$ need to be changed to something else or they'll trip filepath.Join

config.go

pawbana · 2025-10-23T14:45:51Z

config.go

+
+// UpstreamLoggingDir returns the base directory for upstream logging.
+// If empty, the OS's tempdir will be used.
+// Logs are written to $UpstreamLoggingDir/$provider/$model/$interceptionID.{req,res}.log


File per interception seems like a lot of files. For short debugging it should be fine but not in general. Maybe instead of directory structure / separate files structured logging could work?

Structured logging won't be easy to read because of the large payloads.
You're correct it'll be a lot of files. The point of the runtime enable/disable is so that it can be turned on for short periods of time.
I'll add docs to coder/coder#20414 to indicate this.

pawbana · 2025-10-23T15:15:22Z

request_logger.go

+	}
+
+	if reqDump, err := httputil.DumpRequest(req, true); err == nil {
+		logger.Printf("[req] [%s] [%s] %s", id, model, reqDump)


Since model name is already in the filename (well sanitized) then maybe it is not needed in log prefix?

You're right, thanks! I added the model name to the path later. I'll remove this.

pawbana · 2025-10-23T15:16:41Z

request_logger.go

+		return
+	}
+
+	if respDump, err := httputil.DumpResponse(resp, true); err == nil {


err is ignored

Not much we can do if there is an err, is there?

yeah, just linter may complain

dannykopping mentioned this pull request Oct 22, 2025

feat: toggle aibridge upstream request logging coder/coder#20414

Open

dannykopping force-pushed the req_logging branch from cade1ee to 6f25879 Compare October 22, 2025 18:17

dannykopping requested review from deansheather and pawbana October 22, 2025 18:17

dannykopping marked this pull request as ready for review October 22, 2025 18:17

dannykopping added 5 commits October 22, 2025 20:17

chore: req logging

785748c

Signed-off-by: Danny Kopping <[email protected]>

chore: enable runtime request logging

a8b8db2

Signed-off-by: Danny Kopping <[email protected]>

chore: clean up

0452dc8

Signed-off-by: Danny Kopping <[email protected]>

chore: refactoring, tests

a4c07af

Signed-off-by: Danny Kopping <[email protected]>

chore: ensure all configs are concurrency-safe

7a3c4bf

Signed-off-by: Danny Kopping <[email protected]>

dannykopping force-pushed the req_logging branch from 6f25879 to 7a3c4bf Compare October 22, 2025 18:18

dannykopping mentioned this pull request Oct 22, 2025

chore: set cache control markers on tools #38

Closed

deansheather reviewed Oct 23, 2025

View reviewed changes

pawbana reviewed Oct 23, 2025

View reviewed changes

config.go Show resolved Hide resolved

pawbana reviewed Oct 23, 2025

View reviewed changes

		upstreamLoggingDir atomic.String
		enableUpstreamLogging atomic.Bool

		reqLogger := log.New(reqLogFile, "", log.LstdFlags)
		resLogger := log.New(resLogFile, "", log.LstdFlags)

feat: runtime-configurable upstream request logging #37

Are you sure you want to change the base?

feat: runtime-configurable upstream request logging #37

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dannykopping commented Oct 22, 2025 •

edited

Loading

dannykopping commented Oct 22, 2025 •

edited

Loading