[gcp-auth-extension]: Try resolving GCP_PROJECT from Google credentials if not provided#2109
[gcp-auth-extension]: Try resolving GCP_PROJECT from Google credentials if not provided#2109trask merged 1 commit intoopen-telemetry:mainfrom
Conversation
|
|
|
❌ The result from spotlessApply could not be committed to the PR branch, see logs: https://github.com/open-telemetry/opentelemetry-java-contrib/actions/runs/16990756319. |
|
❌ The result from spotlessApply could not be committed to the PR branch, see logs: https://github.com/open-telemetry/opentelemetry-java-contrib/actions/runs/16990824329. |
1503d82 to
bcafbf9
Compare
018394e to
be79f44
Compare
|
Any outstanding blocker about this one? |
...t/java/io/opentelemetry/contrib/gcp/auth/GcpAuthAutoConfigurationCustomizerProviderTest.java
Outdated
Show resolved
Hide resolved
c0929d4 to
46f24dd
Compare
Thanks for the PR @gustavovnicius! My only concern here is to take on the additional
I am engaging internally with the folks maintaining the auth library to see if this request can be resolved at the Java auth library level. |
|
Thanks for the thoughtful feedback, @psx95 totally agree that we should be cautious with new dependencies. I did a quick dependency analysis to verify the real impact of adding google-cloud-core. That’s ~8 small, standard Google client libraries already present in most GCP integrations, not a large footprint (roughly < 1 MB of total JARs), there's a big overlap with transient dependencies of I agree it would be ideal if Another thought is regarding downstream adoption: Without it, the extension in some situations is virtually useless, as the sheer amount of changes and effort to adopt it across the board, far outweighs the extension. |
|
@gustavovnicius, while the number of JARs being pulled is only 8, most of them are not being used (the auth library being the exception) and while the combined size of them is 1MB which might look small, it is extremely huge when you compare it to the size of this artifact's JAR itself which is roughly ~10KB. FWIW, I do agree that this is a fair ask and there is already a draft PR to expose the Project ID from the Auth Library. I think we should follow the progress of this draft PR and make changes in this extension once the feature is available. |
|
New version has been adopted and project id is attempted to be resolved based on credentials. @psx95 |
psx95
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Left a couple of suggestions that might be helpful.
.../main/java/io/opentelemetry/contrib/gcp/auth/GcpAuthAutoConfigurationCustomizerProvider.java
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/contrib/gcp/auth/GcpAuthAutoConfigurationCustomizerProvider.java
Outdated
Show resolved
Hide resolved
...t/java/io/opentelemetry/contrib/gcp/auth/GcpAuthAutoConfigurationCustomizerProviderTest.java
Outdated
Show resolved
Hide resolved
95a41e5 to
801f77e
Compare
|
Hey folks, anything left to do on this one? I'll gladly change it. If there's nothing left I'd really appreciate it if we could get it merged |
|
@psx95 ? |
psx95
left a comment
There was a problem hiding this comment.
Hi @gustavovnicius,
Apologies for the delay on this, and thank you for the changes, they look good to me now, just suggested a few changes in the test, mostly about adding a few edge cases and updating the documentation there.
We should be good to merge after this 👍🏻
Thanks again for your contribution and patience!
.../main/java/io/opentelemetry/contrib/gcp/auth/GcpAuthAutoConfigurationCustomizerProvider.java
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/contrib/gcp/auth/GcpAuthAutoConfigurationCustomizerProvider.java
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/contrib/gcp/auth/GcpAuthAutoConfigurationCustomizerProvider.java
Outdated
Show resolved
Hide resolved
...t/java/io/opentelemetry/contrib/gcp/auth/GcpAuthAutoConfigurationCustomizerProviderTest.java
Show resolved
Hide resolved
...t/java/io/opentelemetry/contrib/gcp/auth/GcpAuthAutoConfigurationCustomizerProviderTest.java
Show resolved
Hide resolved
...t/java/io/opentelemetry/contrib/gcp/auth/GcpAuthAutoConfigurationCustomizerProviderTest.java
Show resolved
Hide resolved
...t/java/io/opentelemetry/contrib/gcp/auth/GcpAuthAutoConfigurationCustomizerProviderTest.java
Outdated
Show resolved
Hide resolved
7a35b08 to
bb8c18c
Compare
|
I believe I covered all your comments @psx95 |
psx95
left a comment
There was a problem hiding this comment.
LGTM!
Thank you for the contribution and your patience! 👍🏻
Description:
Enhancement - Try resolving GOOGLE_CLOUD_PROJECT value using google-cloud-sdk.
Having a mandatory
GOOCLE_CLOUD_PROJECTconfiguration is unnecessary, given the extension already depends on ADC context existing. It can try to infer the project from it.Still mandatory to have a project id, but this gives more flexibility in the usage of the extension.
Existing Issue(s):
#2102
Testing:
Refactored the project id resolver logic into a separate method, including the more comprehensive
ServiceOptionsresolver. Tested the method for both the current strategy and falling back toServiceOptions, also added a missing test for not being able to find the option and throwing an exception.Documentation:
GOOGLE_CLOUD_PROJECTconfiguration was moved into the optional config section, with a note that even though it's optional, the extension needs it either provided or being able to infer it.Outstanding items:
N/A