-
Notifications
You must be signed in to change notification settings - Fork 34
[Feature] Statistics collection v2 #4028
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: develop
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.
I have an upcoming change with a new v3 configuration in review at the moment (https://github.com/nymtech/nym-vpn-client/pull/4015/files). This includes a restructure of the service config into multiple modules. Can you please wait until that is merged before merging this change?
|
Monorepo PR was merged, Cargo.lock updated. |
trojanfoe
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.
Blocking PR has now been merged. Please rebase on develop.
92b224e to
526eb4b
Compare
526eb4b to
ca06423
Compare
pronebird
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.
Reviewable status: 0 of 47 files reviewed, 2 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @neacsu, @rokas-ambrazevicius, @simonwicky, and @trojanfoe)
nym-vpn-core/crates/nym-vpnd/src/service/config/v3.rs line 72 at r2 (raw file):
residential_exit: value.residential_exit, custom_dns, network_stats: value.network_stats,
Don't you need to convert it from persistent representation? cc @trojanfoe
pronebird
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.
Reviewable status: 0 of 47 files reviewed, 3 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @neacsu, @rokas-ambrazevicius, @simonwicky, and @trojanfoe)
nym-vpn-core/crates/nym-vpnd/src/service/config/v3.rs line 31 at r2 (raw file):
residential_exit: bool, custom_dns: Option<Vec<String>>, network_stats: NetworkStatisticsConfig,
This struct is not versioned
pronebird
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.
Reviewable status: 0 of 47 files reviewed, 4 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @neacsu, @rokas-ambrazevicius, @simonwicky, and @trojanfoe)
nym-vpn-core/crates/nym-vpn-lib-types/src/network_stats.rs line 32 at r2 (raw file):
// - If the tunnel is connected in wireguard mode, it will go through wireguard // We thus need this option to allow users to report even if they can't connect (i.e. if they're being censored) /// Allow statistics sending, even if not conncetced to the mixnet/vpn
typo in connected
pronebird
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.
@pronebird reviewed 5 of 40 files at r1, 9 of 26 files at r2, all commit messages.
Reviewable status: 14 of 47 files reviewed, 3 unresolved discussions (waiting on @agentpietrucha, @doums, @mrkiwi, @neacsu, @rokas-ambrazevicius, @simonwicky, and @trojanfoe)
Ticket
JIRA-NET-422
Description
This PR improves on the minimalistic statistics collection that already exists :
This PR requires Statistics API v2 nym#6227 to be merged in the core first, and a Cargo.lock update then
Side note
App toggle and tooltip might need to be updated. iOS still needs to have disabled stats
Checklist:
Screenshots (optional, if UI related)
This change is