Skip to content

Conversation

@theluckiestsoul
Copy link
Contributor

@theluckiestsoul theluckiestsoul commented Nov 18, 2025

  • Add Conn and PacketConn types to wrap net.Conn and network.PacketConn respectively, tracking data consumption and enabling throttling.
  • Introduce DeviceIDExtractor for extracting device information from HTTP headers and metadata.
  • Create Inbound to manage datacap tracking for inbound connections.
  • Add Throttler for managing bandwidth throttling using a token bucket algorithm.
  • Define DataCapInboundOptions for configuring datacap-wrapped inbound options.

related to getlantern/engineering/issues/2766

Copy link
Contributor

Copilot AI left a 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 (Conn and PacketConn) 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.

Copy link
Collaborator

@garmr-ulfr garmr-ulfr left a 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.

@theluckiestsoul
Copy link
Contributor Author

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 inbound_wrapper.go and wrapper.go were supposed to be discarded during development, but I wrongly committed them to the PR.

@myleshorton
Copy link
Contributor

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.

@myleshorton
Copy link
Contributor

But yes the bigger issue is that the proxies don't know the device ID of the client.

@garmr-ulfr
Copy link
Collaborator

@theluckiestsoul I figured out a way to send the device ID with connections. We'll have to take a slightly different route with this server-side. The only thing different is we won't need to make an inbound wrapper, however, everything else here is good. If you don't mind, I'll create a PR to merge into yours for extracting the device ID.

@theluckiestsoul
Copy link
Contributor Author

@theluckiestsoul I figured out a way to send the device ID with connections. We'll have to take a slightly different route with this server-side. The only thing different is we won't need to make an inbound wrapper, however, everything else here is good. If you don't mind, I'll create a PR to merge into yours for extracting the device ID.

@garmr-ulfr Absolutely. Please feel free to move ahead whenever you see fit. We will need to include the device ID, country code, and platform. Thanks for guiding this.

@garmr-ulfr
Copy link
Collaborator

@theluckiestsoul here's the PR that will allow the client information to be retrieved. Once that's merged, we'll be good finish up this.

@theluckiestsoul
Copy link
Contributor Author

@garmr-ulfr do we need to integrate the ClientContextTracker into radinace?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 20 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@garmr-ulfr
Copy link
Collaborator

do we need to integrate the ClientContextTracker into radinace?

Yes, we will. But, then it will only be able to connect to servers that have the context tracker, and vise-versa — which is a whole other issue..

Copy link
Collaborator

@garmr-ulfr garmr-ulfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you address Copilot's comments? If they're wrong or it's not obvious why we're ignoring it, would you add a short comment as to why? It really doesn't need to be much at all. This one, for example, if throttling after reading is intentional, then This is intentional would suffice.

@garmr-ulfr
Copy link
Collaborator

@theluckiestsoul sorry, found an issue with the context tracker that needed to be addressed. The client information was being stored globally and was overwritten on each new connection. Once that's merged, we only need to change 4 lines and then we can merge this! It looks great!

@theluckiestsoul theluckiestsoul merged commit ffdd8a0 into main Dec 19, 2025
1 check passed
@theluckiestsoul theluckiestsoul deleted the add-datacap branch December 19, 2025 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants