Adds more fields to connection stats #368
Conversation
kazuho
left a comment
kazuho
left a comment
There was a problem hiding this comment.
Sorry for the delay. Looks mostly good, left some thoughts. PTAL.
| /** | ||
| * Start time of the connection. | ||
| */ | ||
| int64_t conn_start_at; |
There was a problem hiding this comment.
"conn_" seems excessive to me, as this field is part of quicly_conn_t. Maybe rename to created_at or something?
| if (epoch == QUICLY_EPOCH_INITIAL || epoch == QUICLY_EPOCH_HANDSHAKE) | ||
| ++conn->super.stats.num_packets.handshake_received; | ||
| if (epoch == QUICLY_EPOCH_0RTT) | ||
| ++conn->super.stats.num_packets.zerortt_received; |
There was a problem hiding this comment.
I'm not sure if this is the correct location for updating the metrics.
We update num_packets.received in quicly_accept and quicly_receive. Maybe we should change that to per-packet-type metric (i.e. uint64_t [QUICLY_NUM_EPOCHS]) and increment the counter of the corresponding epoch.
| } | ||
|
|
||
| if (is_ack_only) | ||
| ++conn->super.stats.num_packets.ack_only_received; |
| if (get_epoch(*s->target.first_byte_at) == QUICLY_EPOCH_INITIAL || get_epoch(*s->target.first_byte_at) == QUICLY_EPOCH_HANDSHAKE) | ||
| ++conn->super.stats.num_packets.handshake_sent; | ||
| if (get_epoch(*s->target.first_byte_at) == QUICLY_EPOCH_0RTT) | ||
| ++conn->super.stats.num_packets.zerortt_sent; |
There was a problem hiding this comment.
As stated above, maybe it would be better to change the type of num_packets.sent to uint64_t [QUICLY_NUM_EPOCHS] and increment the per-epoch counter.
| UpdateState: | ||
| QUICLY_PROBE(STREAM_SEND, stream->conn, stream->conn->stash.now, stream, off, end_off - off, is_fin); | ||
| QUICLY_PROBE(QUICTRACE_SEND_STREAM, stream->conn, stream->conn->stash.now, stream, off, end_off - off, is_fin); | ||
| stream->conn->super.stats.num_bytes.stream_sent += end_off - off; |
There was a problem hiding this comment.
This metric would cover both the ordinary streams and CRYPTO streams, as we do not check the sign of the stream-id. Are we fine with that? I do not have a strong opinion but would like to confirm.
| if ((ret = quicly_decode_stream_frame(state->frame_type, &state->src, state->end, &frame)) != 0) | ||
| return ret; | ||
| QUICLY_PROBE(QUICTRACE_RECV_STREAM, conn, conn->stash.now, frame.stream_id, frame.offset, frame.data.len, (int)frame.is_fin); | ||
| conn->super.stats.num_bytes.stream_received += frame.data.len; |
No description provided.