-
Notifications
You must be signed in to change notification settings - Fork 52
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(gax): add protobuf version tracking to headers #3199
base: main
Are you sure you want to change the base?
Conversation
ccdd15d
to
ea7e7ec
Compare
gax-java/gax/src/main/java/com/google/api/gax/core/GaxProperties.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiClientHeaderProvider.java
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/core/GaxProperties.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiClientHeaderProvider.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiClientHeaderProvider.java
Show resolved
Hide resolved
LGTM w/ addition of Showcase tests showing gRPC + httpjson clients are both providing the new headers. Looking for approval from @blakeli0 |
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 |
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 feel these static strings add additional complexity in reading the code, can we try to inline most of them per style guide?
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.
done
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)); |
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 we can inline BUNDLE_VERSION_KEY if it is not expected to be reused.
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.
done
@@ -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"; |
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.
Do you mind adding a comment regarding where do we get this magic string "Bundle-Version"?
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.
added it to the getBundleVersion JavaDoc
@@ -224,6 +247,10 @@ public Builder setApiVersionToken(String apiVersionToken) { | |||
return this; | |||
} | |||
|
|||
public String getProtobufRuntimeToken() { |
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 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?
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.
done
Matcher matcher = pattern.matcher(apiClientHeaderValue); | ||
if (matcher.find()) { | ||
return apiClientHeaderValue.substring(0, matcher.end()) | ||
+ protobufVersionAppendValue |
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 this field can be inlined too.
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.
done
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); |
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.
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>
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.
done
Thank you for the showcase tests - LGTM. |
Quality Gate passed for 'gapic-generator-java-root'Issues Measures |
Quality Gate passed for 'java_showcase_integration_tests'Issues Measures |
Adding |
Understood about DNM, but is this verbally LGTM? |
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)