Added GetParam for Network Statistics #5119

Johan511 · 2025-05-23T16:27:08Z

Description

Currently Network Statistics can not be queried, it is only indicated by QUIC_CONNECTION_EVENT.

Should I user be forced to subscribe to all the network statistics update events? No
Also, we do not signal the event on every update

This PR proposes a method where the application can query for Network Statistics when it wants to using GetParam

Testing

Added tests similar to QuicTest_QUIC_PARAM_CONN_STATISTICS*

Documentation

Added documentation for the newly introduced QUIC_PARAM_CONN_NETWORK_STATISTICS

nibanks · 2025-05-23T20:38:10Z

src/inc/msquic.h

-           uint32_t CongestionWindow;           // Congestion Window
-           uint64_t Bandwidth;                  // Estimated bandwidth
-        } NETWORK_STATISTICS;
+        NETWORK_STATISTICS_INTERNAL NETWORK_STATISTICS;


I'm not a big fan of doing it this way. Since this is the preview features define, we can still change it. So maybe we just have a NETWORK_STATISTICS as the payload of this local struct. I don't like all the extra stuff you had to put in the NETWORK_STATISTICS_INTERNAL. It's too messy.

Made the necessary change

Johan511 · 2025-05-24T08:41:46Z

Requesting review on few things in particular

All members of NETWORK_STATISTICS are now uint64_t, previously BytesInFlight and CongestionWindow were uint32_t. Why make the change? Congestion window has no bound of UINT32_MAX and would like to see all references to it changed to uint64_t in the future. The size of the struct still remains the same on most platforms (because padding), we weren't really saving space in the first place. This change has been undone, only because I do not want to touch the logging, rs.ffi and other components which already use NETWORK_STATISTICS::[BytesInFlight, CongestionWindow] to be uint32_t
This weirdness in naming scheme where type and the member are named the same. Please suggest an alternative name for the type if this is an issue.
Please have a thorough look at QuicConnGetNetworkStatistics(...), if in the future there are NETWORK_STATISTICS members which are supported by only one of Bbr or Cubic, we have no way to signal the user in the API (same issue exists also for QUIC_CONNECTION_EVENT_NETWORK_STATISTICS).

codecov · 2025-05-28T17:19:32Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.97%. Comparing base (bb17196) to head (aad2813).
Report is 16 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5119      +/-   ##
==========================================
+ Coverage   85.93%   86.97%   +1.03%     
==========================================
  Files          59       59              
  Lines       18048    18086      +38     
==========================================
+ Hits        15510    15730     +220     
+ Misses       2538     2356     -182

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

nibanks

Looks pretty good. Just a few minor comments.

src/inc/msquic.h

src/core/connection.c

nibanks · 2025-06-04T02:40:06Z

src/core/connection.c

+        return QUIC_STATUS_INVALID_PARAMETER;
+    }
+
+    memset(Stats, 0, MinimumStatsSize);


Please use CxPlatZeroMemory

Johan511 · 2025-06-04T07:12:22Z

Made the requested changes

youyuanwu · 2025-06-04T17:47:18Z

src/rs/lib.rs

@@ -327,6 +327,7 @@ pub const PARAM_CONN_VERSION_SETTINGS: u32 = 0x05000014;
 pub const PARAM_CONN_INITIAL_DCID_PREFIX: u32 = 0x05000015;
 pub const PARAM_CONN_STATISTICS_V2: u32 = 0x05000016;
 pub const PARAM_CONN_STATISTICS_V2_PLAT: u32 = 0x05000017;
+pub const PARAM_CONN_NETWORK_STATISTICS: u32 = 0x05000020;


No need to add here. All these param numbers should be removed here (at some point). We migrated to use ffi mod generated numbers.

youyuanwu · 2025-06-04T18:35:52Z

Please run cargo build with "--features overwrite" to update rust bindings, the current code is not updated.
After that the conversion code should use as_ref() in let ev = unsafe { value.__bindgen_anon_1.CONNECTED.as_ref() }; to access the additional layer introduced in the anonymous wrapper.

Johan511 · 2025-06-05T04:23:42Z

Hi @youyuanwu
Thank you for the help
Pardon my ignorance, I haven't worked with the rust ecosystem

I have changed all the

