hashicorp / hashicorp/consul

rpc: improve RPC metrics by adding more labels

Open
#10,250 0 comments 0 reactions 0 assignees View on GitHub
theme/reliability theme/telemetry type/enhancement
Dominant language
Go
Stars
30.1k
Forks
4.6k
Avg merge
1d 18h
Merged PRs (30d)
39

Description

`consul.rpc.request` (and a a few others) provide metrics for the count of RPC requests, however none of them provide enough granularity to debug most common issues. There are many RPC methods, and an elevated count of requests doesn't help identify the source of the problem.

We should either add labels to some of the existing metrics, or add new metrics with labels. An ideal label would be the full method name (ex: `Catalog.Register`) however if that is too high cardinality then simply `Catalog` would be an improvement, or one label for type (ex: `Catalog`) and one for a less-fine grained operation identifier (ex: `read` or `write`).

While there are a large number of RPC methods, most clusters will only use a small number of those endpoints, so a label for method name might be ok, despite it being potentially high cardinality.

Another important label is "target DC". RPC requests destined for another DC will get counted locally as well by these metrics, so splitting requests by target DC helps us understand where the requests are being handled.

Based on some learning in #10506, maybe a label for `blocking` vs not blocking would also be useful on this metric.

`error` (true or false) is another important label to be able to understand error rate.

[This comment](https://github.com/hashicorp/consul/issues/11947#issuecomment-1006101517) is also very relevant. We need to emit these metrics in other places that do direct `raft.Apply` calls as well, because those operations generate a very similar amount of load on the system.

The exact list of metric labels may change, but some important ones are:
* RPC method name - ex: `Catalog.Register`, `Health.ServiceChecks`, `KVS.Get`, `KVs.Apply`, etc.
* errored - true or false
* blocking_query - true or false
* request_type - {read, write}
* authenticated - true or false
* target_datacenter
* allow_stale - true or false
* server_state - follower or leader

Note that more granular labels like NodeID or IP of the requestor, or IDs of the requested item or returned item can not be part of metric labels because they are too high cardinality. Issue #11947 builds off of this one and provides a mechanism to report those more fine grained details in logs, and #11948 documents exactly which IDs can be included in those logs. All of this works builds in a direction to eventually add application tracing support to Consul.

Contributor guide

Open the contributing guide

Research direction

Start with the existing `consul.rpc.request` and related RPC metrics, then inspect the direct `raft.Apply` call sites mentioned in the issue and review issues #11947 and #11948. Done means agreeing on a bounded label set and emitting consistently useful metrics without high-cardinality labels, including coverage for the relevant RPC and raft operations.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend-api-design, distributed-systems, observability
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.