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

Support collecting gRPC core metrics #625

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Support collecting gRPC core metrics #625

wants to merge 13 commits into from

Conversation

overvenus
Copy link
Member

Collect gRPC core metrics:

  • Counters:
    • client_calls_created
    • server_calls_created
    • client_channels_created
    • client_subchannels_created
    • server_channels_created
    • insecure_connections_created
    • syscall_write
    • syscall_read
    • tcp_read_alloc_8k
    • tcp_read_alloc_64k
    • http2_settings_writes
    • http2_pings_sent
    • http2_writes_begun
    • http2_transport_stalls
    • http2_stream_stalls
    • cq_pluck_creates
    • cq_next_creates
    • cq_callback_creates
  • Histograms
    • call_initial_size
    • tcp_write_size
    • tcp_write_iov_size
    • tcp_read_size
    • tcp_read_offer
    • tcp_read_offer_iov_size
    • http2_send_message_size
    • http2_metadata_size

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.

Signed-off-by: Neil Shen <[email protected]>
Signed-off-by: Neil Shen <[email protected]>
Signed-off-by: Neil Shen <[email protected]>
Signed-off-by: Neil Shen <[email protected]>
Signed-off-by: Neil Shen <[email protected]>
Signed-off-by: Neil Shen <[email protected]>
Signed-off-by: Neil Shen <[email protected]>
@BusyJay
Copy link
Member

BusyJay commented Aug 1, 2023

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

@overvenus
Copy link
Member Author

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
Copy link
Member

BusyJay commented Aug 1, 2023

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
Copy link
Member Author

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
Copy link
Member

BusyJay commented Aug 1, 2023

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.

Also add some improvements

Signed-off-by: Neil Shen <[email protected]>
Signed-off-by: Neil Shen <[email protected]>
@overvenus overvenus requested review from BusyJay and tabokie August 4, 2023 11:59
@BusyJay
Copy link
Member

BusyJay commented Aug 7, 2023

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
Copy link
Member Author

tikv/grpc#50

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

@nolouch
Copy link

nolouch commented Jan 24, 2024

Can we merge it?

@overvenus
Copy link
Member Author

I think it's ready. I am fine with the current implementation and the implementation suggested by jay in the previous comment #625 (comment).

Copy link
Member

@Connor1996 Connor1996 left a comment

Choose a reason for hiding this comment

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

LGTM

@BusyJay
Copy link
Member

BusyJay commented Feb 1, 2024

Does it help collecting metrics? grpc/grpc#35348 And you need to update your branch before getting it merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants