Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,20 @@ private Tuple<StorageSettings, Opts<UserProject>> resolveSettingsAndOpts() throw
Collections.singletonList(InstantiatingGrpcChannelProvider.HardBoundTokenTypes.ALTS));
}

if (!NoopGrpcInterceptorProvider.INSTANCE.equals(grpcInterceptorProvider)) {
StorageClientMetrics clientMetrics = getStorageClientMetrics();
if (clientMetrics != null) {
StorageGrpcMetricsInterceptor metricsInterceptor =
new StorageGrpcMetricsInterceptor(clientMetrics, getClock(), endpoint);
GrpcInterceptorProvider combinedProvider =
NoopGrpcInterceptorProvider.INSTANCE.equals(grpcInterceptorProvider)
? () -> ImmutableList.of(metricsInterceptor)
: () ->
ImmutableList.<ClientInterceptor>builder()
.addAll(grpcInterceptorProvider.getInterceptors())
.add(metricsInterceptor)
.build();
Comment on lines +338 to +345

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If grpcInterceptorProvider is null, calling grpcInterceptorProvider.getInterceptors() will throw a NullPointerException when the interceptor provider is invoked. We should add a null check to safely handle this case.

Suggested change
GrpcInterceptorProvider combinedProvider =
NoopGrpcInterceptorProvider.INSTANCE.equals(grpcInterceptorProvider)
? () -> ImmutableList.of(metricsInterceptor)
: () ->
ImmutableList.<ClientInterceptor>builder()
.addAll(grpcInterceptorProvider.getInterceptors())
.add(metricsInterceptor)
.build();
GrpcInterceptorProvider combinedProvider =
grpcInterceptorProvider == null
|| NoopGrpcInterceptorProvider.INSTANCE.equals(grpcInterceptorProvider)
? () -> ImmutableList.of(metricsInterceptor)
: () ->
ImmutableList.<ClientInterceptor>builder()
.addAll(grpcInterceptorProvider.getInterceptors())
.add(metricsInterceptor)
.build();

Comment on lines +338 to +345

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If grpcInterceptorProvider is null, NoopGrpcInterceptorProvider.INSTANCE.equals(grpcInterceptorProvider) evaluates to false. This leads to executing the else branch of the ternary operator, where calling grpcInterceptorProvider.getInterceptors() will throw a NullPointerException. Adding a null check for grpcInterceptorProvider prevents this potential runtime crash.

Suggested change
GrpcInterceptorProvider combinedProvider =
NoopGrpcInterceptorProvider.INSTANCE.equals(grpcInterceptorProvider)
? () -> ImmutableList.of(metricsInterceptor)
: () ->
ImmutableList.<ClientInterceptor>builder()
.addAll(grpcInterceptorProvider.getInterceptors())
.add(metricsInterceptor)
.build();
GrpcInterceptorProvider combinedProvider =
grpcInterceptorProvider == null
|| NoopGrpcInterceptorProvider.INSTANCE.equals(grpcInterceptorProvider)
? () -> ImmutableList.of(metricsInterceptor)
: () ->
ImmutableList.<ClientInterceptor>builder()
.addAll(grpcInterceptorProvider.getInterceptors())
.add(metricsInterceptor)
.build();

channelProviderBuilder.setInterceptorProvider(combinedProvider);
} else if (!NoopGrpcInterceptorProvider.INSTANCE.equals(grpcInterceptorProvider)) {
channelProviderBuilder.setInterceptorProvider(grpcInterceptorProvider);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
/*
* Copyright 2026 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.google.cloud.storage;

import com.google.api.core.ApiClock;
import io.grpc.Attributes;
import io.grpc.CallOptions;
import io.grpc.Channel;
import io.grpc.ClientCall;
import io.grpc.ClientInterceptor;
import io.grpc.ForwardingClientCall.SimpleForwardingClientCall;
import io.grpc.ForwardingClientCallListener.SimpleForwardingClientCallListener;
import io.grpc.Grpc;
import io.grpc.Metadata;
import io.grpc.MethodDescriptor;
import io.grpc.Status;
import io.opentelemetry.api.common.AttributesBuilder;
import io.opentelemetry.context.Context;
import java.net.InetAddress;
import java.net.InetSocketAddress;
import java.net.SocketAddress;
import java.util.concurrent.atomic.AtomicBoolean;
import org.checkerframework.checker.nullness.qual.Nullable;

/** gRPC client interceptor that records call metrics such as rpc.client.call.duration. */
final class StorageGrpcMetricsInterceptor implements ClientInterceptor {

private final StorageClientMetrics metrics;
private final ApiClock clock;
@Nullable private final String defaultEndpoint;

StorageGrpcMetricsInterceptor(
StorageClientMetrics metrics, ApiClock clock, @Nullable String defaultEndpoint) {
this.metrics = metrics;
this.clock = clock;
this.defaultEndpoint = defaultEndpoint;
}
Comment on lines +44 to +51

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

To avoid the overhead of string trimming and prefix matching on every single RPC completion, we can precompute whether the defaultEndpoint is a DirectPath endpoint once in the constructor.

  @Nullable private final String defaultEndpoint;
  private final boolean isDirectPathEndpoint;

  StorageGrpcMetricsInterceptor(
      StorageClientMetrics metrics, ApiClock clock, @Nullable String defaultEndpoint) {
    this.metrics = metrics;
    this.clock = clock;
    this.defaultEndpoint = defaultEndpoint;
    this.isDirectPathEndpoint =
        defaultEndpoint != null && defaultEndpoint.trim().startsWith("google-c2p:///");
  }


@Override
public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
MethodDescriptor<ReqT, RespT> method, CallOptions callOptions, Channel next) {
if (metrics == null || metrics.getRpcClientCallDuration() == null) {
return next.newCall(method, callOptions);
}

String shortMethod = extractShortMethodName(method.getFullMethodName());
String initialServerAddress =
StorageOptions.extractServerAddress(
defaultEndpoint != null ? defaultEndpoint : next.authority());
final String serverAddress =
initialServerAddress != null
? initialServerAddress
: StorageOptions.extractServerAddress(next.authority());
Comment on lines +61 to +67

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

When defaultEndpoint is null, StorageOptions.extractServerAddress(next.authority()) is called twice if it returns null. We can simplify this logic to avoid redundant calls and improve readability.

    String targetEndpoint = defaultEndpoint != null ? defaultEndpoint : next.authority();
    String resolvedAddress = StorageOptions.extractServerAddress(targetEndpoint);
    final String serverAddress =
        resolvedAddress != null
            ? resolvedAddress
            : (defaultEndpoint != null ? StorageOptions.extractServerAddress(next.authority()) : null);


Comment on lines +61 to +68

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The current logic for resolving serverAddress is redundant and hard to read. If defaultEndpoint is null, StorageOptions.extractServerAddress(next.authority()) is evaluated twice. Simplifying this logic avoids redundant method calls and improves readability.

Suggested change
String initialServerAddress =
StorageOptions.extractServerAddress(
defaultEndpoint != null ? defaultEndpoint : next.authority());
final String serverAddress =
initialServerAddress != null
? initialServerAddress
: StorageOptions.extractServerAddress(next.authority());
String shortMethod = extractShortMethodName(method.getFullMethodName());
String serverAddress = null;
if (defaultEndpoint != null) {
serverAddress = StorageOptions.extractServerAddress(defaultEndpoint);
}
if (serverAddress == null) {
serverAddress = StorageOptions.extractServerAddress(next.authority());
}

StorageMetricsContext metricsCtx = StorageMetricsContext.from(callOptions);
Context otelContext = metricsCtx != null ? metricsCtx.getOtelContext() : Context.current();

return new SimpleForwardingClientCall<ReqT, RespT>(next.newCall(method, callOptions)) {
@Override
public void start(Listener<RespT> responseListener, Metadata headers) {
long startTimeNanos = clock.nanoTime();
AtomicBoolean callRecorded = new AtomicBoolean(false);

super.start(
new SimpleForwardingClientCallListener<RespT>(responseListener) {
@Override
public void onClose(Status status, Metadata trailers) {
try {
if (callRecorded.compareAndSet(false, true)
&& metrics.getRpcClientCallDuration() != null) {
double durationSeconds =
Math.max(0.0, (clock.nanoTime() - startTimeNanos) / 1_000_000_000.0);
String networkPath = detectNetworkPath(getAttributes(), defaultEndpoint);
Comment on lines +85 to +87

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Update the call to detectNetworkPath to pass the precomputed isDirectPathEndpoint boolean instead of the raw defaultEndpoint string.

Suggested change
double durationSeconds =
Math.max(0.0, (clock.nanoTime() - startTimeNanos) / 1_000_000_000.0);
String networkPath = detectNetworkPath(getAttributes(), defaultEndpoint);
double durationSeconds =
Math.max(0.0, (clock.nanoTime() - startTimeNanos) / 1_000_000_000.0);
String networkPath = detectNetworkPath(getAttributes(), isDirectPathEndpoint);


AttributesBuilder attrBuilder =
metrics
.clientAttributesBuilder()
.put(StorageClientMetrics.KEY_RPC_SYSTEM_NAME, "grpc")
.put(StorageClientMetrics.KEY_RPC_METHOD, shortMethod)
.put(
StorageClientMetrics.KEY_RPC_RESPONSE_STATUS_CODE,
status.getCode().name())
.put(StorageClientMetrics.KEY_NETWORK_PATH, networkPath)
.put(
StorageClientMetrics.KEY_SERVER_ADDRESS,
serverAddress != null ? serverAddress : "")
.put(
StorageClientMetrics.KEY_ERROR_TYPE,
StorageErrorTypeMapper.mapGrpcStatus(status));

metrics
.getRpcClientCallDuration()
.record(durationSeconds, attrBuilder.build(), otelContext);
}
} finally {
super.onClose(status, trailers);
}
}
},
headers);
}
};
}

static String extractShortMethodName(@Nullable String fullMethodName) {
if (fullMethodName == null || fullMethodName.isEmpty()) {
return "UNKNOWN";
}
int idx = fullMethodName.lastIndexOf('/');
if (idx >= 0 && idx < fullMethodName.length() - 1) {
return fullMethodName.substring(idx + 1);
}
return fullMethodName;
}

static String detectNetworkPath(
@Nullable Attributes callAttributes, @Nullable String defaultEndpoint) {
SocketAddress remoteAddr =
callAttributes != null ? callAttributes.get(Grpc.TRANSPORT_ATTR_REMOTE_ADDR) : null;
if (remoteAddr instanceof InetSocketAddress) {
InetAddress inetAddr = ((InetSocketAddress) remoteAddr).getAddress();
if (inetAddr != null) {
return isDirectPathIp(inetAddr)
? StorageClientMetrics.NETWORK_PATH_DIRECTPATH
: StorageClientMetrics.NETWORK_PATH_CLOUDPATH;
}
}
if (defaultEndpoint != null && defaultEndpoint.trim().startsWith("google-c2p:///")) {
return StorageClientMetrics.NETWORK_PATH_DIRECTPATH;
}
return StorageClientMetrics.NETWORK_PATH_UNKNOWN;
}
Comment on lines +130 to +146

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

We can update detectNetworkPath to accept the precomputed isDirectPathEndpoint boolean instead of the raw defaultEndpoint string, avoiding string operations on the hot path.

Suggested change
static String detectNetworkPath(
@Nullable Attributes callAttributes, @Nullable String defaultEndpoint) {
SocketAddress remoteAddr =
callAttributes != null ? callAttributes.get(Grpc.TRANSPORT_ATTR_REMOTE_ADDR) : null;
if (remoteAddr instanceof InetSocketAddress) {
InetAddress inetAddr = ((InetSocketAddress) remoteAddr).getAddress();
if (inetAddr != null) {
return isDirectPathIp(inetAddr)
? StorageClientMetrics.NETWORK_PATH_DIRECTPATH
: StorageClientMetrics.NETWORK_PATH_CLOUDPATH;
}
}
if (defaultEndpoint != null && defaultEndpoint.trim().startsWith("google-c2p:///")) {
return StorageClientMetrics.NETWORK_PATH_DIRECTPATH;
}
return StorageClientMetrics.NETWORK_PATH_UNKNOWN;
}
static String detectNetworkPath(
@Nullable Attributes callAttributes, boolean isDirectPathEndpoint) {
SocketAddress remoteAddr =
callAttributes != null ? callAttributes.get(Grpc.TRANSPORT_ATTR_REMOTE_ADDR) : null;
if (remoteAddr instanceof InetSocketAddress) {
InetAddress inetAddr = ((InetSocketAddress) remoteAddr).getAddress();
if (inetAddr != null) {
return isDirectPathIp(inetAddr)
? StorageClientMetrics.NETWORK_PATH_DIRECTPATH
: StorageClientMetrics.NETWORK_PATH_CLOUDPATH;
}
}
if (isDirectPathEndpoint) {
return StorageClientMetrics.NETWORK_PATH_DIRECTPATH;
}
return StorageClientMetrics.NETWORK_PATH_UNKNOWN;
}


static boolean isDirectPathIp(InetAddress addr) {
byte[] b = addr.getAddress();
if (b.length == 4) {
return (b[0] & 0xFF) == 34 && (b[1] & 0xFF) == 126 && ((b[2] & 0xFF) & 0xC0) == 0;
}
if (b.length == 16) {
return (b[0] & 0xFF) == 0x20
&& (b[1] & 0xFF) == 0x01
&& (b[2] & 0xFF) == 0x48
&& (b[3] & 0xFF) == 0x60
&& (b[4] & 0xFF) == 0x80
&& ((b[5] & 0xFF) & 0xC0) == 0x40;
}
return false;
}
}
Loading
Loading