refactor: rely on context.Context for log/slog and others #1969
Conversation
| type contextHolder struct { | ||
| ctx context.Context | ||
| frankenPHPContext *frankenPHPContext | ||
| } |
There was a problem hiding this comment.
Can't you just put ctx inside of frankenPHPContext ?
There was a problem hiding this comment.
This would create a circular reference. I would prefer not to.
There was a problem hiding this comment.
Actually, just calling Value() as I was doing before is likely good enough.
There was a problem hiding this comment.
We already have a cyclycal dependency between frankenphpContext and http.request. Getting rid of it was the main idea behind #1857.
Would feel cleaner to just pass around a single object internally.
There was a problem hiding this comment.
It would be nice to remove all circular references, indeed.
|
I created a small benchmark to compare using As the API is entirely internal, and we can call it a lot in C<->Go loops, let's keep this as-is. |
09fdb5a to
668597e
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase to rely on context.Context for logging and other context-aware operations, providing OpenTelemetry support and a cleaner API. The changes introduce a contextHolder struct to pass both the Go context and FrankenPHP context together through the system.
Key changes:
- Introduced
contextHolderstruct to couplecontext.ContextwithfrankenPHPContext - Replaced global
loggerwithglobalLoggerandglobalCtxfor better context management - Added context parameter to logging functions and the watcher initialization
- Wrapped logging calls with
Enabled()checks for performance optimization - Updated worker and thread interfaces to return both
context.ContextandfrankenPHPContext
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| context.go | Introduced contextHolder struct to couple context with frankenPHPContext |
| frankenphp.go | Replaced global logger with globalLogger and globalCtx, added context-aware logging |
| frankenphp.c | Updated frankenphp_log_message to pass thread index for context retrieval |
| options.go | Added WithContext option to set the main context |
| phpthread.go | Updated thread interface to return context and frankenPHPContext separately |
| threadregular.go | Embedded contextHolder in regularThread, updated to use context |
| threadworker.go | Split context storage into workerContext and workerFrankenPHPContext |
| threadinactive.go | Added context methods returning nil for inactive threads |
| threadtasks_test.go | Added context methods for task threads |
| worker.go | Changed channels to use contextHolder instead of *frankenPHPContext |
| workerextension.go | Updated SendMessage to accept and use context |
| scaling.go | Updated logging to use global context and logger |
| phpmainthread.go | Updated logging to use global context and logger |
| internal/watcher/watcher.go | Added context parameter to InitWatcher and logging functions |
| internal/watcher/watch_pattern.go | Updated logging to use passed context |
| internal/testserver/main.go | Added context creation and usage |
| frankenphp_test.go | Added TestMain to suppress logs in non-verbose mode |
| phpmainthread_test.go | Added setupGlobals helper to properly initialize test contexts |
| types_test.go | Updated to use globalLogger instead of logger |
| workerextension_test.go | Updated test to pass context to SendMessage |
| cgi.go | Updated to use frankenPHPContext() method |
| caddy/app.go | Updated Stop method to use context-aware logging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@henderkes, do you have any idea how we can save some space and prevent the "no space left on device" error when building the mostly static binary? |
|
That's funny, I started getting the same in my CI in the past five days and have tried to fix it. It only happens on old runners (el7, el8). The containers either report "no space left on device" or "connection lost". I really don't know, unfortunately. GitHub must have changed something. |
|
The error doesn't even make any sense, it manages to build many more extensions in my CI, including |
|
Otherwise, we'll have to disable grpc I guess... |
|
I wouldn't mind it, gRPC takes up over 1/3 of my CI time and more than half the space of the containers. But it's an absolute pain to compile as a user (the pecl and pie releases are broken, because they don't contain submodules, they'll be missing the tools and it takes north of half an hour on a normal PC). |
Coupled with caddyserver/caddy#7346, this will give full OpenTelemetry support, and a cleaner API.