-
Notifications
You must be signed in to change notification settings - Fork 0
feat(datacap): implement datacap tracking for connections #53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements comprehensive datacap tracking functionality for network connections, enabling bandwidth monitoring and throttling capabilities for device-specific data consumption limits.
Key Changes:
- Introduces datacap-aware connection wrappers (
ConnandPacketConn) that track data consumption and support bandwidth throttling - Implements token bucket-based throttling algorithm with asymmetric rate limiting (throttles downloads more than uploads when capped)
- Adds client infrastructure for communicating with a datacap sidecar service for reporting usage and checking quota status
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| option/datacap.go | Defines configuration options for datacap-wrapped inbound connections including sidecar URL, reporting intervals, and throttling settings |
| datacap/wrapper.go | Provides optional datacap tracking wrapper that can wrap connections with tracking functionality or pass through unchanged |
| datacap/throttler.go | Implements token bucket-based bandwidth throttling with separate read/write rate control |
| datacap/packet_conn.go | Wraps packet connections with datacap tracking, periodic reporting, and throttling capabilities |
| datacap/inbound_wrapper.go | Manages datacap tracking for inbound connections with device ID extraction |
| datacap/inbound.go | Implements datacap inbound adapter that wraps connections and routes them with tracking enabled |
| datacap/extractor.go | Extracts device information from HTTP headers for tracking purposes |
| datacap/conn.go | Wraps net.Conn with datacap tracking, periodic reporting, and throttling capabilities |
| datacap/client_test.go | Provides comprehensive test coverage for the datacap client HTTP operations |
| datacap/client.go | Implements HTTP client for communicating with datacap sidecar service |
| constant/proxy.go | Adds datacap type constant to the proxy type registry |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
datacap/inbound.go
Outdated
| httpTimeout = timeout | ||
| } | ||
|
|
||
| statusCheckInterval := 30 * time.Second |
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default status check interval is inconsistent. In inbound.go line 65, the default is 30 seconds, but in option/datacap.go line 45 the comment states "Default: '60s'". This inconsistency could confuse users. Consider standardizing to one value.
| statusCheckInterval := 30 * time.Second | |
| statusCheckInterval := 60 * time.Second |
| e.logger.Debug("failed to peek connection data: ", err) | ||
| return "", "", "", &peekConn{conn, reader} | ||
| } | ||
|
|
Copilot
AI
Nov 18, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable peekBytes is read but never explicitly checked for length or validity. If Peek returns fewer bytes than requested (e.g., due to EOF or small buffer), http.ReadRequest might fail or parse incorrectly. Consider validating the length of peekBytes before attempting to parse the HTTP request.
| // Check if we have a complete HTTP header (look for "\r\n\r\n") | |
| if !strings.Contains(string(peekBytes), "\r\n\r\n") { | |
| e.logger.Debug("incomplete HTTP headers in peeked data") | |
| return "", "", "", &peekConn{conn, reader} | |
| } |
garmr-ulfr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are InboundWrapper and Wrapper for?
It's looking pretty good! A big issue, though, is that, currently, we don't add the device ID (or anything, really) to the requests -- and how to include it didn't even occur to me. Since every protocol handles thing differently, I'm not sure what's the best way to go about that..
I would suggest holding off on this until we figure out how to include the device ID to requests.
During initial development, I was exploring different implementation approaches and created three files. These experimental files |
|
One thing to keep in mind here is that these servers are intended to be run by anyone, not just us, so we should make sure things can still run without any issue in the case. |
|
But yes the bigger issue is that the proxies don't know the device ID of the client. |
Uh oh!
There was an error while loading. Please reload this page.