tikv / grpc-rs

The gRPC library for Rust built on C Core library and futures
Apache License 2.0
1.81k stars 253 forks source link

Support collecting gRPC core metrics #625

Open overvenus opened 1 year ago

overvenus commented 1 year ago

Collect gRPC core metrics:

Details: https://github.com/grpc/grpc/blob/v1.56.0/src/core/lib/debug/stats_data.yaml

This PR add a new cc file called grpc_wrap_stats.cc and generates bindings into a new file called binding_stats.rs. Collecting metrics needs to include headers under in grpc/src/. I think it's better to be an optional feature and codes are in separated files.

BusyJay commented 1 year ago

If the stats are always collected in C/C++ code, then there seems to be no benefits to make it an optional feature.

overvenus commented 1 year ago

If the stats are always collected in C/C++ code, then there seems to be no benefits to make it an optional feature.

If so, GRPCIO_SYS_USE_PKG_CONFIG needs to be removed because stats need to include files under grpc/src.

BusyJay commented 1 year ago

If so, GRPCIO_SYS_USE_PKG_CONFIG needs to be removed because stats need to include files under grpc/src.

You can depend on GRPCIO_SYS_USE_PKG_CONFIG to decide whether implement the feature.

By the way, there should be also standard way to expose metrics like open census and

overvenus commented 1 year ago

You can depend on GRPCIO_SYS_USE_PKG_CONFIG to decide whether implement the feature.

Do you mean functions in stats become no-op when GRPCIO_SYS_USE_PKG_CONFIG=1? Could you elaborate?

By the way, there should be also standard way to expose metrics like open census and

open census and channelz are included in grpc/include, while stats is not. I'm not sure what's the standard way.

BusyJay commented 1 year ago

Do you mean functions in stats become no-op when GRPCIO_SYS_USE_PKG_CONFIG=1

Maybe you should use a feature name internals to indicate the APIs are available only when they are compiled from sources.

open census and channelz should be the standard ways as they are public APIs.

BusyJay commented 1 year ago

How about moving c changes, for example, exposing a header, to tikv/grpc instead? So that users can choose to patch grpc or just ignore unknown symbols.

overvenus commented 1 year ago

https://github.com/tikv/grpc/pull/50

Not sure if this is what you mean, but please take a look.

nolouch commented 10 months ago

Can we merge it?

overvenus commented 10 months ago

I think it's ready. I am fine with the current implementation and the implementation suggested by jay in the previous comment https://github.com/tikv/grpc-rs/pull/625#issuecomment-1667103943.

BusyJay commented 9 months ago

Does it help collecting metrics? https://github.com/grpc/grpc/pull/35348 And you need to update your branch before getting it merged.