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
Merged

Conversation

ldetmer
Copy link
Contributor

@ldetmer ldetmer commented Oct 29, 2024

In order to maximize amount of times protobuf version is logged we have updated logic as follows:

  1. First try to use protbuf RuntimeVersion which is available in protobuf 4.x
  2. if not available try to read using manifest file
  3. if manifest file doesn't not exist then default to "3" as we know runtime < 4

Also updated showcase test to fail in the case protobuf version is missing

Tested:

  1. maven build using client library directly and overriding protobuf
  2. spring boot started project + google cloud storage client client lib, validated that protobuf header was sent

@product-auto-label product-auto-label bot added the size: xs Pull request size is extra small. label Oct 29, 2024
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: xs Pull request size is extra small. labels Nov 4, 2024
@ldetmer ldetmer changed the title update IT tests to ensure that a protobuf version is always getting sent fix: for protobuf version not always getting set Nov 4, 2024
@ldetmer ldetmer changed the title fix: for protobuf version not always getting set fix: protobuf version not always getting set Nov 5, 2024
@ldetmer ldetmer marked this pull request as ready for review November 5, 2024 00:27
@ldetmer ldetmer changed the title fix: protobuf version not always getting set fix: protobuf version not always getting set in headers Nov 5, 2024
@burkedavison
Copy link
Member

LGTM. Blake for approval.

// 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.

package com.google.api.gax.util;

/* Wrapper class for reflection {@link java.lang.Class} methods to enable unit testing. */
public class ClassWrapper {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it's worth it to introduce a new public class just for unit testing in Gax. Can we move the wrapper methods to GaxProperties and make them package private?

Copy link
Contributor Author

@ldetmer ldetmer Nov 6, 2024

Choose a reason for hiding this comment

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

i hope I understood what you meant. I moved the class to package private so I could continue to mock, or did you meant do not unit test this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored to remove the wrapper class/methods at all. Let me know what you think!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, it looks good to me on the surface!

did you meant do not unit test this code

We should definitely do unit tests if possible. I see that the sonar coverage is complaining about the coverage now, can you please take a look at it and see if it makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for calling out, I missed a test case! Fixed

@ldetmer ldetmer requested a review from blakeli0 November 7, 2024 14:53
Copy link

sonarcloud bot commented Nov 7, 2024

Copy link

sonarcloud bot commented Nov 7, 2024

Quality Gate Failed Quality Gate failed for 'java_showcase_integration_tests'

Failed conditions
50.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@ldetmer ldetmer merged commit 7f6e470 into main Nov 7, 2024
48 of 55 checks passed
@ldetmer ldetmer deleted the fix-showcase branch November 7, 2024 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants