Export gRPC client and server metrics #6

acid-ant · 2023-10-17T15:59:51Z

acid-ant commented

2023-10-17 15:59:51 +00:00

Close #4

Description for gRPC metrics copied from open repositories, but this code licensed under Apache 2.0.
@fyrchik, @realloc could this be an obstacle for us?

Links to sources:
https://github.com/grpc-ecosystem/go-grpc-middleware/blob/main/providers/prometheus/server_metrics.go
https://github.com/grpc-ecosystem/go-grpc-middleware/blob/main/providers/prometheus/client_metrics.go

Signed-off-by: Anton Nikiforov an.nikiforov@yadro.com

Close #4 Description for gRPC metrics copied from open repositories, but this code licensed under Apache 2.0. @fyrchik, @realloc could this be an obstacle for us? Links to sources: https://github.com/grpc-ecosystem/go-grpc-middleware/blob/main/providers/prometheus/server_metrics.go https://github.com/grpc-ecosystem/go-grpc-middleware/blob/main/providers/prometheus/client_metrics.go Signed-off-by: Anton Nikiforov <an.nikiforov@yadro.com>

acid-ant requested review from storage-core-committers 2023-10-17 16:00:01 +00:00

acid-ant requested review from storage-core-developers 2023-10-17 16:00:01 +00:00

fyrchik reviewed 2023-10-19 06:43:35 +00:00

metrics/grpc/client.go Outdated

					
				@ -21,0 +58,4 @@

							VariableLabels: []string{"grpc_type", "grpc_service", "grpc_method"},

						},

					}

					metrics.MustRegister(clientMetrics, descs...)

fyrchik commented

2023-10-19 06:42:01 +00:00

Why export MustRegister instead of using our New* wrappers? Not saying this is a bad thing.

Why export `MustRegister` instead of using our `New*` wrappers? Not saying this is a bad thing.

acid-ant commented

2023-10-19 07:03:32 +00:00

Because all methods in grpcprom.ServerMetrics are private, there are no possibility to initialize them.

Because all methods in `grpcprom.ServerMetrics` are private, there are no possibility to initialize them.

fyrchik marked this conversation as resolved

metrics/grpc/server.go Outdated

					
				@ -8,3 +9,3 @@

				)

				var serverMetrics *grpcprom.ServerMetrics = grpcprom.NewServerMetrics(

				var serverMetrics = grpcprom.NewServerMetrics(

fyrchik commented

2023-10-19 06:43:32 +00:00

How is it different from what was before? We register metric descriptions, but the implementation is still in the imported package?

acid-ant commented

2023-10-19 07:42:09 +00:00

Didn't catch your question. You propose to create our own metrics instead of predefined?

fyrchik commented

2023-10-19 15:39:41 +00:00

Yes, but ok, let's do it in a separate task. It is not ideal currently -- definitions are decoupled from the actual metrics, so it is easy to miss something.

👍 1

metrics/grpc/server.go Outdated

					
				@ -16,2 +17,3 @@

				func init() {

					metrics.Register(serverMetrics)

					// Description copied from grpc-ecosystem:

					// https://github.com/grpc-ecosystem/go-grpc-middleware/blob/main/providers/prometheus/server_metrics.go