feat: Adding vendor and vendor information in header #1963
Conversation
ddixit14
requested review from
meltsufin,
suztomo,
blakeli0 and
burkedavison
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/core/GaxProperties.java
Outdated
Show resolved
Hide resolved
|
|
||
| // append the vendor information to the java-version if vendor is present. | ||
| String vendor = System.getProperty("java.vendor"); | ||
| if (vendor != null && !vendor.isEmpty()) { |
There was a problem hiding this comment.
This can be simplified by Guava's Strings.isNullOrEmpty, similar to here
|
Do we need to append this info for graalvm as well? |
|
to-do : Manually confirm graalvm-behaviour (change to graalvm, print the result of gaxProperties getJavaVersion(). |
|
ci / build(8) except for gapic-generator-java (pull_request) Failing after 1m |
product-auto-label
bot
added
size: m
gax-java/gax/src/test/java/com/google/api/gax/core/GaxPropertiesTest.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/test/java/com/google/api/gax/core/GaxPropertiesTest.java
Outdated
Show resolved
Hide resolved
| return System.getProperty("java.version"); | ||
| /** | ||
| * Returns the current runtime version. For GraalVM the values in this method will be fetched at | ||
| * build time and the values should not differ from runtime (executable) |
There was a problem hiding this comment.
| * build time and the values should not differ from runtime (executable) | |
| * build time and the values should not differ from the runtime (executable). |
gax-java/gax/src/main/java/com/google/api/gax/core/GaxProperties.java
Outdated
Show resolved
Hide resolved
Handled graalvm case as well. |
gax-java/gax/src/test/java/com/google/api/gax/core/GaxPropertiesTest.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/core/GaxProperties.java
Outdated
Show resolved
Hide resolved
ddixit14
changed the title
gax-java/gax/src/test/java/com/google/api/gax/core/GaxPropertiesTest.java
Show resolved
Hide resolved
gax-java/gax/src/test/java/com/google/api/gax/core/GaxPropertiesTest.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| public void testGetJavaRuntimeInfo_spaces() { |
There was a problem hiding this comment.
Can you rename it back to the Java vendor name? For those that represent real vendor value, the name should have the vendor name.
There was a problem hiding this comment.
Is the purpose of this unit test to test real vendor name or test the functionality of the method?
There was a problem hiding this comment.
@ddixit14 The test name should answer Blake'a question.
There was a problem hiding this comment.
I have tried to capture both.
There was a problem hiding this comment.
I really think that unit tests should only care about the functionality/behavior of the method and test names should include the behavior being tested and expected outcomes. If we only care about the behavior, the scope is limited and each test is easy to understand. If we start to test real vendor names, what's the scope of it? Do we try to test all the available vendors? If not, why do we pick the selected vendors?
That being said, if you feel strongly about testing with real vendor names, please add comments explaining the why these real vendors are selected and differences between each vendor.
There was a problem hiding this comment.
I have separated the real world tests and functionality tests, and given appropriate names and comments. Can you please review them once, and if there's still any issue then we can continue the discussion?
There was a problem hiding this comment.
As for why we are using real vendor in testing is because this change is being done to capture the customer usage (which version/vendor/vendor.version the customer is using). If we make sure that we are able to capture real vendors information correctly, our usage data will be more accurate. As for the other functionality tests, I have changed the name and commented what is being tested. As for why these 3 real vendors are chosen. (1) graalvm - we support this and we have a specific dashboard for its usage, so for more accuracy. also because I changed its implementation (2) temurin - we officially support this.
There was a problem hiding this comment.
I also want real world values used in unit tests. First, it would detects real world bugs. Second, for future code readers, it works as a document of what kind of values the function deals with in real world.
|
Test case for this? the vendor is ->Amazon.com Inc. |
gax-java/gax/src/test/java/com/google/api/gax/core/GaxPropertiesTest.java
Outdated
Show resolved
Hide resolved
Added this back. |
gax-java/gax/src/test/java/com/google/api/gax/core/GaxPropertiesTest.java
Show resolved
Hide resolved
|
[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed! |
|
[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed! |
|
[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed! |
This PR adds java vendor information to the "gl-java" header value. Only when the information is present, add it to
javaRuntimeInformation.Local testing results for various combinations-
(1)
the vendor is -> Eclipse Adoptium
the vendor version is -> Temurin-11.0.19+7
the javaRuntimeInformation is -> 11.0.19__Eclipse-Adoptium__Temurin-11.0.19-7
(2)
the vendor is ->Amazon.com Inc.
the vendor version is ->Corretto-11.0.19.7.1
the javaRuntimeInformation is ->11.0.19__Amazon.com-Inc.__Corretto-11.0.19.7.1
(3)
the vendor is ->Eclipse Adoptium
the vendor version is ->Temurin-11.0.20.1+1
the javaRuntimeInformation is ->11.0.20.1__Eclipse-Adoptium__Temurin-11.0.20.1-1
(4) [Case when vendor version is not available]
the vendor is ->Oracle Corporation
the vendor version is ->null
the javaRuntimeInformation is ->20.0.1__Oracle-Corporation