let ev = unsafe { value.__bindgen_anon_1.*QUIC_CONNECTION_EVENT__bindgen_ty_1 variants*
to
let ev = unsafe { value.__bindgen_anon_1.*QUIC_CONNECTION_EVENT__bindgen_ty_1 variants*.as_ref()

I still get one same error (rest are resolved)

error[E0412]: cannot find type `NETWORK_STATISTICS` in this scope
    --> src/rs/ffi/linux_bindings.rs:5335:49
     |
5335 |     pub NETWORK_STATISTICS: __BindgenUnionField<NETWORK_STATISTICS>,
     |                                                 ^^^^^^^^^^^^^^^^^^ not found in this scope
     |
help: you might be missing a type parameter
     |
5305 | pub struct QUIC_CONNECTION_EVENT__bindgen_ty_1<NETWORK_STATISTICS> {
     |                                               ++++++++++++++++++++

Do I need to explicitly register/declare NETWORK_STATISTICS somewhere?

Also, for win_bindings.rs, do I need to run cargo on a VM or is there someway I can run cargo with target_os as windows?

youyuanwu · 2025-06-07T00:28:36Z

The bindings does not seem to be generated and checked in correctly.
The easy way is to comment out the conversion code that has the problem and let the github action run and compile successfully. It will post the bindings patch to the PR and you can apply it.

Usually I generate the bindings on linux and windows both locally.

Johan511 · 2025-06-07T00:56:16Z

@youyuanwu
I had a look at build.rs and saw allowlist_item not taking the pattern for the struct I wanted to expose (NETWORK_STATISTICS)

I have tried the following diff in build.rs and as you mentioned cargo build --features overwrite works fine

-        .allowlist_item("QUIC.*|BOOLEAN|BYTE|HQUIC|HRESULT")
+        .allowlist_item("QUIC.*|BOOLEAN|BYTE|HQUIC|HRESULT|NETWORK_STATISTICS")

With this change it seems I dont need as_ref() either
Do you believe this is an appropriate change?

youyuanwu · 2025-06-07T02:10:43Z

This looks good.
But in hindsight the struct should probably be named "QUIC_NETWORK_STATISTICS", to avoid name collisions.
@nibanks

Johan511 · 2025-06-08T04:10:32Z

But in hindsight the struct should probably be named "QUIC_NETWORK_STATISTICS", to avoid name collisions.

I much prefer this, making the change

nibanks · 2025-06-09T22:30:10Z

src/core/connection.c

@@ -32,6 +32,7 @@
 --*/

 #include "precomp.h"
+#include "quic_platform.h"


This shouldn't be necessary, right? I thought precomp already had this.

My bad
My clangd setup aggressively adds includes
Fixed it

Signed-off-by: HHN <harihara.sn@gmail.com>

…K_STATISTICS Signed-off-by: HHN <harihara.sn@gmail.com>

…n type (was previously anon type) Signed-off-by: HHN <harihara.sn@gmail.com>

Co-authored-by: Nick Banks <nibanks@microsoft.com>

Signed-off-by: HHN <harihara.sn@gmail.com>

Johan511 requested a review from a team as a code owner May 23, 2025 16:27

nibanks added external Proposed by non-MSFT Area: API labels May 23, 2025

nibanks reviewed May 23, 2025

View reviewed changes

Johan511 force-pushed the getparam-netstat branch 2 times, most recently from 1cde3c3 to 2ffe127 Compare May 24, 2025 08:35

Johan511 force-pushed the getparam-netstat branch from 2ffe127 to 95382db Compare May 26, 2025 11:41

Johan511 force-pushed the getparam-netstat branch from a3a137e to 2427212 Compare May 28, 2025 19:00

nibanks reviewed Jun 4, 2025

View reviewed changes

Johan511 force-pushed the getparam-netstat branch from 509f3ca to e104677 Compare June 4, 2025 07:11

youyuanwu reviewed Jun 4, 2025

View reviewed changes

youyuanwu previously approved these changes Jun 9, 2025

View reviewed changes

nibanks reviewed Jun 9, 2025

View reviewed changes

Johan511 and others added 6 commits June 10, 2025 10:48

Added GetParam for Network Statistics

0c1fdff

Signed-off-by: HHN <harihara.sn@gmail.com>

added enable preview feature guard in QuicTest_QUIC_PARAM_CONN_NETWOR…

1017ab1

…K_STATISTICS Signed-off-by: HHN <harihara.sn@gmail.com>

fixed clippy error because now decltype(NETWORK_STATISTICS) is a know…

d80c6ed

…n type (was previously anon type) Signed-off-by: HHN <harihara.sn@gmail.com>

Apply suggestions from code review

f74fa49

Co-authored-by: Nick Banks <nibanks@microsoft.com>

Fixing some rust ffi issues

c88b275

Signed-off-by: HHN <harihara.sn@gmail.com>

remove unnecesery include

aad2813

Signed-off-by: HHN <harihara.sn@gmail.com>

Johan511 dismissed youyuanwu’s stale review via aad2813 June 10, 2025 05:21

Johan511 force-pushed the getparam-netstat branch from a172c79 to aad2813 Compare June 10, 2025 05:21

nibanks approved these changes Jun 10, 2025

View reviewed changes

nibanks merged commit dd842c9 into microsoft:main Jun 10, 2025
440 checks passed

Added GetParam for Network Statistics #5119

Added GetParam for Network Statistics #5119

Uh oh!

Conversation

Johan511 commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Documentation

Uh oh!

nibanks May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Johan511 May 24, 2025

Choose a reason for hiding this comment

Uh oh!

Johan511 commented May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nibanks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nibanks Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

Johan511 commented Jun 4, 2025

Uh oh!

youyuanwu Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

youyuanwu commented Jun 4, 2025

Uh oh!

Johan511 commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youyuanwu commented Jun 7, 2025

Uh oh!

Johan511 commented Jun 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youyuanwu commented Jun 7, 2025

Uh oh!

Johan511 commented Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nibanks Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

Johan511 Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Johan511 commented May 23, 2025 •

edited

Loading

Johan511 commented May 24, 2025 •

edited

Loading

codecov bot commented May 28, 2025 •

edited

Loading

Johan511 commented Jun 5, 2025 •

edited

Loading

Johan511 commented Jun 7, 2025 •

edited

Loading

Johan511 commented Jun 8, 2025 •

edited

Loading