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.

Sign up for GitHub

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

Jump to bottom

feat: expose property in GrpcTransportChannel if it uses direct path. #3170

Merged
merged 10 commits into from
Sep 13, 2024
Merged

feat: expose property in GrpcTransportChannel if it uses direct path. #3170

merged 10 commits into from
Sep 13, 2024

Conversation

zhumin8
Copy link
Contributor

@zhumin8 zhumin8 commented Sep 5, 2024 •
edited
Loading

This should fix #3134.

Spanner needs to know if GrpcSpannerStub’s transportChannel uses DirectPath for tracing purposes.
canUseDirectPath() from stubSettings.getTransportChannelProvider() does not provide accurate information because credentials are set afterwards when creating the transportChannel for GrpcSpannerStub (GrpcSpannerStub.create(stubSettings))

Proposed change:
Expose a property in GrpcTransportChannel and pass this information down from channel provider.
Spanner can access from ClientContext as shown below

SpannerStubSettings stubSettings = options
        .getSpannerStubSettings()
        .toBuilder()
        .setTransportChannelProvider(channelProvider)
        .setCredentialsProvider(credentialsProvider)
        .setStreamWatchdogProvider(watchdogProvider)
        .setTracerFactory(options.getApiTracerFactory())
        .build();
ClientContext clientContext = ClientContext.create(stubSettings);
// create GrpcSpannerStub from clientContext instead. 
// this is equivelant to GrpcSpannerStub.create(stubSettings)
this.spannerStub =
    GrpcSpannerStub.create(clientContext);
// access this property from transport channel.
((GrpcTransportChannel) clientContext.getTransportChannel()).isDirectPath();

@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Sep 5, 2024
@zhumin8
Copy link
Contributor Author

zhumin8 commented Sep 6, 2024 •
edited
Loading

Hi @mohanli-ml,
We are trying to expose information about whether direct path is used via transport channel (GrpcTransportChannel). And want to double check on what is considered a direct path. In particular, what is xds and is it one type of direct path?
Regarding logic here, is it fair to say that there are 2 flavors of direct path, one “basic” direct path and one c2p direct path? And criteria for c2p direct path is canUseDirectPath() && isDirectPathXdsEnabled()?
cc. @blakeli0

Update:
Answer from offline conv: These are two DirectPath infrastructures, but both considered DirectPath. Also, is xds enabled is set via Spanner here, thus no need to expose it.

@zhumin8 zhumin8 marked this pull request as ready for review September 12, 2024 17:59
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Sep 12, 2024
blakeli0
blakeli0 reviewed Sep 12, 2024
View reviewed changes
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcTransportChannel.java
@@ -100,7 +102,7 @@ public void close() {
}

public static Builder newBuilder() {
return new AutoValue_GrpcTransportChannel.Builder();
return new AutoValue_GrpcTransportChannel.Builder().setDirectPath(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add a test case for the default value here?

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 13, 2024

Quality Gate Passed Quality Gate passed for 'gapic-generator-java-root'

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 13, 2024

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@blakeli0 blakeli0 merged commit 9a432f7 into main Sep 13, 2024
49 checks passed
@blakeli0 blakeli0 deleted the fix-3134 branch September 13, 2024 15:34
@release-please release-please bot mentioned this pull request Sep 13, 2024
Merged
ldetmer pushed a commit that referenced this pull request Sep 17, 2024
@zhumin8 @ldetmer
feat: expose property in GrpcTransportChannel if it uses direct path. (…
8efc704 
…#3170)

This should fix #3134.

Spanner needs to know if `GrpcSpannerStub`’s transportChannel uses
DirectPath for tracing purposes.
`canUseDirectPath()` from `stubSettings.getTransportChannelProvider()`
does not provide accurate information because credentials are set
afterwards when creating the transportChannel for `GrpcSpannerStub`
(`GrpcSpannerStub.create(stubSettings)`)

Proposed change:
Expose a property in `GrpcTransportChannel` and pass this information
down from channel provider.
Spanner can access from `ClientContext` as shown below
```
SpannerStubSettings stubSettings = options
        .getSpannerStubSettings()
        .toBuilder()
        .setTransportChannelProvider(channelProvider)
        .setCredentialsProvider(credentialsProvider)
        .setStreamWatchdogProvider(watchdogProvider)
        .setTracerFactory(options.getApiTracerFactory())
        .build();
ClientContext clientContext = ClientContext.create(stubSettings);
// create GrpcSpannerStub from clientContext instead. 
// this is equivelant to GrpcSpannerStub.create(stubSettings)
this.spannerStub =
    GrpcSpannerStub.create(clientContext);
// access this property from transport channel.
((GrpcTransportChannel) clientContext.getTransportChannel()).isDirectPath();
```
lqiu96 pushed a commit that referenced this pull request Sep 23, 2024
@release-please
chore(main): release 2.46.0 (#3187)
e26db8a 
🤖 I have created a release *beep* *boop*
---


<details><summary>2.46.0</summary>

##
[2.46.0](v2.45.0...v2.46.0)
(2024-09-23)


### Features

* expose property in GrpcTransportChannel if it uses direct path.
([#3170](#3170))
([9a432f7](9a432f7))
* generate a GAPIC library from api definition
([#3208](#3208))
([b6b5d7b](b6b5d7b))
* Metrics tracer addAttribute map overload
([#3202](#3202))
([1a988df](1a988df))


### Bug Fixes

* generate pr description with repo level change
([#3182](#3182))
([edd2168](edd2168))


### Dependencies

* update dependency com.google.errorprone:error_prone_annotations to
v2.32.0
([#3192](#3192))
([b280706](b280706))
* update dependency com.google.errorprone:error_prone_annotations to
v2.32.0
([#3193](#3193))
([ed0cd17](ed0cd17))
* update dependency filelock to v3.16.1
([#3210](#3210))
([703ac3d](703ac3d))
* update dependency idna to v3.10
([#3201](#3201))
([211c3ec](211c3ec))
* update dependency org.threeten:threetenbp to v1.7.0
([#3205](#3205))
([c88a722](c88a722))
* update dependency org.threeten:threetenbp to v1.7.0
([#3206](#3206))
([3e9fbac](3e9fbac))
* update dependency platformdirs to v4.3.3
([#3200](#3200))
([b62b05d](b62b05d))
* update dependency platformdirs to v4.3.6
([#3209](#3209))
([227ffa5](227ffa5))
* update dependency urllib3 to v2.2.3
([#3194](#3194))
([f69d511](f69d511))
* update dependency virtualenv to v20.26.5
([#3212](#3212))
([d3ef97a](d3ef97a))
* update google api dependencies
([#3183](#3183))
([02eea8d](02eea8d))
* update google auth library dependencies to v1.26.0
([#3216](#3216))
([0b369e9](0b369e9))
* update google auth library dependencies to v1.27.0
([#3221](#3221))
([a3cb9e7](a3cb9e7))
* update googleapis/java-cloud-bom digest to 06f632d
([#3198](#3198))
([49dcd35](49dcd35))
* update googleapis/java-cloud-bom digest to e7d8909
([#3207](#3207))
([de497ee](de497ee))
* update opentelemetry-java monorepo to v1.42.1
([#3189](#3189))
([38117d8](38117d8))
* Upgrade Protobuf-Java to v3.25.5
([#3217](#3217))
([860c1bc](860c1bc))
</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>
@surbhigarg92 surbhigarg92 mentioned this pull request Oct 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blakeli0 blakeli0 blakeli0 approved these changes

Assignees
No one assigned
Labels
size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

canUseDirectPath returning false when DirectPath is enabled
2 participants
@zhumin8 @blakeli0