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
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,26 @@
* <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) {
// If not on Compute Engine, FileNotFoundException will be thrown. Use empty string
// as it won't match with the GCE_PRODUCTION_NAME constants
systemProductName = "";
}
}

@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 @@ -147,6 +159,19 @@ private InstantiatingGrpcChannelProvider(Builder builder) {
: builder.directPathServiceConfig;
}

/**
* Package-Private constructor that is only visible for testing DirectPath functionality inside
* tests. This overrides the computed systemProductName when the class is initialized to help
* configure the result of {@link #isOnComputeEngine()} check.
*
* <p>If productName is null, that represents the result of an IOException
*/
@VisibleForTesting
InstantiatingGrpcChannelProvider(Builder builder, String productName) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding the EnvProvider as another param would need to make the envProvider variable non-final. I kept the package-private setter for now until we can migrate to using junit-pioneer to set the env vars.

this(builder);
systemProductName = productName;
}

/**
* @deprecated If executor is not set, this channel provider will create channels with default
* grpc executor.
Expand Down Expand Up @@ -257,8 +282,8 @@ private boolean isDirectPathEnabled() {
return false;
}

@VisibleForTesting
boolean isDirectPathXdsEnabled() {
@InternalApi
public boolean isDirectPathXdsEnabled() {
// Method 1: Enable DirectPath xDS by option.
if (Boolean.TRUE.equals(attemptDirectPathXds)) {
return true;
Expand Down Expand Up @@ -320,15 +345,9 @@ boolean isCredentialDirectPathCompatible() {
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;
}
// systemProductName will be empty string if not on Compute Engine
return systemProductName.contains(GCE_PRODUCTION_NAME_PRIOR_2016)
|| systemProductName.contains(GCE_PRODUCTION_NAME_AFTER_2016);
}
return false;
}
Expand Down Expand Up @@ -370,10 +389,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 +462,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 @@ -753,6 +787,12 @@ public Builder setAttemptDirectPathXds() {
return this;
}

@VisibleForTesting
Builder setEnvProvider(EnvironmentProvider envProvider) {
blakeli0 marked this conversation as resolved.
Show resolved Hide resolved
this.envProvider = envProvider;
return this;
}

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