Conversation
…gsValidation Without SPANNER_EMULATOR_HOST set, the Spanner client library doesn't add the passthrough:/// prefix or skip dial settings validation. This causes ~100x slowdown (~1s per client creation) because the new authentication library hits a slow auth path. Since spanemuboost connects via emu.URI() without setting the env var, the fix from googleapis/google-cloud-go#10947 was being bypassed. Add passthrough:/// prefix to endpoint and internaloption.SkipDialSettingsValidation() to both ClientOptions() and defaultClientOpts(). Fixes #7 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical performance bottleneck affecting client creation for the Spanner emulator, particularly when the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request aims to fix slow client creation by adding passthrough:/// to the endpoint and SkipDialSettingsValidation. These changes should effectively address the performance overhead with emulator connections. The use of google.golang.org/api/option/internaloption introduces a dependency on an internal, unstable API, which has been highlighted in a comment due to potential maintainability risks.
emulator.go
Outdated
| option.WithEndpoint("passthrough:///" + e.container.URI()), | ||
| option.WithGRPCDialOption(grpc.WithTransportCredentials(insecure.NewCredentials())), | ||
| option.WithoutAuthentication(), | ||
| internaloption.SkipDialSettingsValidation(), |
There was a problem hiding this comment.
Using internaloption.SkipDialSettingsValidation() introduces a dependency on an internal package of the Google Cloud client library. Internal APIs are not guaranteed to be stable and can change or be removed in future versions without warning, which could break this library.
While the performance gain is significant, it's important to acknowledge this trade-off. Consider adding a comment here explaining why this internal option is used and the potential for future breakage.
There was a problem hiding this comment.
Good point. Added inline comments and a doc comment explaining:
- Why
passthrough:///is needed (bypass gRPC DNS resolution / slow auth path) - Why
SkipDialSettingsValidationis required (thepassthrough:///prefix fails default endpoint validation) - That this is the same pattern used by the Spanner, Bigtable, and Datastore client libraries for their emulator paths (perf(spanner): use passthrough with emulator endpoint googleapis/google-cloud-go#10947, #13739, #13807)
Regarding the internaloption stability concern: this is a valid trade-off, but the same internal option is used by multiple Google Cloud SDKs in their emulator code paths, so it's unlikely to be removed without a replacement. If it does break, the fix would be straightforward.
…n rationale Document why the passthrough:/// scheme and internaloption.SkipDialSettingsValidation are used, referencing the upstream Spanner/Bigtable/Datastore SDKs that follow the same pattern. Note that this is currently a defensive measure for the planned grpc.DialContext → grpc.NewClient migration in the auth layer. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a performance optimization for client creation by adding the passthrough:/// prefix and SkipDialSettingsValidation option. This is a good defensive measure to align with the practices of official Google Cloud SDKs and prevent future performance regressions. The changes are well-documented and the test updates are appropriate. My review includes a suggestion to refactor a small amount of code duplication to improve long-term maintainability.
emulator.go
Outdated
| return []option.ClientOption{ | ||
| option.WithEndpoint(e.container.URI()), | ||
| // passthrough:/// tells gRPC to use the address as-is without DNS resolution. | ||
| option.WithEndpoint("passthrough:///" + e.container.URI()), | ||
| option.WithGRPCDialOption(grpc.WithTransportCredentials(insecure.NewCredentials())), | ||
| option.WithoutAuthentication(), | ||
| // SkipDialSettingsValidation is required because the passthrough:/// prefix | ||
| // fails the default endpoint validation. This is an internal option also used | ||
| // by the Spanner, Bigtable, and Datastore client libraries for emulator paths. | ||
| internaloption.SkipDialSettingsValidation(), | ||
| } |
There was a problem hiding this comment.
This function's implementation is identical to defaultClientOpts in internal.go. To avoid code duplication and improve maintainability, I suggest refactoring this to call defaultClientOpts. I've made a related suggestion on internal.go to move the inline comments there to keep the explanation with the implementation.
return defaultClientOpts(e.container)There was a problem hiding this comment.
Done. ClientOptions() now delegates to defaultClientOpts(e.container). Inline comments moved to defaultClientOpts as the single implementation.
defaultClientOpts is shared between ClientOptions() (new API) and the deprecated newClients path. Once the deprecated path is removed, defaultClientOpts should be inlined back into ClientOptions().
| return []option.ClientOption{ | ||
| option.WithEndpoint(emulator.URI()), | ||
| option.WithEndpoint("passthrough:///" + emulator.URI()), | ||
| option.WithGRPCDialOption(grpc.WithTransportCredentials(insecure.NewCredentials())), | ||
| option.WithoutAuthentication(), | ||
| internaloption.SkipDialSettingsValidation(), | ||
| } |
There was a problem hiding this comment.
To centralize the logic for creating client options as suggested in the comment on emulator.go, the inline comments explaining the options should be moved here. This keeps the implementation and its documentation together.
return []option.ClientOption{
// passthrough:/// tells gRPC to use the address as-is without DNS resolution.
option.WithEndpoint("passthrough:///" + emulator.URI()),
option.WithGRPCDialOption(grpc.WithTransportCredentials(insecure.NewCredentials())),
option.WithoutAuthentication(),
// SkipDialSettingsValidation is required because the passthrough:/// prefix
// fails the default endpoint validation. This is an internal option also used
// by the Spanner, Bigtable, and Datastore client libraries for emulator paths.
internaloption.SkipDialSettingsValidation(),
}There was a problem hiding this comment.
Done. Inline comments are now in defaultClientOpts alongside the implementation.
ClientOptions() and defaultClientOpts() had identical implementations. Refactor ClientOptions() to delegate to defaultClientOpts(), and move inline comments to defaultClientOpts() as the single source of truth. Note that defaultClientOpts should be inlined back into ClientOptions() once the deprecated newClients path is removed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request proactively addresses a potential performance issue with Spanner client creation by adding the passthrough:/// prefix to the endpoint and including internaloption.SkipDialSettingsValidation. The code is well-commented and refactored to avoid duplication. I have one suggestion to improve the corresponding test to make it more specific.
| if opts := emu.ClientOptions(); len(opts) != 4 { | ||
| t.Errorf("ClientOptions() returned %d options, want 4", len(opts)) | ||
| } |
There was a problem hiding this comment.
This test correctly checks that the number of client options has increased to 4. However, it could be made more robust by also verifying the content of the options. Specifically, it would be valuable to assert that one of the options sets the endpoint with the passthrough:/// prefix, as this is a key part of the change.
While inspecting option.ClientOption can be tricky and might require reflection, it would provide a stronger guarantee that ClientOptions() is behaving as expected and make the test less brittle to future changes that might alter the number of options.
There was a problem hiding this comment.
Thanks for the suggestion. I'll leave this as a count check for now — option.ClientOption is an opaque interface, so verifying content would require reflection or type assertions against internal types, adding complexity without proportional value. The count check serves as a sufficient guard to catch unintended additions or removals.
|
/gemini summary |
Summary of ChangesThis pull request addresses a potential future performance regression in Spanner emulator client creation by proactively optimizing gRPC client options. By introducing a 'passthrough:///' endpoint prefix and skipping dial settings validation, the changes ensure efficient client instantiation, preventing a significant slowdown that would occur with a planned migration in the authentication layer. This aligns the library's behavior with established patterns in other Google Cloud SDKs for emulator connections. Highlights
Changelog
Activity
|
Summary
passthrough:///prefix to endpoint inClientOptions()anddefaultClientOpts()to bypass gRPC name resolution and the slow auth pathinternaloption.SkipDialSettingsValidation()(required becausepassthrough:///fails default endpoint validation)Background
The Spanner client library's
init()hooks automatically applypassthrough:///andSkipDialSettingsValidationwhenSPANNER_EMULATOR_HOSTis set (googleapis/google-cloud-go#10947). Since spanemuboost connects viaemu.URI()without setting that env var, these options were missing.Currently the auth layer uses
grpc.DialContext(passthrough resolver by default), so the performance issue is not actively triggered. However, when the auth layer migrates togrpc.NewClient(dns resolver by default), spanemuboost users would silently regress with ~100x slower client creation. This is a defensive fix that aligns with the same pattern used by Spanner, Bigtable, and Datastore SDKs for their emulator paths.See #7 for the full timeline and analysis.
Test plan
go build ./...passesgo vet ./...passesgo test -run TestEmulatorAccessors -v(updated assertion from 3 to 4 options)go test ./... -count=1Fixes #7
🤖 Generated with Claude Code