Skip to content
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

chore(qlog): Add meaningful qlog titles and descriptions. #2281

Merged
merged 4 commits into from
Dec 12, 2024

Conversation

mansf-osk
Copy link
Contributor

Hey Hey! 👋

First time neqo contributor here!
I'm Oskar, working student on the necko team, for those that don't already know me 😄

@mxinden was showing me qvis the other day and we noticed that description and title were just showing something the likes of Example qlog [...]. With this change they now show Neqo [client|server] qlog.

I also added a server- prefix to the filename of the server qlogs as I found it hard to understand which was which at first glance, but happy to take that out again if it's not wanted.

I hope these small change are somewhat useful and would appreciate a review and maybe a pointer as to what I could work on next. I'm definitely very interested to work more on neqo! 🙂

Cheers
Oskar

Copy link
Collaborator

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Apart from the one suggestion, this looks good to me.

Let's give others a bit of time to review as well before merging.

neqo-bin/src/client/mod.rs Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Dec 12, 2024

Failed Interop Tests

QUIC Interop Runner, client vs. server, differences relative to fd42bed.

neqo-latest as client

neqo-latest as server

All results

Succeeded Interop Tests

QUIC Interop Runner, client vs. server

neqo-latest as client

neqo-latest as server

Unsupported Interop Tests

QUIC Interop Runner, client vs. server

neqo-latest as client

neqo-latest as server

@mxinden
Copy link
Collaborator

mxinden commented Dec 12, 2024

@mansf-osk unit tests are failing. It should be as simple as updating the constant below:

pub const EXPECTED_LOG_HEADER: &str = concat!(
"\u{1e}",
r#"{"qlog_version":"0.3","qlog_format":"JSON-SEQ","trace":{"vantage_point":{"name":"neqo-Client","type":"client"},"title":"neqo-Client trace","description":"Example qlog trace description","configuration":{"time_offset":0.0},"common_fields":{"reference_time":0.0,"time_format":"relative"}}}"#,
"\n"
);

You can run them locally via cargo test.

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.43%. Comparing base (fd42bed) to head (89784fc).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2281   +/-   ##
=======================================
  Coverage   93.42%   93.43%           
=======================================
  Files         113      113           
  Lines       36743    36743           
  Branches     1632     1632           
=======================================
+ Hits        34327    34329    +2     
  Misses       1689     1689           
+ Partials      727      725    -2     

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

Copy link

Benchmark results

Performance differences relative to fd42bed.

decode 4096 bytes, mask ff: No change in performance detected.
       time:   [11.844 µs 11.875 µs 11.913 µs]
       change: [-2.3064% -0.6455% +0.8317%] (p = 0.47 > 0.05)

Found 11 outliers among 100 measurements (11.00%)
2 (2.00%) low mild
9 (9.00%) high severe

decode 1048576 bytes, mask ff: No change in performance detected.
       time:   [3.1110 ms 3.1344 ms 3.1687 ms]
       change: [-0.0578% +0.8095% +1.9023%] (p = 0.12 > 0.05)

Found 18 outliers among 100 measurements (18.00%)
18 (18.00%) high severe

decode 4096 bytes, mask 7f: No change in performance detected.
       time:   [19.804 µs 19.862 µs 19.923 µs]
       change: [-0.3626% +0.0859% +0.6318%] (p = 0.73 > 0.05)

Found 20 outliers among 100 measurements (20.00%)
4 (4.00%) low mild
1 (1.00%) high mild
15 (15.00%) high severe

decode 1048576 bytes, mask 7f: No change in performance detected.
       time:   [5.1721 ms 5.1882 ms 5.2080 ms]
       change: [-0.0793% +0.3291% +0.7621%] (p = 0.12 > 0.05)

Found 17 outliers among 100 measurements (17.00%)
17 (17.00%) high severe

decode 4096 bytes, mask 3f: No change in performance detected.
       time:   [6.9029 µs 6.9337 µs 6.9706 µs]
       change: [-0.2766% +0.1656% +0.6230%] (p = 0.49 > 0.05)

