-
Notifications
You must be signed in to change notification settings - Fork 49
Overhaul stats: Add the PeerClient
and version as labels to metrics
#1456
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
Comments
The list of peer IDS included in BEP 20: https://www.bittorrent.org/beps/bep_0020.html The list of peer IDS from: AI model: GPT-4o 📋 BitTorrent Client Peer ID Prefixes
🔗 Sources
|
Since there are many clients, I won't use labeled metrics to count requests from these clients. That would generate a lot of time series in Prometheus. The reason for adding this label was to identify peers that are sending the wrong connection ID. I'm assuming there could be some bad implementations. I only want to confirm if that happens. Alternative SolutionI think we could change the BanService to include two new HashMaps to count:
That would help to identify if a concrete client software or version is not sending the connection ID. We could expose these counters via the
On the other hand, I increased the ban period from 1 hour to 24 hours. It seems the number of banned IPS does not have an upper limit: cc @da2ce7 |
After discussing this issue today again with @da2ce7 I've realised that it does not matter whether I used the labelled metrics or an independent data structure. The real problem is if I add a new label to the Number of metric values = NC * NV In a HashMap where the key is the client name and the value is a list of the number of bans per version of that client. Something like: type ClientMap = HashMap<String, VersionMap>;
type VersionMap = HashMap<String, Arc<AtomicUsize>>; If I create an independent metric like this:
The number of time series generated in Prometheus would be the same as if we pull the JSON. Assuming, on average, 15 for the number of different clients and 5 versions per client, we would have only 75 new entries in the metrics collections. In my original idea of adding a new label to The key point is that even if it is related to requests, we disregard the rest of the request and focus solely on the relevant information for IP bans, specifically the client software. |
…t sendable The error will be included in the UdpError event ans sent via tokio channel.
Not exposing the original complex error type becuase: - It's too complex. - It forces all errors to be "Sent", "PartialEq". - It would expose a lot of internals.
a8f3a97 refactor: [#1551] extract event handler for each udp event (Jose Celano) 89ac87c refactor: [#1551] extract methods in udp event handler" (Jose Celano) 07c6e89 refactor: rename UDP tracker server error variants (Jose Celano) 21bea5b refactor: [#1456] increase ban counters asyncronously (Jose Celano) ad1b19a feat: trigger UDP error event when there is no transaction ID too (Jose Celano) 525ab73 refactor: [#1456] extract methods (Jose Celano) f485501 refactor: [#1456 clean code (Jose Celano) 0108c26 fix: test. Error message changed (Jose Celano) d7902f1 refactor: [#1456] remove unused enum variant in udp server error (Jose Celano) 8f3c22a feat: [#1456] expose error kind in the UdpError event (Jose Celano) 52b9660 feat: [#1456] wrapper over aquatic RequestParseError to make it sendable (Jose Celano) Pull request description: I will add a new metric to count which client's software is used when a UDP error is produced (only for connection ID errors, for now). This PR make some changes before adding the new metric. ### Subtasks - [x] Wrapper for aquatic parse error. It's not sendable. - [x] Add the error to the `Event::UdpError` event. - [x] Move logic to increase the number of wrong connection IDs per IP to the event listener. - [x] Rename errors. - [x] Refactor event handler. ACKs for top commit: josecelano: ACK a8f3a97 Tree-SHA512: faa185a8050ef9e45f317ef06aa74e52bb385c31167fa0f199411bb1a47a573429daa31b2cccdd024f8fd75c91227618350d10e41d12e8b4062fb1e8d7f7bfdc
Implemented. I'm deploying it to the tracker demo. |
Uh oh!
There was an error while loading. Please reload this page.
It depends on:
Peer
to HTTP coreannounce
event #1376Peer
to UDP coreannounce
event #1384We are receiving many bad requests from BitTorrent clients in the tracker demo. For example:
connection id could not be verified
torrust-demo#14It would be invaluable to include the clients' software version as a label in the metrics so we can check what BitTorrent clients are producing those errors. If they are known clients with public repositories, we could open issues.
We should include the new labels in all metrics where the information is available.
We are using the aquatic
PeerId
which included thePeerClient
info.https://github.com/greatest-ape/aquatic/blob/master/crates/peer_id/src/lib.rs
This is the current metric where we collect UDP errors:
The new one could include these new labels:
peer_client_software_name
peer_client_software_version
NOTICE: We have to generate a key value for the name.
The text was updated successfully, but these errors were encountered: