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.

Sign up for GitHub

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

Adds more fields to connection stats #368

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Adds more fields to connection stats #368

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc11e5d
add more fields
janaiyengar Jul 4, 2020
2c12c8d
kazuho comments
janaiyengar Jul 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 59 additions & 1 deletion include/quicly.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,30 @@ struct st_quicly_conn_streamgroup_state_t {
* Total number of packets for which acknowledgements were received after being marked lost. \
*/ \
uint64_t late_acked; \
/** \
* Total number of ack-only packets received. \
*/ \
uint64_t ack_only_received; \
/** \
* Total number of ack-only packets sent. (TODO) \
*/ \
uint64_t ack_only_sent; \
/** \
* Total number of packets received during the handshake. \
*/ \
uint64_t handshake_received; \
/** \
* Total number of packets sent during the handshake. \
*/ \
uint64_t handshake_sent; \
/** \
* Total number of 0-RTT packets received. \
*/ \
uint64_t zerortt_received; \
/** \
* Total number of 0-RTT packets sent. \
*/ \
uint64_t zerortt_sent; \
} num_packets; \
struct { \
/** \
Expand All @@ -367,9 +391,17 @@ struct st_quicly_conn_streamgroup_state_t {
* Total bytes sent, at UDP datagram-level. \
*/ \
uint64_t sent; \
/** \
* Total stream bytes received. \
*/ \
uint64_t stream_received; \
/** \
* Total stream bytes sent. \
*/ \
uint64_t stream_sent; \
} num_bytes; \
/** \
* Total number of PTOs during the connections. \
* Total number of PTOs during the connection. \
*/ \
uint32_t num_ptos

Expand All @@ -386,6 +418,32 @@ typedef struct st_quicly_stats_t {
* Congestion control stats (experimental; TODO cherry-pick what can be exposed as part of a stable API).
*/
quicly_cc_t cc;
/**
* Connection duration.
*/
uint64_t duration;
/**
* QUIC version.
*/
uint32_t version;
/**
* Maximum packet size used.
*/
uint16_t max_packet_size;
kazuho marked this conversation as resolved.
Show resolved Hide resolved
/**
* Largest allowed and largest opened stream of each type, for locally created streams.
*/
quicly_stream_id_t largest_allowed_local_uni_stream_id;
quicly_stream_id_t largest_open_local_uni_stream_id;
quicly_stream_id_t largest_allowed_local_bidi_stream_id;
quicly_stream_id_t largest_open_local_bidi_stream_id;
/**
* Largest allowed and largest opened stream of each type, for streams created by remote endpoint.
*/
quicly_stream_id_t largest_allowed_remote_uni_stream_id;
quicly_stream_id_t largest_open_remote_uni_stream_id;
quicly_stream_id_t largest_allowed_remote_bidi_stream_id;
quicly_stream_id_t largest_open_remote_bidi_stream_id;
kazuho marked this conversation as resolved.
Show resolved Hide resolved
} quicly_stats_t;

/**
Expand Down
30 changes: 30 additions & 0 deletions lib/quicly.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,10 @@ struct st_quicly_conn_t {
} active_acked_cache;
} on_ack_stream;
} stash;
/**
* Start time of the connection.
*/
int64_t conn_start_at;
Copy link
Member

Choose a reason for hiding this comment

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

"conn_" seems excessive to me, as this field is part of quicly_conn_t. Maybe rename to created_at or something?

};

struct st_quicly_handle_payload_state_t {
Expand Down Expand Up @@ -1097,6 +1101,18 @@ int quicly_get_stats(quicly_conn_t *conn, quicly_stats_t *stats)
/* set or generate the non-pre-built stats fields here */
stats->rtt = conn->egress.loss.rtt;
stats->cc = conn->egress.cc;
stats->duration = conn->stash.now - conn->conn_start_at;
stats->version = conn->super.version;
stats->max_packet_size = conn->egress.max_udp_payload_size;

stats->largest_allowed_local_uni_stream_id = conn->egress.max_streams.uni.count * 4 + 2 + !quicly_is_client(conn);
stats->largest_open_local_uni_stream_id = (conn->super.local.uni.next_stream_id < 4) ? conn->super.local.uni.next_stream_id : conn->super.local.uni.next_stream_id - 4;
stats->largest_allowed_local_bidi_stream_id = conn->egress.max_streams.bidi.count * 4 + !quicly_is_client(conn);
stats->largest_open_local_bidi_stream_id = (conn->super.local.bidi.next_stream_id < 4) ? conn->super.local.bidi.next_stream_id : conn->super.local.bidi.next_stream_id - 4;
stats->largest_allowed_remote_uni_stream_id = conn->ingress.max_streams.uni.max_committed * 4 + 2 + quicly_is_client(conn);
stats->largest_open_remote_uni_stream_id = (conn->super.remote.uni.next_stream_id < 4) ? conn->super.remote.uni.next_stream_id : conn->super.remote.uni.next_stream_id - 4;
stats->largest_allowed_remote_bidi_stream_id = conn->ingress.max_streams.bidi.max_committed * 4 + quicly_is_client(conn);;
stats->largest_open_remote_bidi_stream_id = (conn->super.remote.bidi.next_stream_id < 4) ? conn->super.remote.bidi.next_stream_id : conn->super.remote.bidi.next_stream_id - 4;
return 0;
}

Expand Down Expand Up @@ -1253,6 +1269,13 @@ static int record_receipt(quicly_conn_t *conn, struct st_quicly_pn_space_t *spac
conn->egress.send_ack_at = conn->stash.now + QUICLY_DELAYED_ACK_TIMEOUT;
}

if (is_ack_only)
++conn->super.stats.num_packets.ack_only_received;
Copy link
Member

Choose a reason for hiding this comment

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

This seems fine.

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;
Copy link
Member

Choose a reason for hiding this comment

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

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.


ret = 0;
Exit:
return ret;
Expand Down Expand Up @@ -1865,6 +1888,7 @@ static quicly_conn_t *create_connection(quicly_context_t *ctx, const char *serve
memset(conn, 0, sizeof(*conn));
conn->super.ctx = ctx;
lock_now(conn, 0);
conn->conn_start_at = conn->stash.now;
set_address(&conn->super.local.address, local_addr);
set_address(&conn->super.remote.address, remote_addr);
quicly_local_cid_init_set(&conn->super.local.cid_set, ctx->cid_encryptor, local_cid);
Expand Down Expand Up @@ -2781,6 +2805,10 @@ static int commit_send_packet(quicly_conn_t *conn, quicly_send_context_t *s, enu

++conn->egress.packet_number;
++conn->super.stats.num_packets.sent;
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;
Copy link
Member

Choose a reason for hiding this comment

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

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.


if (mode != QUICLY_COMMIT_SEND_PACKET_MODE_COALESCED) {
conn->super.stats.num_bytes.sent += datagram_size;
Expand Down Expand Up @@ -3198,6 +3226,7 @@ int quicly_send_stream(quicly_stream_t *stream, quicly_send_context_t *s)
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;
Copy link
Member

Choose a reason for hiding this comment

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

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.

/* update sendstate (and also MAX_DATA counter) */
if (stream->sendstate.size_inflight < end_off) {
if (stream->stream_id >= 0)
Expand Down Expand Up @@ -4278,6 +4307,7 @@ static int handle_stream_frame(quicly_conn_t *conn, struct st_quicly_handle_payl
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;
Copy link
Member

Choose a reason for hiding this comment

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

Here also.

if ((ret = get_stream_or_open_if_new(conn, frame.stream_id, &stream)) != 0 || stream == NULL)
return ret;
return apply_stream_frame(stream, &frame);
Expand Down
24 changes: 21 additions & 3 deletions misc/probe2trace.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,20 +121,38 @@
push @fmt, map {qq("rtt_$_":\%u)} qw(minimum smoothed latest);
push @fmt, map {qq("cc_$_":\%u)} qw(type cwnd ssthresh cwnd_initial cwnd_exiting_slow_start cwnd_minimum cwnd_maximum num_loss_episodes);
push @fmt, map {qq("num_packets_$_":\%llu)} qw(sent ack_received lost lost_time_threshold late_acked received decryption_failed);
push @fmt, map {qq("num_bytes_$_":\%llu)} qw(sent received);
push @fmt, map {qq("num_packets_$_":\%llu)} qw(ack_only_received ack_only_sent handshake_received handshake_sent zerortt_received zerortt_sent);
push @fmt, map {qq("num_bytes_$_":\%llu)} qw(sent received stream_sent stream_received);
push @fmt, qq("num_ptos":\%u);
push @fmt, qq("duration":\%llu);
push @fmt, qq("version":\%u);
push @fmt, qq("max_packet_size":\%u);
push @fmt, map {qq("largest_allowed_$_":\%llu)} qw(local_uni_stream_id local_bidi_stream_id remote_uni_stream_id remote_bidi_stream_id);
push @fmt, map {qq("largest_open_$_":\%llu)} qw(local_uni_stream_id local_bidi_stream_id remote_uni_stream_id remote_bidi_stream_id);
if ($arch eq 'linux') {
push @ap, map{"((struct st_quicly_stats_t *)arg$i)->rtt.$_"} qw(minimum smoothed variance);
push @ap, map{"((struct st_quicly_stats_t *)arg$i)->cc.$_"} qw(type cwnd ssthresh cwnd_initial cwnd_exiting_slow_start cwnd_minimum cwnd_maximum num_loss_episodes);
push @ap, map{"((struct st_quicly_stats_t *)arg$i)->num_packets.$_"} qw(sent ack_received lost lost_time_threshold late_acked received decryption_failed);
push @ap, map{"((struct st_quicly_stats_t *)arg$i)->num_bytes.$_"} qw(sent received);
push @ap, map{"((struct st_quicly_stats_t *)arg$i)->num_packets.$_"} qw(ack_only_received ack_only_sent handshake_received handshake_sent zerortt_received zerortt_sent);
push @ap, map{"((struct st_quicly_stats_t *)arg$i)->num_bytes.$_"} qw(sent received stream_sent stream_received);
push @ap, "((struct st_quicly_stats_t *)arg$i)->num_ptos";
push @ap, "((struct st_quicly_stats_t *)arg$i)->duration";
push @ap, "((struct st_quicly_stats_t *)arg$i)->version";
push @ap, "((struct st_quicly_stats_t *)arg$i)->max_packet_size";
push @ap, map{"((struct st_quicly_stats_t *)arg$i)->largest_allowed_$_"} qw(local_uni_stream_id local_bidi_stream_id remote_uni_stream_id remote_bidi_stream_id);
push @ap, map{"((struct st_quicly_stats_t *)arg$i)->largest_open_$_"} qw(local_uni_stream_id local_bidi_stream_id remote_uni_stream_id remote_bidi_stream_id);
} else {
push @ap, map{"arg${i}->rtt.$_"} qw(minimum smoothed variance);
push @ap, map{"arg${i}->cc.$_"} qw(type cwnd ssthresh cwnd_initial cwnd_exiting_slow_start cwnd_minimum cwnd_maximum num_loss_episodes);
push @ap, map{"(unsigned long long)arg${i}->num_packets.$_"} qw(sent ack_received lost lost_time_threshold late_acked received decryption_failed);
push @ap, map{"(unsigned long long)arg${i}->num_bytes.$_"} qw(sent received);
push @ap, map{"(unsigned long long)arg${i}->num_packets.$_"} qw(ack_only_received ack_only_sent handshake_received handshake_sent zerortt_received zerortt_sent);
push @ap, map{"(unsigned long long)arg${i}->num_bytes.$_"} qw(sent received stream_sent stream_received);
push @ap, "arg${i}->num_ptos";
push @ap, "(unsigned long long)arg${i}->duration";
push @ap, "arg${i}->version";
push @ap, "arg${i}->max_packet_size";
push @ap, map{"(unsigned long long)arg${i}->largest_allowed_$_"} qw(local_uni_stream_id local_bidi_stream_id remote_uni_stream_id remote_bidi_stream_id);
push @ap, map{"(unsigned long long)arg${i}->largest_open_$_"} qw(local_uni_stream_id local_bidi_stream_id remote_uni_stream_id remote_bidi_stream_id);
}
} else {
$name = 'time'
Expand Down