Added GetParam for Network Statistics

[Johan511]

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]

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.

[Johan511]

Made the necessary change

[Johan511]

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]

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.

[nibanks]

Please use CxPlatZeroMemory

[Johan511]

Made the requested changes

[youyuanwu]

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]

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]

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]

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]

@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]

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

[Johan511]

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

I much prefer this, making the change

[nibanks]

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

[Johan511]

My bad
My clangd setup aggressively adds includes
Fixed it