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.

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
Changes from 4 commits
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
62 changes: 62 additions & 0 deletions A80-grpc-metrics-for-tcp-connection
@@ -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: https://groups.google.com/g/grpc-io/c/AyT0LVgoqFs

## 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?

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 | Records 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

@yashykt yashykt Apr 27, 2024

Choose a reason for hiding this comment

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

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

Copy link
Author

@nanahpang nanahpang Apr 29, 2024

Choose a reason for hiding this comment

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

Sounds good, updated.

markdroth marked this conversation as resolved.
Show resolved Hide resolved
| grpc.tcp.delivery_rate | Distribution | bit/s | grpc.tcp.remote_peer_string | Records latest throughput measured of the TCP connection. |
| 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. These are retransmissions that TCP later discovered unnecessary.|

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