Found 14 outliers among 100 measurements (14.00%)
4 (4.00%) low mild
4 (4.00%) high mild
6 (6.00%) high severe

decode 1048576 bytes, mask 3f: No change in performance detected.
       time:   [1.7597 ms 1.7640 ms 1.7695 ms]
       change: [-0.5255% -0.0601% +0.3404%] (p = 0.85 > 0.05)

Found 6 outliers among 100 measurements (6.00%)
1 (1.00%) high mild
5 (5.00%) high severe

coalesce_acked_from_zero 1+1 entries: 💚 Performance has improved.
       time:   [99.350 ns 99.599 ns 99.866 ns]
       change: [-5.2651% -4.8418% -4.3782%] (p = 0.00 < 0.05)

Found 10 outliers among 100 measurements (10.00%)
2 (2.00%) high mild
8 (8.00%) high severe

coalesce_acked_from_zero 3+1 entries: 💚 Performance has improved.
       time:   [117.55 ns 117.94 ns 118.36 ns]
       change: [-2.9611% -2.3882% -1.8502%] (p = 0.00 < 0.05)

Found 12 outliers among 100 measurements (12.00%)
12 (12.00%) high severe

coalesce_acked_from_zero 10+1 entries: 💚 Performance has improved.
       time:   [117.31 ns 118.13 ns 119.23 ns]
       change: [-2.5171% -2.0768% -1.6167%] (p = 0.00 < 0.05)

Found 12 outliers among 100 measurements (12.00%)
2 (2.00%) low severe
2 (2.00%) low mild
8 (8.00%) high severe

coalesce_acked_from_zero 1000+1 entries: 💚 Performance has improved.
       time:   [97.472 ns 97.611 ns 97.763 ns]
       change: [-4.4914% -3.8239% -3.2083%] (p = 0.00 < 0.05)

Found 11 outliers among 100 measurements (11.00%)
4 (4.00%) high mild
7 (7.00%) high severe

RxStreamOrderer::inbound_frame(): Change within noise threshold.
       time:   [111.26 ms 111.40 ms 111.62 ms]
       change: [+0.2249% +0.3587% +0.5745%] (p = 0.00 < 0.05)

Found 21 outliers among 100 measurements (21.00%)
10 (10.00%) low mild
10 (10.00%) high mild
1 (1.00%) high severe

SentPackets::take_ranges: No change in performance detected.
       time:   [5.5429 µs 5.6911 µs 5.8490 µs]
       change: [-3.7978% -0.8247% +2.1471%] (p = 0.59 > 0.05)

Found 7 outliers among 100 measurements (7.00%)
6 (6.00%) high mild
1 (1.00%) high severe

transfer/pacing-false/varying-seeds: No change in performance detected.
       time:   [28.253 ms 28.773 ms 29.288 ms]
       change: [-0.0748% +2.7210% +5.4849%] (p = 0.06 > 0.05)
transfer/pacing-true/varying-seeds: Change within noise threshold.
       time:   [29.696 ms 30.213 ms 30.720 ms]
       change: [-5.0001% -2.7205% -0.2172%] (p = 0.03 < 0.05)
transfer/pacing-false/same-seed: No change in performance detected.
       time:   [24.270 ms 24.806 ms 25.336 ms]
       change: [-2.3270% +0.5814% +3.6418%] (p = 0.70 > 0.05)

Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) low mild

transfer/pacing-true/same-seed: No change in performance detected.
       time:   [27.766 ms 28.642 ms 29.511 ms]
       change: [-1.8316% +2.3713% +6.7333%] (p = 0.27 > 0.05)
1-conn/1-100mb-resp/mtu-1504 (aka. Download)/client: No change in performance detected.
       time:   [860.60 ms 870.08 ms 879.78 ms]
       thrpt:  [113.66 MiB/s 114.93 MiB/s 116.20 MiB/s]
change:
       time:   [-2.4346% -0.9967% +0.5069%] (p = 0.18 > 0.05)
       thrpt:  [-0.5044% +1.0067% +2.4953%]
1-conn/10_000-parallel-1b-resp/mtu-1504 (aka. RPS)/client: No change in performance detected.
       time:   [315.69 ms 318.35 ms 321.03 ms]
       thrpt:  [31.149 Kelem/s 31.412 Kelem/s 31.676 Kelem/s]
change:
       time:   [-0.3350% +0.9085% +2.0989%] (p = 0.15 > 0.05)
       thrpt:  [-2.0558% -0.9003% +0.3362%]
1-conn/1-1b-resp/mtu-1504 (aka. HPS)/client: No change in performance detected.
       time:   [33.620 ms 33.809 ms 34.011 ms]
       thrpt:  [29.403  elem/s 29.578  elem/s 29.744  elem/s]
change:
       time:   [-0.9962% -0.1077% +0.7489%] (p = 0.81 > 0.05)
       thrpt:  [-0.7434% +0.1078% +1.0062%]

Found 9 outliers among 100 measurements (9.00%)
3 (3.00%) low mild
1 (1.00%) high mild
5 (5.00%) high severe

1-conn/1-100mb-resp/mtu-1504 (aka. Upload)/client: 💔 Performance has regressed.
       time:   [1.6859 s 1.7055 s 1.7255 s]
       thrpt:  [57.954 MiB/s 58.635 MiB/s 59.317 MiB/s]
change:
       time:   [+1.6004% +3.2026% +4.9465%] (p = 0.00 < 0.05)
       thrpt:  [-4.7134% -3.1032% -1.5752%]

Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mild

Client/server transfer results

Transfer of 33554432 bytes over loopback.

Client Server CC Pacing MTU Mean [ms] Min [ms] Max [ms]
gquiche gquiche 1504 566.4 ± 113.7 503.7 821.5
neqo gquiche reno on 1504 791.1 ± 49.7 763.2 926.9
neqo gquiche reno 1504 841.0 ± 91.7 771.7 1017.9
neqo gquiche cubic on 1504 797.5 ± 48.9 769.3 928.3
neqo gquiche cubic 1504 788.1 ± 36.7 749.8 876.8
msquic msquic 1504 186.0 ± 102.3 99.2 411.5
neqo msquic reno on 1504 281.0 ± 109.9 209.5 509.7
neqo msquic reno 1504 318.2 ± 90.1 209.9 427.8
neqo msquic cubic on 1504 261.2 ± 109.9 207.3 508.6
neqo msquic cubic 1504 248.7 ± 70.9 205.3 421.1
gquiche neqo reno on 1504 745.2 ± 138.6 607.2 976.2
gquiche neqo reno 1504 736.6 ± 111.2 553.6 870.8
gquiche neqo cubic on 1504 732.9 ± 101.8 620.8 934.6
gquiche neqo cubic 1504 743.3 ± 113.2 590.0 906.2
msquic neqo reno on 1504 556.7 ± 160.3 460.8 955.2
msquic neqo reno 1504 488.3 ± 38.1 458.7 587.2
msquic neqo cubic on 1504 508.1 ± 58.2 475.3 672.2
msquic neqo cubic 1504 494.5 ± 31.3 466.3 556.8
neqo neqo reno on 1504 464.7 ± 34.7 426.1 524.9
neqo neqo reno 1504 558.8 ± 107.5 470.3 812.7
neqo neqo cubic on 1504 573.2 ± 13.5 555.1 602.1
neqo neqo cubic 1504 582.5 ± 118.2 465.0 792.3

⬇️ Download logs

@mxinden mxinden added this pull request to the merge queue Dec 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 12, 2024
@mxinden
Copy link
Collaborator

mxinden commented Dec 12, 2024

Failure on Windows installing Rust seems unrelated. Retriggered.

@larseggert larseggert added this pull request to the merge queue Dec 12, 2024
Merged via the queue into mozilla:main with commit 3001a3a Dec 12, 2024
61 of 63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants