[Performance] Debugging issue with node#8287
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a benchmarking suite for Spanner insert operations, including Go and Node.js implementations, containerization scripts, and Kubernetes manifests. However, the PR also includes significant regressions in the core library: it disables OpenTelemetry context initialization, guts tracing functionality, removes automatic project ID token replacement in the base gRPC service, and stops injecting request ID headers. Feedback highlights that these changes, while potentially useful for performance isolation, should be controlled by flags rather than hardcoded in the source. Additionally, inconsistencies were found in the benchmark scripts regarding Spanner versions, hardcoded build platforms, and gRPC connection pool sizes.
| this._observabilityOptions?.enableEndToEndTracing, | ||
| ); | ||
| ensureInitialContextManagerSet(); | ||
| // ensureInitialContextManagerSet(); |
There was a problem hiding this comment.
Commenting out ensureInitialContextManagerSet() disables the initialization of the OpenTelemetry context manager. This will break context propagation for tracing and metrics unless the user has manually configured a global context manager. If this was intended for performance debugging, it should be controlled via a flag rather than being commented out in the source code.
| delete reqOpts.objectMode; | ||
|
|
||
| return replaceProjectIdToken(reqOpts, this.projectId); | ||
| return reqOpts; |
There was a problem hiding this comment.
Removing replaceProjectIdToken from the base GrpcService class breaks the automatic placeholder resolution for all services extending this class. While the Spanner class has been modified to handle this (albeit with issues), other services or future extensions will fail if they rely on this base functionality.
| return reqOpts; | |
| return replaceProjectIdToken(reqOpts, this.projectId); |
| } | ||
| }, | ||
| ); | ||
| return cb(new noopSpan()); |
There was a problem hiding this comment.
Gutting the startTrace function effectively disables all tracing functionality in the library. While this might be useful for isolating performance bottlenecks during debugging, it is a major regression in functionality that should not be merged into production code.
|
|
||
| attempt = attempt || 1; | ||
| return _metadataWithRequestId(session, nthRequest!, attempt, headers); | ||
| return headers; |
There was a problem hiding this comment.
Disabling the injection of the x-goog-spanner-request-id header removes the ability to correlate client-side requests with server-side logs. This is a critical diagnostic tool for Spanner support and debugging.
|
|
||
| ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" | ||
| IMAGE_REPO="${IMAGE_REPO:-us-central1-docker.pkg.dev/span-cloud-testing/gargsurbhi-images}" | ||
| SPANNER_VERSION="${SPANNER_VERSION:-8.17.1}" |
There was a problem hiding this comment.
The default SPANNER_VERSION is set to 8.17.1, but the Kubernetes manifests and other Dockerfiles refer to 8.7.1. This discrepancy might lead to image pull errors or inconsistent benchmark results.
| SPANNER_VERSION="${SPANNER_VERSION:-8.17.1}" | |
| SPANNER_VERSION="${SPANNER_VERSION:-8.7.1}" |
| $CONTAINER_TOOL build --platform linux/amd64 -f issue/Dockerfile.node-raw -t $IMAGE_REPO/issue-raw-grpc-node:latest . | ||
| # Build Go Raw gRPC image | ||
| $CONTAINER_TOOL build --platform linux/amd64 -f issue/Dockerfile.go-raw -t $IMAGE_REPO/issue-raw-grpc-go:latest . |
There was a problem hiding this comment.
The platform is hardcoded to linux/amd64 here, ignoring the $PLATFORM variable defined earlier in the script. This prevents building for other architectures (e.g., linux/arm64).
| $CONTAINER_TOOL build --platform linux/amd64 -f issue/Dockerfile.node-raw -t $IMAGE_REPO/issue-raw-grpc-node:latest . | |
| # Build Go Raw gRPC image | |
| $CONTAINER_TOOL build --platform linux/amd64 -f issue/Dockerfile.go-raw -t $IMAGE_REPO/issue-raw-grpc-go:latest . | |
| $CONTAINER_TOOL build --platform "$PLATFORM" -f issue/Dockerfile.node-raw -t $IMAGE_REPO/issue-raw-grpc-node:latest . | |
| # Build Go Raw gRPC image | |
| $CONTAINER_TOOL build --platform "$PLATFORM" -f issue/Dockerfile.go-raw -t $IMAGE_REPO/issue-raw-grpc-go:latest . |
| fmt.Println("gRPC Connection Pool Size: 1") | ||
|
|
||
| // Initialize the generated client with a single connection to match Node.js default behavior | ||
| client, err := gapic.NewClient(ctx, option.WithGRPCConnectionPool(4)) |
There was a problem hiding this comment.
There is a mismatch between the code and the preceding comment. The comment states the client is initialized with a single connection, but the code uses a pool size of 4. Additionally, this is inconsistent with main.go which uses a pool size of 1, potentially making performance comparisons between the two benchmarks misleading.
| client, err := gapic.NewClient(ctx, option.WithGRPCConnectionPool(4)) | |
| client, err := gapic.NewClient(ctx, option.WithGRPCConnectionPool(1)) |
| let reqOpts; | ||
| if (!this.projectIdReplaced_) { | ||
| reqOpts = extend(true, {}, config.reqOpts); | ||
| reqOpts = replaceProjectIdToken(reqOpts, projectId!); |
There was a problem hiding this comment.
Isn't this 'undoing' the change further up? As in: Isn't this method called for every gRPC invocation, which means that this method will still be called for each request?
There was a problem hiding this comment.
prepareGapicRequest_ is called for each request. For the first request the projectIdReplaced_ will be replaced and set to true . And then database.formattedName_ and instance.formattedName_ will replace {{projectId}} to the actual project ID.
On subsequent requests the reqOpts will be constructed from database.formattedName_ and instance.formattedName_. which will have replaced projectID.
Having said that, I will do more testing to confirm this behavior, specially for parallel calls.
| ); | ||
| }); | ||
| }); | ||
| config.headers[CLOUD_RESOURCE_HEADER] = replaceProjectIdToken( |
There was a problem hiding this comment.
And here also? (Or is this method only called once at startup?)
3 participants
No description provided.