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

REST VC metrics #13588

Merged
merged 17 commits into from
Mar 5, 2024
Merged

REST VC metrics #13588

merged 17 commits into from
Mar 5, 2024

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Feb 6, 2024

No description provided.

@rkapka rkapka added Ready For Review A pull request ready for code review Validator Client labels Feb 6, 2024
@rkapka rkapka requested a review from a team as a code owner February 6, 2024 17:49
terencechain
terencechain previously approved these changes Feb 7, 2024
Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

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

For what it's worth, the current implementation for gRPC has two pairs of counter metrics tracking on the client side.

  1. unary requests (started_total, handled_total)
  2. stream requests (started_total, handled_total)

https://github.com/grpc-ecosystem/go-grpc-middleware/blob/37827591b26cd8d936deec02939c80820462b860/providers/prometheus/client_metrics.go#L34

Then it has three histograms

  1. client handled (unary requests)
  2. client stream sent
  3. client stream received

https://github.com/grpc-ecosystem/go-grpc-middleware/blob/37827591b26cd8d936deec02939c80820462b860/providers/prometheus/client_options.go#L40

All of these have three labels []string{"grpc_type", "grpc_service", "grpc_method"}) and sometimes a fourth label grpc_code.

With all of that, I would suggest that we mirror that in the implementation here for http.

Counters:

  • Total number of RPCs started on the client (labels = "type", "service", "method")
  • Total number of RPCs completed by the client, regardless of success or failure. (labels = "type", "service", "method", "code")

Histograms

  • Histogram of response latency (seconds) of the RPC until it is finished by the application. (labels = "type", "service", "method")

# Conflicts:
#	beacon-chain/node/node.go
#	beacon-chain/rpc/service.go
#	beacon-chain/rpc/service_test.go
var (
httpRequestLatency = promauto.NewHistogramVec(
prometheus.HistogramOpts{
Name: "http_request_latency_seconds",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

promhttp measures seconds

@rkapka rkapka added the Blocked Blocked by research or external factors label Feb 19, 2024
@rkapka rkapka removed the Blocked Blocked by research or external factors label Feb 26, 2024
# Conflicts:
#	beacon-chain/rpc/service.go
#	beacon-chain/rpc/service_test.go
#	validator/client/beacon-api/beacon_api_validator_client.go
# Conflicts:
#	beacon-chain/rpc/service.go
@rkapka rkapka added this pull request to the merge queue Mar 5, 2024
Merged via the queue into develop with commit ee9274a Mar 5, 2024
17 checks passed
@rkapka rkapka deleted the rest-metrics branch March 5, 2024 19:07
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review Validator Client
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants