feat(qlog): log version_information on server

[mxinden]

This commit adds support for the qlog version_information QUIC event on the server.

See #1505 for the client side.

Depends on qlog patch release with cloudflare/quiche#1684 ✔️

Meta issue: #528

Fixes #1549

[mxinden]

Following up on Martin's comment in #1505 (review):

For the server, look in server.rs for fn accept_connection(...).

Done in this pull request. Let me know if the direction is correct.

There shouldn't be a negotiated equivalent on the server

Do I understand correctly that you don't want to log the happy path, i.e. the case where version negotiation succeeds on the server? Fine by me.

For VN, you can make a call in fn process_input(...) right where it calls PacketBuilder::version_negotiation(...), which is where the Version Negotiation packet is sent.

Done in this pull request. Let me know if the direction is correct.

[mxinden]

Note that remote_address is not used by self.create_qlog_trace. Want me to change the signature of self.create_qlog_trace to only take the odcid?

[martinthomson]

That's worth doing. We might have needed an address once upon a time. Or maybe all the other instances where a trace was needed involved an address. Now that we have a case where we make a useless AttemptKey, it seems worthwhile changing the signature to eliminate that extra work (and have the other call sites use AttemptKey::odcid().

[mxinden]

👍 Continuing discussion in #1531 (review).

[codecov-commenter]

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (abf2636) 86.52% compared to head (1617f56) 86.49%.
Report is 4 commits behind head on main.

Files	Patch %	Lines
neqo-transport/src/server.rs	65.62%	11 Missing ⚠️
neqo-transport/src/qlog.rs	44.44%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1531      +/-   ##
==========================================
- Coverage   86.52%   86.49%   -0.03%     
==========================================
  Files         117      117              
  Lines       38170    38264      +94     
==========================================
+ Hits        33025    33096      +71     
- Misses       5145     5168      +23     

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

[mxinden]

@martinthomson can you give this pull request a first review? Is this the direction you intended in #1505 (review)?

[martinthomson]

Once you get the refactoring sorted, I'll merge this. Sorry for missing it (I blame my vacation).

[martinthomson]

That's worth doing. We might have needed an address once upon a time. Or maybe all the other instances where a trace was needed involved an address. Now that we have a case where we make a useless AttemptKey, it seems worthwhile changing the signature to eliminate that extra work (and have the other call sites use AttemptKey::odcid().

[mxinden]

Thanks for the review. Mind taking another look @martinthomson?

[mxinden]

PublicPacket::dcid returns a &ConnectionIdRef<'a>:

neqo/neqo-transport/src/packet/mod.rs

Lines 676 to 678 in 9e6cd93

	pub fn dcid(&self) -> &ConnectionIdRef<'a> {
	&self.dcid
	}

Thus I opted for create_qlog_trace to take a &ConnectionIdRef instead of a &ConnectionId since the former can be derived from the latter for free.

Let me know if that is fine by you, or whether you consider this a premature optimization not worth the noise.

(I am a bit confused why PublicPacket::dcid returns a reference to a reference (i.e. & and Ref<'_>) to some data. But that might be out of scope for this pull request.)

[martinthomson]

Oh, the ref to a ref is probably just to avoid putting Copy on that struct. Which is silly because it is just a reference itself.