A80: gRPC Metrics for TCP connection #428

nanahpang · 2024-04-22T18:15:24Z

Add the proposal of gRPC TCP per-connection metrics.

yousukseung

LGTM to me as discussed offline, I'll leave others to approve.

ctiller · 2024-04-23T17:24:01Z

A80-grpc-metrics-for-tcp-connection

+
+| Name        | Disposition | Description |
+| ----------- | ----------- | ----------- |
+| grpc.tcp.remote_peer_address | optional | Store the peer address info in the format as `ip:port`. |


@markdroth @yashykt I feel like there's an equivalent label already in use?

This could be a relatively expensive fan-out for a busy server

Agreed, we discussed about this offline. Is there a way to support an opaque user-defined peer info string? In prod, by default we only record the cell information due to the fan-out concern.

We need to figure out the right format here

I propose URI form, such as ipv4:1.2.3.4:567. @ejona86 @dfawley, would that be a problem for Java and Go if we ever added these metrics in your languages?

ctiller · 2024-04-23T17:24:54Z

A80-grpc-metrics-for-tcp-connection

+
+| Name          | Type  | Unit  | Labels  | Description |
+| ------------- | ----- | ----- | ------- | ----------- |
+| grpc.tcp.min_rtt | Distribution | s | grpc.tcp.remote_peer_string | Reports TCP's current estimate of minimum round trip time (RTT), typically used as an indication of the network health between two endpoints. |


@yashykt how well does seconds here align with other metrics we have?

We are following OpenTelemetry's conventions mostly which advocates for seconds 's' as the unit for duration

ctiller · 2024-04-23T17:25:42Z

A80-grpc-metrics-for-tcp-connection

+| grpc.tcp.delivery_rate | Distribution | bit/s | grpc.tcp.remote_peer_string | Records the most recent non-app-limited throughput at the time that Fathom samples the connection statistics. |
+| grpc.tcp.packets_sent | Counter | {packet} | grpc.tcp.remote_peer_string | Records total packets TCP sends in the calculation period. |
+| grpc.tcp.packets_retransmitted | Counter | {packet} | grpc.tcp.remote_peer_string | Records total packets lost in the calculation period, including lost or spuriously retransmitted packets. |
+| grpc.tcp.packets_spurious_retransmitted | Counter | {packet} | grpc.tcp.remote_peer_string | Records total packets spuriously retransmitted packets in the calculation period. |


I'm not sure what spurious means in this context... maybe we could link to some write-up, or include some more text defining these things?

I could not find an authoritative source we can link, but I think we can include what's on go/fathom-metrics i.e. "These are retransmissions that TCP later discovered unnecessary.". The definition is public.

Sounds good, I will add that in the description.

atollena · 2024-04-24T08:22:07Z

A80-grpc-metrics-for-tcp-connection

+
+| Name          | Type  | Unit  | Labels  | Description |
+| ------------- | ----- | ----- | ------- | ----------- |
+| grpc.tcp.min_rtt | Distribution | s | grpc.tcp.remote_peer_string | Reports TCP's current estimate of minimum round trip time (RTT), typically used as an indication of the network health between two endpoints. |


A couple of remarks on this:

Do you know how portable this is going to be? It would help to explain how you plan to collect that data. Do you plan to use getsockopt + TCP_INFO that yields a min_rtt in some cases? Even on linux, I'm not sure all TCP algorithms measure the minimum RTT.

What is a Distribution type? Do you mean a histogram?

Since this is a property of the transport, this probably belongs under grpc.transport.tcp.

I think min_rtt is general enough that we can include, I presume most congestion control tracks RTT or equivalent to keep track of the estimated BDP.

And I think this gRFC is supposed to cover the definition of the metric and not about the collection method. @yashykt Yes, in Linux we can get it via either getsockopt(TCP_INFO) or TCP_NLA_MIN_RTT from cmsg when timestamping is enabled.

I think min_rtt is general enough that we can include, I presume most congestion control tracks RTT or equivalent to keep track of the estimated BDP.

OK, and even if it weren't the case, there is probably no problem with omitting it.

And I think this gRFC is supposed to cover the definition of the metric and not about the collection method.

OK, yes. My point is more that the definition needs to be precise enough so that different implementations measure the same thing.

I think if it's important for the correctness of the metrics, it makes sense to give more information on this metric. Maybe you could say that a valid implementation of this is through getsockopt?

I added a summary of how the metrics are collected and linked some references. Let me know if anything is unclear. Thanks.

atollena · 2024-04-24T08:24:38Z

A80-grpc-metrics-for-tcp-connection

+| Name          | Type  | Unit  | Labels  | Description |
+| ------------- | ----- | ----- | ------- | ----------- |
+| grpc.tcp.min_rtt | Distribution | s | grpc.tcp.remote_peer_string | Reports TCP's current estimate of minimum round trip time (RTT), typically used as an indication of the network health between two endpoints. |
+| grpc.tcp.delivery_rate | Distribution | bit/s | grpc.tcp.remote_peer_string | Records the most recent non-app-limited throughput at the time that Fathom samples the connection statistics. |


What is Fathom? A quick internet search yields https://dl.acm.org/doi/pdf/10.1145/3603269.3604815 which sounds related (but I have not read it)... Do you think anything that uses it is going to useful to anyone outside Google?

No, and app-limitedness is also not a general property either. I think we should update to something like "Latest throughput measured of the TCP connection."

Sounds good, updated.

A80-grpc-metrics-for-tcp-connection

yashykt · 2024-04-27T00:27:12Z

A80-grpc-metrics-for-tcp-connection

+
+| Name        | Disposition | Description |
+| ----------- | ----------- | ----------- |
+| grpc.tcp.remote_peer_address | optional | Store the peer address info in the format as `ip:port`. |


yashykt · 2024-04-27T00:27:40Z

A80-grpc-metrics-for-tcp-connection

+
+| Name          | Type  | Unit  | Labels  | Description |
+| ------------- | ----- | ----- | ------- | ----------- |
+| grpc.tcp.min_rtt | Distribution | s | grpc.tcp.remote_peer_string | Records TCP's current estimate of minimum round trip time (RTT), typically used as an indication of the network health between two endpoints. |


I think maybe we should add the remote_string and maybe the local_string as optional labels on these metrics

Sounds good, updated.

yashykt · 2024-04-27T00:28:47Z

A80-grpc-metrics-for-tcp-connection

+
+| Name          | Type  | Unit  | Labels  | Description |
+| ------------- | ----- | ----- | ------- | ----------- |
+| grpc.tcp.min_rtt | Distribution | s | grpc.tcp.remote_peer_string | Reports TCP's current estimate of minimum round trip time (RTT), typically used as an indication of the network health between two endpoints. |


We are following OpenTelemetry's conventions mostly which advocates for seconds 's' as the unit for duration

yashykt · 2024-04-27T00:30:45Z

A80-grpc-metrics-for-tcp-connection

+
+| Name          | Type  | Unit  | Labels  | Description |
+| ------------- | ----- | ----- | ------- | ----------- |
+| grpc.tcp.min_rtt | Distribution | s | grpc.tcp.remote_peer_string | Reports TCP's current estimate of minimum round trip time (RTT), typically used as an indication of the network health between two endpoints. |


I think if it's important for the correctness of the metrics, it makes sense to give more information on this metric. Maybe you could say that a valid implementation of this is through getsockopt?

yashykt

(approved by mistake)

markdroth · 2024-04-29T17:11:18Z

A80-grpc-metrics-for-tcp-connection

+
+| Name        | Disposition | Description |
+| ----------- | ----------- | ----------- |
+| grpc.tcp.remote_peer_address | optional | Store the peer address info in the format as `ip:port`. |


I propose URI form, such as ipv4:1.2.3.4:567. @ejona86 @dfawley, would that be a problem for Java and Go if we ever added these metrics in your languages?

A80-grpc-metrics-for-tcp-connection

…rics-for-tcp-connection.md

markdroth

Need review from @ejona86 and @dfawley to make sure these definitions are still okay in terms of potential future implementation in other languages.

markdroth · 2024-05-10T19:48:37Z

A80-grpc-metrics-for-tcp-connection.md

+To improve the network debugging capabilities for gRPC users, we propose adding per-connection TCP metrics in gRPC. The metrics will utilize the metrics framework outlined in  [A79].
+
+### Related Proposals: 
+* [A79]: gRPC Non-Per-Call Metrics Framework (pending)


This gRFC has been merged, so please remove the "(pending)" and change the lin to point to A79-non-per-call-metrics-architecture.md.

Sure, updated.

markdroth · 2024-05-10T19:54:24Z

A80-grpc-metrics-for-tcp-connection.md

+| ------------- | ----- | ----- | ------- | ----------- |
+| grpc.tcp.min_rtt | Histogram (double) | s | grpc.tcp.peer_address, grpc.tcp.local_address | Records TCP's current estimate of minimum round trip time (RTT), typically used as an indication of the network health between two endpoints.  |
+| grpc.tcp.delivery_rate | Histogram (double) | bit/s | grpc.tcp.peer_address, grpc.tcp.local_address | Records latest throughput measured of the TCP connection. |
+| grpc.tcp.packets_sent | Counter (int64) | {packet} | grpc.tcp.peer_address, grpc.tcp.local_address | Records total packets TCP sends in the calculation period. |


@yashykt What types should we be defining here?

I updated the Counter type to uint64 based on the register methods in metrics.h.
@yashykt Feel free to modify if it will use other types.

Create A80-grpc-metrics-for-tcp-connection

dedfb16

nanahpang requested review from yashykt, ctiller, markdroth and yousukseung April 22, 2024 18:15

yousukseung reviewed Apr 22, 2024

View reviewed changes

ctiller reviewed Apr 23, 2024

View reviewed changes

Update A80-grpc-metrics-for-tcp-connection

ffaeb22

atollena reviewed Apr 24, 2024

View reviewed changes

nanahpang added 2 commits April 24, 2024 13:36

Update A80-grpc-metrics-for-tcp-connection

d413291

Update A80-grpc-metrics-for-tcp-connection

5b5ba3f

yashykt approved these changes Apr 27, 2024

View reviewed changes

yashykt requested changes Apr 27, 2024

View reviewed changes

markdroth reviewed Apr 29, 2024

View reviewed changes

markdroth changed the title ~~Create A80-grpc-metrics-for-tcp-connection~~ A80: gRPC Metrics for TCP connection Apr 29, 2024

markdroth reviewed Apr 29, 2024

View reviewed changes

A80-grpc-metrics-for-tcp-connection Outdated Show resolved Hide resolved

nanahpang added 5 commits April 29, 2024 14:25

Update and rename A80-grpc-metrics-for-tcp-connection to A80-grpc-met…

583e6b3

…rics-for-tcp-connection.md

Update A80-grpc-metrics-for-tcp-connection.md

8aa21c1

Update A80-grpc-metrics-for-tcp-connection.md

9f8038c

Update A80-grpc-metrics-for-tcp-connection.md

59ab138

Update A80-grpc-metrics-for-tcp-connection.md

ce27a69

nanahpang requested review from atollena, yousukseung, markdroth, ctiller and yashykt May 2, 2024 20:12

markdroth reviewed May 10, 2024

View reviewed changes

Update A80-grpc-metrics-for-tcp-connection.md

d239c39

nanahpang requested review from ejona86 and dfawley and removed request for atollena May 10, 2024 20:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A80: gRPC Metrics for TCP connection #428

A80: gRPC Metrics for TCP connection #428

nanahpang commented Apr 22, 2024 •

edited

yousukseung left a comment

ctiller Apr 23, 2024

ctiller Apr 23, 2024

yousukseung Apr 23, 2024

yashykt Apr 27, 2024

yashykt Apr 29, 2024

markdroth Apr 29, 2024

ctiller Apr 23, 2024

yashykt Apr 27, 2024

ctiller Apr 23, 2024

yousukseung Apr 23, 2024

nanahpang Apr 23, 2024

atollena Apr 24, 2024

yousukseung Apr 24, 2024

atollena Apr 25, 2024

yashykt Apr 27, 2024

nanahpang May 2, 2024 •

edited

atollena Apr 24, 2024

yousukseung Apr 24, 2024

nanahpang Apr 24, 2024

yashykt Apr 27, 2024

yashykt Apr 27, 2024 •

edited

nanahpang Apr 29, 2024 •

edited

yashykt Apr 27, 2024

yashykt Apr 27, 2024

yashykt left a comment

markdroth Apr 29, 2024

markdroth left a comment

markdroth May 10, 2024

nanahpang May 10, 2024

markdroth May 10, 2024

nanahpang May 10, 2024

A80: gRPC Metrics for TCP connection #428

Are you sure you want to change the base?

A80: gRPC Metrics for TCP connection #428

Conversation

nanahpang commented Apr 22, 2024 • edited

yousukseung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanahpang May 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashykt Apr 27, 2024 • edited

Choose a reason for hiding this comment

nanahpang Apr 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashykt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markdroth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nanahpang commented Apr 22, 2024 •

edited

nanahpang May 2, 2024 •

edited

yashykt Apr 27, 2024 •

edited

nanahpang Apr 29, 2024 •

edited