Context tracker manager #77
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the client context tracking architecture to fix a critical bug where the global registry was being overwritten by each new connection. The new architecture separates client-side and server-side responsibilities for better maintainability and proper context isolation.
- Introduced
ClientContextInjectorfor client-side injection of client info andManagerfor server-side management, replacing the monolithicClientContextTracker - Changed the protocol from simple JSON exchange to HTTP-based communication (POST /clientinfo with HTTP 200 OK response) for stream connections, and prefixed packets for datagram connections
- Updated tests to validate both scenarios: with and without client context tracking
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| tracker/clientcontext/tracker.go | Removed the old ClientContextTracker implementation and all connection wrapper types; retained only shared types (ClientInfo, MatchBounds, boundsRule) and updated package documentation |
| tracker/clientcontext/manager.go | New server-side component that reads client info from connections, stores it in context, and propagates it to registered trackers |
| tracker/clientcontext/injector.go | New client-side component that sends client info to the server after connection handshake using HTTP POST requests |
| tracker/clientcontext/tracker_test.go | Restructured integration tests with separate subtests for scenarios with and without client context tracking; removed unused imports |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
garmr-ulfr
force-pushed
the
context-tracker-manager
branch
from
a3ede52 to
c56378a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| req.Write(buf) | ||
| if _, err = conn.Write(req.Bytes()); err != nil { | ||
| return fmt.Errorf("writing client info: %w", err) | ||
| } |
There was a problem hiding this comment.
You could just use a normal http.Request here and then err := req.Write(conn) I think?
This PR refactors the client context tracking architecture to fix a critical bug where the client information was being stored in the global registry and overwriting it on each new connection.
Key changes include:
New architecture for client context tracking
ClientContextInjectorininjector.gofor sending client info from the client side, andManagerinmanager.gofor receiving and managing client info on the server side, replacing the previous unifiedClientContextTracker. This separation clarifies responsibilities and improves maintainability. [1] [2] [3] [4] [5]Context propagation and extensibility
Managerstores client info in connection context and allows other trackers to access it throughClientInfoFromContext, enabling richer downstream processing.Documentation and cleanup
ClientContextTrackerimplementation. Also cleaned up unused imports intracker_test.go. [1] [2]