Conversation
There was a problem hiding this comment.
Code Review
This pull request disables gRPC Channelz by default to reduce per-RPC tracking and allocation overhead, while allowing users to override this behavior via client options. The feedback suggests making the new unit tests more robust by explicitly asserting that the pooled channels are initialized before checking their properties.
5371e26 to
3459f10
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request disables gRPC Channelz by default to reduce per-RPC tracking and allocation overhead, while allowing it to be overridden via client options. It also refactors client instantiation, promise chains, and adds corresponding unit tests. Regarding the feedback, the refactoring of getGrpcCredentials_ to use .catch(callback) after .then() introduces a risk of invoking the callback twice if the callback itself throws an error; it is recommended to use the two-argument form of .then() instead.
@grpc/grpc-js has Channelz enabled by default ('grpc.enable_channelz': 1),
which instruments every channel, subchannel, socket, and RPC attempt.
On every call start and completion, Channelz updates call counters and
allocates timestamps (`new Date()`) across both channel and transport
layers, as well as maintaining trace ring buffers and child reference maps.
Cloud Spanner telemetry is handled via OpenTelemetry, and the client does
not expose or consume the gRPC Channelz admin service.
This change sets 'grpc.enable_channelz': 0 by default in Spanner and
GrpcService channel options, causing @grpc/grpc-js to use no-op stubs
and eliminating per-RPC allocations. Users who require live connection
introspection (e.g. via grpcdebug) can still explicitly pass
'grpc.enable_channelz': 1 in SpannerOptions to re-enable it.
3459f10 to
fe420b1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the grpc.enable_channelz option to Spanner, disabling it by default to eliminate per-RPC tracking and allocation overhead while allowing users to override it. It also refactors client instantiation, cleans up promise chains, and adds corresponding tests. The review feedback points out that the integration test in test/spanner.ts asserts on deep, undocumented internal properties of @grpc/grpc-js and grpc-gcp, which makes the test suite fragile. It is recommended to simplify this test by only verifying that the option is correctly propagated.
@grpc/grpc-js has Channelz enabled by default ('grpc.enable_channelz': 1), which instruments every channel, subchannel, socket, and RPC attempt.
On every call start and completion, Channelz updates call counters and allocates timestamps (
new Date()) across both channel and transport layers, as well as maintaining trace ring buffers and child reference maps. Cloud Spanner telemetry is handled via OpenTelemetry, and the client does not expose or consume the gRPC Channelz admin service.This change sets 'grpc.enable_channelz': 0 by default in Spanner and GrpcService channel options, causing @grpc/grpc-js to use no-op stubs and eliminating per-RPC allocations. Users who require live connection introspection (e.g. via grpcdebug) can still explicitly pass 'grpc.enable_channelz': 1 in SpannerOptions to re-enable it.