fix: Decrease log level for directpath warnings outside GCE#4139
fix: Decrease log level for directpath warnings outside GCE#4139lqiu96 merged 1 commit intogoogleapis:mainfrom
Conversation
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 refines the logging behavior for DirectPath misconfigurations within the Highlights
Changelog
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 refactors the DirectPath misconfiguration logging to avoid warning about its unavailability when not running on a GCE environment. The changes in the production code look good and improve clarity. However, a test was modified in a way that removes the cleanup logic for a TransportChannel, which could lead to resource leaks. I've added a comment to address this in the test file.
...ava/gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
Show resolved
Hide resolved
|
@lqiu96 can I get a gcbrun on this, please? |
|
/gcbrun |
|
/gcbrun |
|
Showcase Error: Flaky showcase test. I will re-run showcase tests |
|
/gcbrun |
|
Upper bounds CI failure is known issue. Looks like Wes has a PR to try and address it: #4143 |
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Outdated
Show resolved
Hide resolved
| .contains( | ||
| "DirectPath is misconfigured. The DirectPath XDS option was set, but the attemptDirectPath option was not. Please set both the attemptDirectPath and attemptDirectPathXds options."); | ||
|
|
||
| if (InstantiatingGrpcChannelProvider.isOnComputeEngine()) { |
There was a problem hiding this comment.
is it possible to use mockito-inline to mock this static method? IIUC, otherwise we're always skipping this in the test
There was a problem hiding this comment.
Oh, I just kept it as it was in these tests. I thought the tests run in different environments.
There was a problem hiding this comment.
We don't have dedicated tests to run on different environments (GCE) or tests to hit DirectPath.
I think we indirectly test these when we run the GraalVM checks (via Cloud Build) as they also run the unit tests. On second though, we probably do need to keep this check.
I guess users outside of GCE should create a different client without direct path enabled? Are they trying to use the same client configuration for both GCE and non-GCE? |
we don’t want to have a different client for directpath/cloudpath. And we don’t want our end users to know anything about directpath…it’s supposed to be an implementation detail |
e359aa4 to
d785023
Compare
|
/gcbrun |
🤖 I have created a release *beep* *boop* --- <details><summary>2.68.0</summary> ## [2.68.0](v2.67.0...v2.68.0) (2026-03-17) ### Features * Add client request duration metric. ([#4132](#4132)) ([6a76397](6a76397)) * Add more attributes to golden signals metrics. ([#4135](#4135)) ([59d0624](59d0624)) * **gax-httpjson:** add HttpJsonErrorParser utility ([#4137](#4137)) ([a1b7565](a1b7565)) * **generator:** add extra allowed modules that will not be removed from the monorepo if they are present ([#4124](#4124)) ([774fe6e](774fe6e)) * **o11y:** introduce `gcp.client.repo` and `gcp.client.artifact` attributes ([#4120](#4120)) ([105f644](105f644)) * **o11y:** Introduce `rpc.system.name` and `rpc.method` in gRPC ([#4121](#4121)) ([7ab6d2e](7ab6d2e)) * **o11y:** introduce server.port attribute ([#4128](#4128)) ([56aa343](56aa343)) ### Bug Fixes * add null checks for ApiTracerFactory in ClientContext ([#4122](#4122)) ([4b3dbe2](4b3dbe2)) * Decrease log level for directpath warnings outside GCE ([#4139](#4139)) ([c9651e7](c9651e7)) * **gax-grpc:** add pick_first fallback to direct path service config ([#4143](#4143)) ([b150fe9](b150fe9)) * Populate method level attributes in metrics recording ([#4149](#4149)) ([7b7e6c9](7b7e6c9)) * suppress warnings in generated projects for non-idiomatic durations ([#4119](#4119)) ([4206e6e](4206e6e)) * Use ServiceName + MethodName as the regex for Otel ([#2543](#2543)) ([b9ae73f](b9ae73f)) ### Documentation * **hermetic_build:** fix config field name in readme ([#4130](#4130)) ([a0c8f67](a0c8f67)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Directpath is not available outside of GCE. When a Cloud service client has Directpath enabled, the users with clients outside of GCE receive this warning and it's not actionable.