-
Notifications
You must be signed in to change notification settings - Fork 46
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
Overhaul stats: Refactor events #1341
Comments
Hi @da2ce7 I'm planing to start refactoring HTTP tracker events. I would like to have something like: pub enum Event {
Tcp4Announce { connection: ConnectionContext },
Tcp4Scrape { connection: ConnectionContext },
Tcp6Announce { connection: ConnectionContext },
Tcp6Scrape { connection: ConnectionContext },
}
pub struct ConnectionContext {
pub client_ip: IpAddr,
pub server_socket_addr: SocketAddr,
} I don't have the The easy solution would be to new fields (services or configuration) to the This is the current pub struct HttpTrackerCoreContainer {
// todo: replace with TrackerCoreContainer
pub core_config: Arc<Core>,
pub announce_handler: Arc<AnnounceHandler>,
pub scrape_handler: Arc<ScrapeHandler>,
pub whitelist_authorization: Arc<whitelist::authorization::WhitelistAuthorization>,
pub authentication_service: Arc<AuthenticationService>,
pub http_tracker_config: Arc<HttpTracker>,
pub http_stats_event_sender: Arc<Option<Box<dyn statistics::event::sender::Sender>>>,
pub http_stats_repository: Arc<statistics::repository::Repository>,
pub announce_service: Arc<AnnounceService>,
pub scrape_service: Arc<ScrapeService>,
} And this is how it would look after the change: pub struct HttpTrackerCoreContainer {
// todo: replace with TrackerCoreContainer
pub core_config: Arc<Core>,
pub announce_handler: Arc<AnnounceHandler>,
pub scrape_handler: Arc<ScrapeHandler>,
pub whitelist_authorization: Arc<whitelist::authorization::WhitelistAuthorization>,
pub authentication_service: Arc<AuthenticationService>,
pub http_tracker_config: Arc<HttpTracker>,
pub http_stats_event_sender: Arc<Option<Box<dyn statistics::event::sender::Sender>>>,
pub announce_service: Arc<AnnounceService>,
pub scrape_service: Arc<ScrapeService>,
pub running_http_trackers: Arc<HashMap<SocketAddr>, RunningHttpTrackerCoreContainer>,
}
pub struct RunningHttpTrackerCoreContainer {
pub server_socket_addr: SocketAddr,
pub http_stats_repository: Arc<statistics::repository::Repository>,
} However, I think we should not put that inside the pub struct AppContainer {
// ... other services
pub running_hattp_trackers: RunningHttpTrackers,
}
pub struct RunningHttpTrackers {
pub running_http_trackers: Arc<HashMap<SocketAddr>, RunningHttpTrackerCoreContainer>,
}
pub struct RunningHttpTrackerCoreContainer {
pub server_socket_addr: SocketAddr,
pub http_stats_repository: Arc<statistics::repository::Repository>,
} The main reason is I want to be able to run a single HTTP tracker with a new binary (if needed or for other projects) and the new field does not make sense. The "multiple trackers" feature is only a feature for the Torrust Tracker binary. |
I have changed my mind. The pub http_tracker_config: Arc<HttpTracker>, For the time being, I'm going to pass the server socket address from top to bottom as a new parameter without adding a "wrapper". If we need to build specific services for one tracker server running bound to a socket we need to build those services at the I will give an example. We can run 2 HTTP trackers on different ports ( In the future we want to collect independent metrics for each tracker. That means we could have
I think option 3 is the right one because:
In order to implement these changes (progressively and in parallel) I would recommend:
The final stats endpoint could be like:
And adding some others metrics suggested here. I would not joins the HTTP and UDP tackers' metrics in the same array because their metrics could have different fields in the future. cc @da2ce7 |
Sub-issues implemented. |
There are three types of statistics events (one for the HTTP tracker and another for the UDP tracker):
bittorrent_http_tracker_core::statistics::event::Event
:bittorrent_udp_tracker_core::statistics::event::Event
torrust_udp_tracker_server::statistics::event::Event
:I think the code would be simpler and the events would be more useful if we enrich them. Something like this:
bittorrent_http_tracker_core::statistics::event::Event
:bittorrent_udp_tracker_core::statistics::event::Event
:torrust-udp-tracker-server::statistics::event::Event
:That would also simplify the way we send them, from this:
To this:
These events would be more useful for external consumers. For example, adding the IP would allow external event consumers to extract more information and build their own aggregate data. This relates to #1307.
We might need to add more info like the server socket address of the UDP server that handled the request.
See #1263 (comment)
The text was updated successfully, but these errors were encountered: