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

A80: gRPC Metrics for TCP connection #428

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

A80: gRPC Metrics for TCP connection #428

Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
dedfb16
Create A80-grpc-metrics-for-tcp-connection
nanahpang Apr 22, 2024
ffaeb22
Update A80-grpc-metrics-for-tcp-connection
nanahpang Apr 23, 2024
d413291
Update A80-grpc-metrics-for-tcp-connection
nanahpang Apr 24, 2024
5b5ba3f
Update A80-grpc-metrics-for-tcp-connection
nanahpang Apr 25, 2024
583e6b3
Update and rename A80-grpc-metrics-for-tcp-connection to A80-grpc-met…
nanahpang Apr 29, 2024
8aa21c1
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang Apr 29, 2024
9f8038c
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 1, 2024
59ab138
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 2, 2024
ce27a69
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 2, 2024
d239c39
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 10, 2024
0726f6e
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 15, 2024
3bfe76b
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 15, 2024
2ccf768
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 21, 2024
83ac908
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 22, 2024
2a11aea
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 22, 2024
b6dc6d9
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 22, 2024
0aceebe
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 22, 2024
052d5cf
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 22, 2024
7e5bc86
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 24, 2024
092fbc1
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 24, 2024
bd18940
Update A80-grpc-metrics-for-tcp-connection.md
nanahpang May 24, 2024
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
62 changes: 62 additions & 0 deletions A80-grpc-metrics-for-tcp-connection
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
A80: gRPC Metrics for TCP connection
markdroth marked this conversation as resolved.
Show resolved Hide resolved
----
* Author(s): Yash Tibrewal (@yashykt), Nana Pang (@nanahpang), Yousuk Seung (@yousukseung)
* Approver: Craig Tiller (@ctiller), Mark Roth (@markdroth)
* Status: {Draft, In Review, Ready for Implementation, Implemented}
* language: {...}
* Last updated: 2024-04-18
* Discussion at: {...}

## Abstract

This document proposes adding new TCP connection metrics to gRPC for improved network analysis and debugging.

## Background

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)

[A79]: https://github.com/grpc/proposal/pull/421

## Proposal
markdroth marked this conversation as resolved.
Show resolved Hide resolved

This document proposes changes to the following gRPC components.

#### Per-Connection TCP Metrics

We will provide the following metrics:
- `grpc.tcp.min_rtt`
- `grpc.tcp.delivery_rate`
- `grpc.tcp.packets_sent`
- `grpc.tcp.packets_retransmitted`
- `grpc.tcp.packets_spurious_retransmitted`

The metrics will have label:

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Not yet

Copy link
Member

Choose a reason for hiding this comment

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

We need to figure out the right format here

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I think this should be clarified more. For instance, is ipv4:///1.2.3.4:567 also OK? What is the format for ipv6? What other schemes do we support? @ejona86 is concerned that ipv4:1.2.3.4:567 is not a URI we should support in the first place. So maybe this should just be specified fully as strictly:

- ipv4:<ipv4-address>:<port-number>
OR
- ipv6:[<ipv6-address>]:<port-number>

markdroth marked this conversation as resolved.
Show resolved Hide resolved

The metrics will be exported as:

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of remarks on this:

  1. 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.
  2. What is a Distribution type? Do you mean a histogram?
  3. Since this is a property of the transport, this probably belongs under grpc.transport.tcp.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

@nanahpang nanahpang May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

| 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. |
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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."

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, updated.

| 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. |
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 what spurious means in this context... maybe we could link to some write-up, or include some more text defining these things?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, I will add that in the description.


### Metric Stability

All metrics added in this proposal will start as experimental. The long term goal will be to
de-experimentalize them and have them be on by default, but the exact
criteria for that change are TBD.

### Temporary environment variable protection

This proposal does not include any features enabled via external I/O, so
it does not need environment variable protection.