feat(gax): add protobuf version tracking to headers #3199

ldetmer · 2024-09-13T15:48:51Z

Update the Java client libraries to report the runtime version of Protobuf as part of the existing x-goog-api-client request header.

Tested: java-cloud-library api (billing) and hand written api (storage)

gax-java/gax/src/main/java/com/google/api/gax/core/GaxProperties.java

gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiClientHeaderProvider.java

gax-java/gax/src/main/java/com/google/api/gax/core/GaxProperties.java

gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiClientHeaderProvider.java

burkedavison · 2024-09-16T20:03:07Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiClientHeaderProvider.java

@@ -41,8 +43,11 @@
 public class ApiClientHeaderProvider implements HeaderProvider, Serializable {


Could we have two tests added to Showcase that show the token being added in both grpc + httpjson clients?

added, but i notice that native-showcase is failing, would that be because the sdk-platform-java lib is not included in the image yet?

No - the native tests install the repo's modules first: https://github.com/googleapis/sdk-platform-java/blob/main/.github/workflows/ci.yaml#L272

Then starts the Showcase server, and finally runs the mvn test command: https://github.com/googleapis/sdk-platform-java/blob/main/.github/workflows/ci.yaml#L287

burkedavison · 2024-09-16T20:06:15Z

LGTM w/ addition of Showcase tests showing gRPC + httpjson clients are both providing the new headers.

Looking for approval from @blakeli0

blakeli0 · 2024-09-16T20:56:03Z

gax-java/gax/src/test/java/com/google/api/gax/rpc/ApiClientHeaderProviderTest.java

+  private static final String PROTOBUF_APPENDER = "--" + PROTOBUF_HEADER_VERSION_KEY + "-.*";
+  private static final String FULL_HEADER_MATCH =
+      "^gl-java/.* gapic/4\\.5\\.6"
+          + PROTOBUF_APPENDER


I feel these static strings add additional complexity in reading the code, can we try to inline most of them per style guide?

blakeli0 · 2024-09-16T21:03:16Z

gax-java/gax/src/main/java/com/google/api/gax/core/GaxProperties.java

+      File file = new File(clazz.getProtectionDomain().getCodeSource().getLocation().toURI());
+      try (JarFile jar = new JarFile(file.getPath())) {
+        Attributes attributes = jar.getManifest().getMainAttributes();
+        return Optional.ofNullable(attributes.getValue(BUNDLE_VERSION_KEY));


I think we can inline BUNDLE_VERSION_KEY if it is not expected to be reused.

blakeli0 · 2024-09-16T21:03:42Z

gax-java/gax/src/main/java/com/google/api/gax/core/GaxProperties.java

@@ -43,6 +49,9 @@ public class GaxProperties {
  private static final String DEFAULT_VERSION = "";
  private static final String GAX_VERSION = getLibraryVersion(GaxProperties.class, "version.gax");
  private static final String JAVA_VERSION = getRuntimeVersion();
+  private static final String PROTOBUF_VERSION =
+      getBundleVersion(Any.class).orElse(DEFAULT_VERSION);
+  static final String BUNDLE_VERSION_KEY = "Bundle-Version";


Do you mind adding a comment regarding where do we get this magic string "Bundle-Version"?

added it to the getBundleVersion JavaDoc

blakeli0 · 2024-09-16T21:13:19Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiClientHeaderProvider.java

@@ -224,6 +247,10 @@ public Builder setApiVersionToken(String apiVersionToken) {
      return this;
    }

+    public String getProtobufRuntimeToken() {


I know other fields have a public getter, but I don't think they have to be. Since this method is only used within Gax, can we change this to package private?

blakeli0 · 2024-09-16T21:16:43Z

gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiClientHeaderProvider.java

+    Matcher matcher = pattern.matcher(apiClientHeaderValue);
+    if (matcher.find()) {
+      return apiClientHeaderValue.substring(0, matcher.end())
+          + protobufVersionAppendValue


I think this field can be inlined too.

burkedavison · 2024-09-18T19:48:19Z

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITVersionHeaders.java

+        Pattern.compile("gl-java/.+ gapic/.+--protobuf-.+ gax/.+ rest/ protobuf/.*");
+    httpJsonClient.echo(EchoRequest.newBuilder().build());
+    ArrayList headerValues =
+        (ArrayList) httpJsonInterceptor.metadata.getHeaders().get(HTTP_CLIENT_API_HEADER_KEY);


Rather than using raw-types, use ArrayList<?> if you don't know the type. In this case, we're casting to String in the next line so we might as well cast to ArrayList<String>

burkedavison · 2024-09-18T21:09:21Z

Thank you for the showcase tests - LGTM.

sonarcloud · 2024-09-18T21:15:15Z

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

Issues
9 New issues
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 · 2024-09-18T21:18:38Z

Quality Gate passed for 'java_showcase_integration_tests'

Issues
14 New issues
0 Accepted issues

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

See analysis details on SonarCloud

ldetmer added 2 commits September 11, 2024 11:15

track protobuf version

31ebd95

add support for gccl

e6aea43

product-auto-label bot added the size: m Pull request size is medium. label Sep 13, 2024

ldetmer and others added 4 commits September 13, 2024 11:55

formatting fixes

a64e14b

Merge branch 'main' into protobuf-metric

3a7b27d

formatting fixes

0037a0a

feat(gax): add protobuf version to headers to track

ea7e7ec

ldetmer force-pushed the protobuf-metric branch from ccdd15d to ea7e7ec Compare September 13, 2024 18:20

ldetmer and others added 2 commits September 13, 2024 14:22

feat(gax): add protobuf version to headers to track

4e18aa6

Merge branch 'main' into protobuf-metric

b1a8960

ldetmer changed the title ~~Protobuf metric~~ feat(gax): add protobuf version to headers to track Sep 13, 2024

ldetmer added 5 commits September 14, 2024 19:57

feat(gax): add protobuf version to headers to track

8d6065c

feat(gax): add protobuf version to headers to track

2dbc3a4

feat(gax): add protobuf version to headers to track

b69684b

feat(gax): add protobuf version to headers to track

02051b3

feat(gax): add protobuf version to headers to track

31da0e5

ldetmer changed the title ~~feat(gax): add protobuf version to headers to track~~ feat(gax): add protobuf version tracking to headers Sep 16, 2024

ldetmer marked this pull request as ready for review September 16, 2024 13:39

ldetmer requested a review from burkedavison September 16, 2024 13:39

fixed todo wording

5a918f4

burkedavison reviewed Sep 16, 2024

View reviewed changes

ldetmer added 3 commits September 16, 2024 15:30

cleaned up append token logic

94d3f45

cleaned up append token logic

822ac0c

cleaned up append token logic

390c922

ldetmer requested a review from burkedavison September 16, 2024 19:41

fixed language

01eb41a

burkedavison reviewed Sep 16, 2024

View reviewed changes

blakeli0 reviewed Sep 16, 2024

View reviewed changes

added showcase tests + moved strings inline

92e901b

ldetmer and others added 3 commits September 18, 2024 14:59

formatting fix

a202b8b

Merge branch 'main' into protobuf-metric

f9fac2d

formatting fix

9849efd

burkedavison reviewed Sep 18, 2024

View reviewed changes

ldetmer added 4 commits September 18, 2024 15:55

print header to debug native image

604f00d

print header to debug native image

9c51ec7

fixed native showcase test

a99c4b1

fixed formatting

c7b3d2b

ldetmer requested review from blakeli0 and burkedavison September 18, 2024 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(gax): add protobuf version tracking to headers #3199

feat(gax): add protobuf version tracking to headers #3199

ldetmer commented Sep 13, 2024 •

edited

Loading

burkedavison Sep 16, 2024

ldetmer Sep 18, 2024 •

edited

Loading

burkedavison Sep 18, 2024

burkedavison commented Sep 16, 2024

blakeli0 Sep 16, 2024

ldetmer Sep 18, 2024

blakeli0 Sep 16, 2024

ldetmer Sep 18, 2024

blakeli0 Sep 16, 2024

ldetmer Sep 18, 2024

blakeli0 Sep 16, 2024

ldetmer Sep 18, 2024

blakeli0 Sep 16, 2024

ldetmer Sep 18, 2024

burkedavison Sep 18, 2024

ldetmer Sep 18, 2024

burkedavison commented Sep 18, 2024

sonarcloud bot commented Sep 18, 2024

sonarcloud bot commented Sep 18, 2024

		@@ -41,8 +43,11 @@
		public class ApiClientHeaderProvider implements HeaderProvider, Serializable {

feat(gax): add protobuf version tracking to headers #3199

Are you sure you want to change the base?

feat(gax): add protobuf version tracking to headers #3199

Conversation

ldetmer commented Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

ldetmer Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burkedavison commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burkedavison commented Sep 18, 2024

sonarcloud bot commented Sep 18, 2024

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

sonarcloud bot commented Sep 18, 2024

Quality Gate passed for 'java_showcase_integration_tests'

ldetmer commented Sep 13, 2024 •

edited

Loading

ldetmer Sep 18, 2024 •

edited

Loading