-
Notifications
You must be signed in to change notification settings - Fork 53
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
Conversation
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Show resolved
Hide resolved
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Show resolved
Hide resolved
0d9781f
to
05b1cde
Compare
MetricsTracer metricsTracer = | ||
new MetricsTracer( | ||
MethodName.of(spanName.getClientName(), spanName.getMethodName()), metricsRecorder); | ||
for (Map.Entry<String, String> attributeEntrySet : attributes.entrySet()) { | ||
metricsTracer.addAttributes(attributeEntrySet.getKey(), attributeEntrySet.getValue()); | ||
} | ||
return metricsTracer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possible options:
- Use MetricTracer's
addAttributes()
to add only add client level attributes. The clientName and methodName attributes will continue to be passed via MethodName via the constructor. - Add MethodName attribute with
addAttributes()
and refactorMetricsTracer
to only take in metricsRecorder
Probably going to keep it this way (option 1).
public MetricsTracerFactory(MetricsRecorder metricsRecorder, Map<String, String> attributes) { | ||
this.metricsRecorder = metricsRecorder; | ||
this.attributes = attributes; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional constructor to take in user configured in client level attributes.
In the future, we can have Gax automatically collect attributes and this would need a constructor to take in a set of attribute flags. Class is marked with InternalApi and could be open to more refactoring if we want to pursue this.
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Show resolved
Hide resolved
"Echo.Echo", | ||
MetricsTracer.LANGUAGE_ATTRIBUTE, | ||
MetricsTracer.DEFAULT_LANGUAGE, | ||
"directpath_enabled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract the String literals that are used twice to variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITOtelMetrics.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsTracerFactory.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsTracerFactory.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsTracerFactory.java
Outdated
Show resolved
Hide resolved
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Show resolved
Hide resolved
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
Show resolved
Hide resolved
@@ -117,6 +126,7 @@ public final class InstantiatingGrpcChannelProvider implements TransportChannelP | |||
@Nullable private final Boolean allowNonDefaultServiceAccount; | |||
@VisibleForTesting final ImmutableMap<String, ?> directPathServiceConfig; | |||
@Nullable private final MtlsProvider mtlsProvider; | |||
private final SystemProductNameReader systemProductNameReader; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally we thought exposing a default scope method would be good enough, but now that we are introducing a new field and a new private class just for testing, I'm not sure it is worth it. I'm now actually leaning towards keep isOnComputeEngine()
as it is.
In the future, we can introduce mockito-inline
or mockito v5
(requires Java 11) that can mock/spy final classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a bit of change added just for testing. I think there is quite a bit of added benefit, namely that we get to test multiple functions and also to get to test how they interact together:
Lines 668 to 809 in b1be846
@Test | |
public void canUseDirectPath_happyPath() { | |
InstantiatingGrpcChannelProvider provider = | |
InstantiatingGrpcChannelProvider.newBuilder() | |
.setEnvProvider(envProvider) | |
.setAttemptDirectPath(true) | |
.setCredentials(computeEngineCredentials) | |
.setSystemProductNameReader(systemProductNameReader) | |
.setEndpoint(DEFAULT_ENDPOINT) | |
.build(); | |
Truth.assertThat(provider.canUseDirectPath()).isTrue(); | |
} | |
@Test | |
public void canUseDirectPath_directPathEnvVarDisabled() { | |
EnvironmentProvider envProvider = Mockito.mock(EnvironmentProvider.class); | |
Mockito.when( | |
envProvider.getenv( | |
InstantiatingGrpcChannelProvider.DIRECT_PATH_ENV_DISABLE_DIRECT_PATH)) | |
.thenReturn("true"); | |
InstantiatingGrpcChannelProvider provider = | |
InstantiatingGrpcChannelProvider.newBuilder() | |
.setEnvProvider(envProvider) | |
.setAttemptDirectPath(true) | |
.setCredentials(computeEngineCredentials) | |
.setSystemProductNameReader(systemProductNameReader) | |
.setEndpoint(DEFAULT_ENDPOINT) | |
.build(); | |
Truth.assertThat(provider.canUseDirectPath()).isFalse(); | |
} | |
@Test | |
public void canUseDirectPath_directPathEnvVarNotSet_attemptDirectPathIsTrue() { | |
EnvironmentProvider envProvider = Mockito.mock(EnvironmentProvider.class); | |
// If system property is not set, then System.getProperty() returns null | |
Mockito.when( | |
envProvider.getenv( | |
InstantiatingGrpcChannelProvider.DIRECT_PATH_ENV_DISABLE_DIRECT_PATH)) | |
.thenReturn(null); | |
InstantiatingGrpcChannelProvider provider = | |
InstantiatingGrpcChannelProvider.newBuilder() | |
.setEnvProvider(envProvider) | |
.setAttemptDirectPath(true) | |
.setCredentials(computeEngineCredentials) | |
.setSystemProductNameReader(systemProductNameReader) | |
.setEndpoint(DEFAULT_ENDPOINT) | |
.build(); | |
Truth.assertThat(provider.canUseDirectPath()).isTrue(); | |
} | |
@Test | |
public void canUseDirectPath_directPathEnvVarNotSet_attemptDirectPathIsFalse() { | |
EnvironmentProvider envProvider = Mockito.mock(EnvironmentProvider.class); | |
Mockito.when( | |
envProvider.getenv( | |
InstantiatingGrpcChannelProvider.DIRECT_PATH_ENV_DISABLE_DIRECT_PATH)) | |
.thenReturn(null); | |
InstantiatingGrpcChannelProvider provider = | |
InstantiatingGrpcChannelProvider.newBuilder() | |
.setEnvProvider(envProvider) | |
.setAttemptDirectPath(false) | |
.setCredentials(computeEngineCredentials) | |
.setSystemProductNameReader(systemProductNameReader) | |
.setEndpoint(DEFAULT_ENDPOINT) | |
.build(); | |
Truth.assertThat(provider.canUseDirectPath()).isFalse(); | |
} | |
@Test | |
public void canUseDirectPath_nonComputeCredentials() { | |
Credentials credentials = Mockito.mock(Credentials.class); | |
InstantiatingGrpcChannelProvider provider = | |
InstantiatingGrpcChannelProvider.newBuilder() | |
.setEnvProvider(envProvider) | |
.setAttemptDirectPath(true) | |
.setCredentials(credentials) | |
.setSystemProductNameReader(systemProductNameReader) | |
.setEndpoint(DEFAULT_ENDPOINT) | |
.build(); | |
Truth.assertThat(provider.canUseDirectPath()).isFalse(); | |
} | |
@Test | |
public void canUseDirectPath_systemProductNameInvalid() throws IOException { | |
InstantiatingGrpcChannelProvider.SystemProductNameReader systemProductNameReader = | |
Mockito.mock(InstantiatingGrpcChannelProvider.SystemProductNameReader.class); | |
Mockito.when(systemProductNameReader.getSystemProductName()).thenReturn("testing"); | |
InstantiatingGrpcChannelProvider provider = | |
InstantiatingGrpcChannelProvider.newBuilder() | |
.setEnvProvider(envProvider) | |
.setAttemptDirectPath(true) | |
.setCredentials(computeEngineCredentials) | |
.setSystemProductNameReader(systemProductNameReader) | |
.setEndpoint(DEFAULT_ENDPOINT) | |
.build(); | |
Truth.assertThat(provider.canUseDirectPath()).isFalse(); | |
} | |
@Test | |
public void canUseDirectPath_systemPropertyIsNotLinux() throws IOException { | |
System.setProperty("os.name", "Windows"); | |
InstantiatingGrpcChannelProvider provider = | |
InstantiatingGrpcChannelProvider.newBuilder() | |
.setEnvProvider(envProvider) | |
.setAttemptDirectPath(true) | |
.setCredentials(computeEngineCredentials) | |
.setSystemProductNameReader(systemProductNameReader) | |
.setEndpoint(DEFAULT_ENDPOINT) | |
.build(); | |
Truth.assertThat(provider.canUseDirectPath()).isFalse(); | |
} | |
@Test | |
public void canUseDirectPath_systemProductNameThrowsIOException() throws IOException { | |
InstantiatingGrpcChannelProvider.SystemProductNameReader systemProductNameReader = | |
Mockito.mock(InstantiatingGrpcChannelProvider.SystemProductNameReader.class); | |
Mockito.when(systemProductNameReader.getSystemProductName()).thenThrow(new IOException()); | |
InstantiatingGrpcChannelProvider provider = | |
InstantiatingGrpcChannelProvider.newBuilder() | |
.setEnvProvider(envProvider) | |
.setAttemptDirectPath(true) | |
.setCredentials(computeEngineCredentials) | |
.setSystemProductNameReader(systemProductNameReader) | |
.setEndpoint(DEFAULT_ENDPOINT) | |
.build(); | |
Truth.assertThat(provider.canUseDirectPath()).isFalse(); | |
} | |
@Test | |
public void canUseDirectPath_nonGDUUniverseDomain() { | |
String nonGDUEndpoint = "test.random.com:443"; | |
InstantiatingGrpcChannelProvider provider = | |
InstantiatingGrpcChannelProvider.newBuilder() | |
.setEnvProvider(envProvider) | |
.setAttemptDirectPath(true) | |
.setCredentials(computeEngineCredentials) | |
.setSystemProductNameReader(systemProductNameReader) | |
.setEndpoint(nonGDUEndpoint) | |
.build(); | |
Truth.assertThat(provider.canUseDirectPath()).isFalse(); | |
} |
The added classes and methods are all package-private scope and should not impact the customers at all. And since they are package-private, we should be able to easily remove and refactor to use mockito v5 for this same purpose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is the same but the behavior is slightly different. isOnComputeEngine()
used to be a static method, now it is not because we have to initialize an instance of SystemProductNameReader
. It would be great if we can keep isOnComputeEngine()
static. Or even better, isOnComputeEngine()
does not have to be a static method, it could be a static block so that it does not get executed every time we call it. The benefit is marginal though since InstantiatingGrpcChannelProvider
should only be called during client initialization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I have overlooked the intention behind isOnComputeEngine()
being static. Let me see if I can find a way around this (either with keeping it static or moving it to a static clock).
I would also prefer to not change the behavior if possible.
Quality Gate failed for 'gapic-generator-java-root'Failed conditions |
Quality Gate failed for 'java_showcase_integration_tests'Failed conditions |
* @return if DirectPath is enabled for the client AND if the configurations are valid | ||
*/ | ||
@InternalApi | ||
public boolean canUseDirectPath() { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
Lines 373 to 376 in 16aec03
if (isDirectPathEnabled() | |
&& isCredentialDirectPathCompatible() | |
&& isOnComputeEngine() | |
&& canUseDirectPathWithUniverseDomain()) { |
There was a problem hiding this comment.
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()
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lqiu96 LGTM.
7e641ee
to
5896193
Compare
This seems to due to the added |
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; | ||
} | ||
} |
There was a problem hiding this comment.
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.
/** | ||
* 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; | ||
} |
There was a problem hiding this comment.
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
Based on #2818, I'll need to remove junit-pioneer and add mocks for EnvProvider. |
* <p>If productName is null, that represents the result of an IOException | ||
*/ | ||
@VisibleForTesting | ||
InstantiatingGrpcChannelProvider(Builder builder, String productName) { |
There was a problem hiding this comment.
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.
.readFirstLine(); | ||
} catch (IOException e) { | ||
// Keep existing behavior the same (null means it is not on compute engine) | ||
systemProductName = null; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Updating.
* @return if DirectPath is enabled for the client AND if the configurations are valid | ||
*/ | ||
@InternalApi | ||
public boolean canUseDirectPath() { |
There was a problem hiding this comment.
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()
?
Quality Gate failed for 'gapic-generator-java-root'Failed conditions |
Quality Gate failed for 'java_showcase_integration_tests'Failed conditions |
…2614) Allow the MetricsTracerFactory to take in a second parameter (Map of attributes) that will be treated as client level attributes. These attributes will be added to every single MetricsTracer created throughout the lifecycle of the client. Was able to verify this behavior inside Cloud Monitoring: ![image](https://github.com/googleapis/sdk-platform-java/assets/6621793/832f485e-2834-4765-b381-9a15e2913412) Additional Attribute was recorded. Via: ``` InstantiatingGrpcChannelProvider channelProvider = InstantiatingGrpcChannelProvider.newBuilder().build(); Map<String, String> clientAttributesMapping = new HashMap<>(); clientAttributesMapping.put("directpath_enabled", String.valueOf(channelProvider.canUseDirectPath())); ... options .setApiTracerFactory(new MetricsTracerFactory(recorder, clientAttributesMapping)) .build(); ``` --------- Co-authored-by: Blake Li <blakeli@google.com>
🤖 I have created a release *beep* *boop* --- <details><summary>2.42.0</summary> ## [2.42.0](v2.41.0...v2.42.0) (2024-06-25) ### Features * Allow Adding Client Level Attributes to MetricsTracerFactory ([#2614](#2614)) ([f122c6f](f122c6f)) * gapic-generator-java to perform a no-op when no services are detected ([#2460](#2460)) ([c0b5646](c0b5646)) * Make Layout Parser generally available in V1 ([e508ae6](e508ae6)) * populate `.repo-metadata.json` from highest version ([#2890](#2890)) ([f587541](f587541)) * push SNAPSHOT versions of the hermetic build docker image ([#2888](#2888)) ([81df866](81df866)) ### Bug Fixes * **deps:** update the Java code generator (gapic-generator-java) to 1.2.3 ([e508ae6](e508ae6)) * Expose Gax meter name ([#2865](#2865)) ([6c5d6ce](6c5d6ce)) * Move the logic of getting systemProductName from static block to static method ([#2874](#2874)) ([536f1eb](536f1eb)) * Update default Otel Attribute from method_name to method ([#2833](#2833)) ([af10a9e](af10a9e)) ### Dependencies * update dependency com.google.auto.value:auto-value to v1.11.0 ([#2842](#2842)) ([dd27fdf](dd27fdf)) * update dependency com.google.auto.value:auto-value-annotations to v1.11.0 ([#2843](#2843)) ([bf8e67f](bf8e67f)) * update dependency com.google.cloud:grpc-gcp to v1.6.1 ([#2943](#2943)) ([9f16b40](9f16b40)) * update dependency org.checkerframework:checker-qual to v3.44.0 ([#2848](#2848)) ([7a99c50](7a99c50)) * update dependency org.easymock:easymock to v5.3.0 ([#2871](#2871)) ([c243f7d](c243f7d)) * update google api dependencies ([#2846](#2846)) ([b5ef698](b5ef698)) * update googleapis/java-cloud-bom digest to 17cc5ec ([#2882](#2882)) ([d6abd8e](d6abd8e)) * update netty dependencies to v4.1.111.final ([#2877](#2877)) ([b5f10b9](b5f10b9)) * update opentelemetry-java monorepo to v1.39.0 ([#2863](#2863)) ([9d1f3a8](9d1f3a8)) </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> Co-authored-by: Joe Wang <joewa@google.com>
Allow the MetricsTracerFactory to take in a second parameter (Map of attributes) that will be treated as client level attributes. These attributes will be added to every single MetricsTracer created throughout the lifecycle of the client.
Was able to verify this behavior inside Cloud Monitoring:
Additional Attribute was recorded.
Via: