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

fix: protobuf version not always getting set in headers #3322

Merged
merged 12 commits into from
Nov 7, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public class GaxProperties {
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);
getProtobufVersion(Any.class, "com.google.protobuf.RuntimeVersion");;

private GaxProperties() {}

Expand Down Expand Up @@ -148,4 +148,28 @@ static Optional<String> getBundleVersion(Class<?> clazz) {
return Optional.empty();
}
}

/**
* Returns the Protobuf runtime version as reported by com.google.protobuf.RuntimeVersion, if
* class is available, otherwise by reading from MANIFEST file. If niether option is available
* defaults to protobuf version 3 as RuntimeVersion class is available in protobuf version 4+
*/
@VisibleForTesting
static String getProtobufVersion(Class clazz, String protobufRuntimeVersionClassName) {
try {
Class<?> protobufRuntimeVersionClass = Class.forName(protobufRuntimeVersionClassName);
return protobufRuntimeVersionClass.getField("MAJOR").get(null)
+ "."
+ protobufRuntimeVersionClass.getField("MINOR").get(null)
+ "."
+ protobufRuntimeVersionClass.getField("PATCH").get(null);
} catch (ClassNotFoundException
| NoSuchFieldException
| IllegalAccessException
| SecurityException e) {
// If manifest file is not available default to protobuf generic version 3 as we know
// RuntimeVersion class is available in protobuf jar 4+.
return getBundleVersion(clazz).orElse("3");
Copy link
Collaborator

Choose a reason for hiding this comment

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

So we are going to have two different format of protobuf versions (3.x.x vs 3)? Is it going to cause issues when we try to aggregate the metrics?

Copy link
Member

Choose a reason for hiding this comment

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

There might be 3.1.2, which provides precision. But there shouldn't be any 3.x.x.

We shouldn't be providing precision (significant digits) when we don't know them. We can fix any parsing logic as necessary.

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ private static String checkAndAppendProtobufVersionIfNecessary(
// TODO(b/366417603): appending protobuf version to existing client library token until resolved
Pattern pattern = Pattern.compile("(gccl|gapic)\\S*");
Matcher matcher = pattern.matcher(apiClientHeaderValue);
if (matcher.find()) {
if (matcher.find() && GaxProperties.getProtobufVersion() != null) {
return apiClientHeaderValue.substring(0, matcher.end())
+ "--"
+ PROTOBUF_HEADER_VERSION_KEY
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[
{
"name": "com.google.protobuf.RuntimeVersion",
"fields" : [
{ "name" : "MAJOR" },
{ "name" : "MINOR" },
{ "name" : "PATCH" }
]
}
]
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import static org.junit.jupiter.api.Assertions.assertTrue;

import com.google.common.base.Strings;
import com.google.protobuf.Any;
import java.io.IOException;
import java.util.Optional;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -160,12 +161,8 @@ void testGetJavaRuntimeInfo_nullJavaVersion() {

@Test
public void testGetProtobufVersion() throws IOException {
Version version = readVersion(GaxProperties.getProtobufVersion());

assertTrue(version.major >= 3);
if (version.major == 3) {
assertTrue(version.minor >= 25);
}
assertTrue(
Pattern.compile("^\\d+\\.\\d+\\.\\d+").matcher(GaxProperties.getProtobufVersion()).find());
}

@Test
Expand All @@ -175,6 +172,34 @@ public void testGetBundleVersion_noManifestFile() throws IOException {
assertFalse(version.isPresent());
}

@Test
void testGetProtobufVersion_success() {
String version = GaxProperties.getProtobufVersion(Any.class, "com.google.protobuf.Any");

assertTrue(Pattern.compile("^\\d+\\.\\d+\\.\\d+").matcher(version).find());
}

@Test
void testGetProtobufVersion_classNotFoundException() throws Exception {
String version = GaxProperties.getProtobufVersion(Any.class, "foo.NonExistantClass");

assertTrue(Pattern.compile("^\\d+\\.\\d+\\.\\d+").matcher(version).find());
}

@Test
void testgetProtobufVersion_noSuchFieldException() throws Exception {
String version = GaxProperties.getProtobufVersion(Any.class, "java.lang.Class");

assertTrue(Pattern.compile("^\\d+\\.\\d+\\.\\d+").matcher(version).find());
}

@Test
void testGetProtobufVersion_noManifest() throws Exception {
String version = GaxProperties.getProtobufVersion(GaxProperties.class, "foo.NonExistantClass");

assertEquals("3", version);
}

private Version readVersion(String version) {
assertTrue(Pattern.compile("^\\d+\\.\\d+\\.\\d+").matcher(version).find());
String[] versionComponents = version.split("\\.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ void testHttpJsonCompliance_userApiVersionSetSuccess() throws IOException {
@Test
void testGrpcCall_sendsCorrectApiClientHeader() {
Pattern defautlGrpcHeaderPattern =
Pattern.compile("gl-java/.* gapic/.*?--protobuf-.* gax/.* grpc/.* protobuf/.*");
Pattern.compile("gl-java/.* gapic/.*?--protobuf-\\d.* gax/.* grpc/.* protobuf/\\d.*");
grpcClient.echo(EchoRequest.newBuilder().build());
String headerValue = grpcInterceptor.metadata.get(API_CLIENT_HEADER_KEY);
assertTrue(defautlGrpcHeaderPattern.matcher(headerValue).matches());
Expand All @@ -250,7 +250,7 @@ void testGrpcCall_sendsCorrectApiClientHeader() {
@Test
void testHttpJson_sendsCorrectApiClientHeader() {
Pattern defautlHttpHeaderPattern =
Pattern.compile("gl-java/.* gapic/.*?--protobuf-.* gax/.* rest/ protobuf/.*");
Pattern.compile("gl-java/.* gapic/.*?--protobuf-\\d.* gax/.* rest/ protobuf/\\d.*");
httpJsonClient.echo(EchoRequest.newBuilder().build());
ArrayList<String> headerValues =
(ArrayList<String>)
Expand Down
Loading