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.

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

feat: Allow Adding Client Level Attributes to MetricsTracerFactory #2614

Merged
merged 9 commits into from
Jun 3, 2024
1 change: 1 addition & 0 deletions gax-java/gax-grpc/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ _TEST_COMPILE_DEPS = [
"@com_google_api_grpc_grpc_google_common_protos//jar",
"//gax:gax_testlib",
"@com_googlecode_java_diff_utils_diffutils//jar",
"@org_junit_pioneer_junit_pioneer//jar",
]

java_library(
Expand Down
10 changes: 10 additions & 0 deletions gax-java/gax-grpc/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,16 @@
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<argLine>
--add-opens java.base/java.util=ALL-UNNAMED
--add-opens java.base/java.lang=ALL-UNNAMED
</argLine>
</configuration>
</plugin>
</plugins>
</build>
</project>
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import com.google.api.core.ApiFunction;
import com.google.api.core.BetaApi;
import com.google.api.core.InternalApi;
import com.google.api.core.InternalExtensionOnly;
import com.google.api.gax.core.ExecutorProvider;
import com.google.api.gax.rpc.FixedHeaderProvider;
import com.google.api.gax.rpc.HeaderProvider;
Expand Down Expand Up @@ -82,13 +81,25 @@
* <p>The client lib header and generator header values are used to form a value that goes into the
* http header of requests to the service.
*/
@InternalExtensionOnly
public final class InstantiatingGrpcChannelProvider implements TransportChannelProvider {
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved

static String systemProductName;

static {
try {
systemProductName =
Files.asCharSource(new File("/sys/class/dmi/id/product_name"), StandardCharsets.UTF_8)
.readFirstLine();
} catch (IOException e) {
// Keep existing behavior the same (null means it is not on compute engine)
systemProductName = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can return an empty String so that we don't have to do a null check below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Updating.

}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SystemProductName logic moved to a static block and initialized once. Stored in a variable so that it can be overriden.


@VisibleForTesting
static final Logger LOG = Logger.getLogger(InstantiatingGrpcChannelProvider.class.getName());

private static final String DIRECT_PATH_ENV_DISABLE_DIRECT_PATH =
"GOOGLE_CLOUD_DISABLE_DIRECT_PATH";
static final String DIRECT_PATH_ENV_DISABLE_DIRECT_PATH = "GOOGLE_CLOUD_DISABLE_DIRECT_PATH";
private static final String DIRECT_PATH_ENV_ENABLE_XDS = "GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS";
static final long DIRECT_PATH_KEEP_ALIVE_TIME_SECONDS = 3600;
static final long DIRECT_PATH_KEEP_ALIVE_TIMEOUT_SECONDS = 20;
Expand Down Expand Up @@ -145,6 +156,7 @@ private InstantiatingGrpcChannelProvider(Builder builder) {
builder.directPathServiceConfig == null
? getDefaultDirectPathServiceConfig()
: builder.directPathServiceConfig;
systemProductName = builder.systemProductName;
}

/**
Expand Down Expand Up @@ -319,16 +331,11 @@ boolean isCredentialDirectPathCompatible() {
@VisibleForTesting
static boolean isOnComputeEngine() {
String osName = System.getProperty("os.name");
if ("Linux".equals(osName)) {
try {
String result =
Files.asCharSource(new File("/sys/class/dmi/id/product_name"), StandardCharsets.UTF_8)
.readFirstLine();
return result.contains(GCE_PRODUCTION_NAME_PRIOR_2016)
|| result.contains(GCE_PRODUCTION_NAME_AFTER_2016);
} catch (IOException ignored) {
return false;
}
// The additional systemProductName null check is in case there is an IOException.
// The IOException will set the systemProductName to null and will return false
if ("Linux".equals(osName) && systemProductName != null) {
return systemProductName.contains(GCE_PRODUCTION_NAME_PRIOR_2016)
|| systemProductName.contains(GCE_PRODUCTION_NAME_AFTER_2016);
}
return false;
}
Expand Down Expand Up @@ -370,10 +377,7 @@ private ManagedChannel createSingleChannel() throws IOException {

// Check DirectPath traffic.
boolean useDirectPathXds = false;
if (isDirectPathEnabled()
&& isCredentialDirectPathCompatible()
&& isOnComputeEngine()
&& canUseDirectPathWithUniverseDomain()) {
if (canUseDirectPath()) {
CallCredentials callCreds = MoreCallCredentials.from(credentials);
ChannelCredentials channelCreds =
GoogleDefaultChannelCredentials.newBuilder().callCredentials(callCreds).build();
Expand Down Expand Up @@ -446,6 +450,24 @@ && canUseDirectPathWithUniverseDomain()) {
return managedChannel;
}

/**
* Marked as Internal Api and intended for internal use. DirectPath must be enabled via the
* settings and a few other configurations/settings must also be valid for the request to go
* through DirectPath.
*
* <p>Checks: 1. Credentials are compatible 2.Running on Compute Engine 3. Universe Domain is
* configured to for the Google Default Universe
*
* @return if DirectPath is enabled for the client AND if the configurations are valid
*/
@InternalApi
public boolean canUseDirectPath() {
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

We should also add a check for isDirectPathXdsEnabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mohanli-ml I believe you had helped implement this logic before. We're trying to expose a getter for the conditions that would enable DirectPath for this gRPC channel. Should isDirectPathXdsEnabled() be added here?

I copied over the original configs set:

if (isDirectPathEnabled()
&& isCredentialDirectPathCompatible()
&& isOnComputeEngine()
&& canUseDirectPathWithUniverseDomain()) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there are conditions that we have canUseDirectPath is true but isDirectPathXdsEnabled is false based on the current logic, maybe we can expose isDirectPathXdsEnabled as a public method, and the Spanner team can set a client level attribute based on canUseDirectPath() && isDirectPathXdsEnabled()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make isDirectPathXdsEnabled() public with @InternalApi annotation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@surbhigarg92 Would you be fine with the changes above?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lqiu96 LGTM.

return isDirectPathEnabled()
&& isCredentialDirectPathCompatible()
&& isOnComputeEngine()
&& canUseDirectPathWithUniverseDomain();
}

/** The endpoint to be used for the channel. */
@Override
public String getEndpoint() {
Expand Down Expand Up @@ -513,6 +535,7 @@ public static final class Builder {
@Nullable private Boolean attemptDirectPathXds;
@Nullable private Boolean allowNonDefaultServiceAccount;
@Nullable private ImmutableMap<String, ?> directPathServiceConfig;
@Nullable private String systemProductName;

private Builder() {
processorCount = Runtime.getRuntime().availableProcessors();
Expand Down Expand Up @@ -541,6 +564,7 @@ private Builder(InstantiatingGrpcChannelProvider provider) {
this.allowNonDefaultServiceAccount = provider.allowNonDefaultServiceAccount;
this.directPathServiceConfig = provider.directPathServiceConfig;
this.mtlsProvider = provider.mtlsProvider;
this.systemProductName = null;
}

/**
Expand Down Expand Up @@ -753,6 +777,16 @@ public Builder setAttemptDirectPathXds() {
return this;
}

/**
* Package-Private scope as it is used to test DirectPath functionality in tests. This overrides
* the computed systemProductName when the class is initialized.
*/
@VisibleForTesting
Builder setSystemProductName(String systemProductName) {
this.systemProductName = systemProductName;
return this;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Package-Private setter to allow tests to override the computed SystemProductName


/**
* Sets a service config for direct path. If direct path is not enabled, the provided service
* config will be ignored.
Expand Down
Loading
Loading