gemini-code-assist[bot] commented on code in PR #38846:
URL: https://github.com/apache/beam/pull/38846#discussion_r3373382536
##########
sdks/java/extensions/opentelemetry-gcp-auth-extension/src/main/java/org/apache/beam/sdk/extensions/opentelemetry/gcp/auth/GcpAuthAutoConfigurationCustomizerProvider.java:
##########
@@ -96,150 +108,132 @@ public class GcpAuthAutoConfigurationCustomizerProvider
* enable GCP integration.
*
* @param autoConfiguration the AutoConfigurationCustomizer to customize.
+ * @throws GoogleAuthException if there's an error retrieving Google
Application Default
+ * Credentials.
+ * @throws io.opentelemetry.sdk.autoconfigure.spi.ConfigurationException if
required options are
+ * not configured through environment variables or system properties.
*/
@Override
public void customize(@Nonnull AutoConfigurationCustomizer
autoConfiguration) {
+ GoogleCredentials credentials;
+ try {
+ credentials = GoogleCredentials.getApplicationDefault();
+ } catch (IOException e) {
+ throw new GoogleAuthException(Reason.FAILED_ADC_RETRIEVAL, e);
+ }
autoConfiguration
- .addSpanExporterCustomizer(this::customizeSpanExporter)
- .addMetricExporterCustomizer(this::customizeMetricExporter)
- .addResourceCustomizer(this::customizeResource);
+ .addSpanExporterCustomizer(
+ (spanExporter, configProperties) ->
+ customizeSpanExporter(spanExporter, credentials,
configProperties))
+ .addMetricExporterCustomizer(
+ (metricExporter, configProperties) ->
+ customizeMetricExporter(metricExporter, credentials,
configProperties))
+ .addResourceCustomizer(
+ (resource, configProperties) ->
+ customizeResource(resource, credentials, configProperties));
}
Review Comment:
Eagerly loading `GoogleCredentials.getApplicationDefault()` inside the
`customize` method will throw a `GoogleAuthException` during OpenTelemetry SDK
initialization if GCP Application Default Credentials are not configured. This
happens even if the user has explicitly disabled GCP authentication (e.g., by
setting the signal type to `none`) or is not targeting GCP endpoints, because
this customizer is loaded via SPI on startup.
To fix this, we should defer the loading of credentials using a lazy
`Supplier` so that they are only retrieved when a GCP-targeted signal is
actually processed.
```java
@Override
public void customize(@Nonnull AutoConfigurationCustomizer
autoConfiguration) {
java.util.function.Supplier<GoogleCredentials> credentialsSupplier =
new java.util.function.Supplier<>() {
private GoogleCredentials credentials;
@Override
public synchronized GoogleCredentials get() {
if (credentials == null) {
try {
credentials = GoogleCredentials.getApplicationDefault();
} catch (IOException e) {
throw new GoogleAuthException(Reason.FAILED_ADC_RETRIEVAL,
e);
}
}
return credentials;
}
};
autoConfiguration
.addSpanExporterCustomizer(
(spanExporter, configProperties) ->
customizeSpanExporter(spanExporter, credentialsSupplier,
configProperties))
.addMetricExporterCustomizer(
(metricExporter, configProperties) ->
customizeMetricExporter(metricExporter, credentialsSupplier,
configProperties))
.addResourceCustomizer(
(resource, configProperties) ->
customizeResource(resource, credentialsSupplier,
configProperties));
}
```
##########
sdks/java/extensions/opentelemetry-gcp-auth-extension/src/main/java/org/apache/beam/sdk/extensions/opentelemetry/gcp/auth/GcpAuthAutoConfigurationCustomizerProvider.java:
##########
@@ -96,150 +108,132 @@ public class GcpAuthAutoConfigurationCustomizerProvider
* enable GCP integration.
*
* @param autoConfiguration the AutoConfigurationCustomizer to customize.
+ * @throws GoogleAuthException if there's an error retrieving Google
Application Default
+ * Credentials.
+ * @throws io.opentelemetry.sdk.autoconfigure.spi.ConfigurationException if
required options are
+ * not configured through environment variables or system properties.
*/
@Override
public void customize(@Nonnull AutoConfigurationCustomizer
autoConfiguration) {
+ GoogleCredentials credentials;
+ try {
+ credentials = GoogleCredentials.getApplicationDefault();
+ } catch (IOException e) {
+ throw new GoogleAuthException(Reason.FAILED_ADC_RETRIEVAL, e);
+ }
autoConfiguration
- .addSpanExporterCustomizer(this::customizeSpanExporter)
- .addMetricExporterCustomizer(this::customizeMetricExporter)
- .addResourceCustomizer(this::customizeResource);
+ .addSpanExporterCustomizer(
+ (spanExporter, configProperties) ->
+ customizeSpanExporter(spanExporter, credentials,
configProperties))
+ .addMetricExporterCustomizer(
+ (metricExporter, configProperties) ->
+ customizeMetricExporter(metricExporter, credentials,
configProperties))
+ .addResourceCustomizer(
+ (resource, configProperties) ->
+ customizeResource(resource, credentials, configProperties));
}
@Override
public int order() {
return Integer.MAX_VALUE - 1;
}
- private synchronized GoogleCredentials getCredentials() {
- if (credentials == null) {
- try {
- credentials = GoogleCredentials.getApplicationDefault();
- } catch (IOException e) {
- throw new GoogleAuthException(Reason.FAILED_ADC_RETRIEVAL, e);
- }
- }
- return credentials;
- }
-
- private SpanExporter customizeSpanExporter(
- SpanExporter exporter, ConfigProperties configProperties) {
+ private static SpanExporter customizeSpanExporter(
+ SpanExporter exporter, GoogleCredentials credentials, ConfigProperties
configProperties) {
if (isSignalTargeted(SIGNAL_TYPE_TRACES, configProperties)) {
- return addAuthorizationHeaders(exporter, configProperties);
+ return addAuthorizationHeaders(exporter, credentials, configProperties);
+ } else if (!hasSignal(SIGNAL_TYPE_NONE, configProperties)) {
+ String[] params = {SIGNAL_TYPE_TRACES,
SIGNAL_TARGET_WARNING_FIX_SUGGESTION};
+ logger.log(
+ Level.WARNING,
+ "GCP Authentication Extension is not configured for signal type:
{0}. {1}",
+ params);
}
return exporter;
}
Review Comment:
Update `customizeSpanExporter` to accept a lazy
`Supplier<GoogleCredentials>` instead of eagerly loaded `GoogleCredentials`.
```java
private static SpanExporter customizeSpanExporter(
SpanExporter exporter,
java.util.function.Supplier<GoogleCredentials> credentialsSupplier,
ConfigProperties configProperties) {
if (isSignalTargeted(SIGNAL_TYPE_TRACES, configProperties)) {
return addAuthorizationHeaders(exporter, credentialsSupplier.get(),
configProperties);
} else if (!hasSignal(SIGNAL_TYPE_NONE, configProperties)) {
String[] params = {SIGNAL_TYPE_TRACES,
SIGNAL_TARGET_WARNING_FIX_SUGGESTION};
logger.log(
Level.WARNING,
"GCP Authentication Extension is not configured for signal type:
{0}. {1}",
params);
}
return exporter;
}
```
##########
sdks/java/extensions/opentelemetry-gcp-auth-extension/src/main/java/org/apache/beam/sdk/extensions/opentelemetry/gcp/auth/GcpAuthAutoConfigurationCustomizerProvider.java:
##########
@@ -262,16 +256,13 @@ private Map<String, String>
getRequiredHeaderMap(ConfigProperties configProperti
}
// Updates the current resource with the attributes required for ingesting
OTLP data on GCP.
- private Resource customizeResource(Resource resource, ConfigProperties
configProperties) {
- if (!isAnySignalTargeted(configProperties)) {
- return resource;
- }
-
+ private static Resource customizeResource(
+ Resource resource, GoogleCredentials credentials, ConfigProperties
configProperties) {
String gcpProjectId;
try {
gcpProjectId =
ConfigurableOption.GOOGLE_CLOUD_PROJECT.getConfiguredValue(configProperties);
} catch (ConfigurationException e) {
- gcpProjectId = getCredentials().getProjectId();
+ gcpProjectId = credentials.getProjectId();
if (gcpProjectId == null || gcpProjectId.isEmpty()) {
throw e;
}
Review Comment:
In addition to accepting a lazy `Supplier<GoogleCredentials>`, we must
restore the check to verify if any signal is targeted for GCP authentication
before attempting to customize the resource. Without this check,
`customizeResource` will always run and throw a `ConfigurationException` if the
GCP project ID is missing, even if the user has explicitly disabled GCP
authentication (e.g., by setting the signal type to `none`).
```java
private static Resource customizeResource(
Resource resource,
java.util.function.Supplier<GoogleCredentials> credentialsSupplier,
ConfigProperties configProperties) {
if (!isSignalTargeted(SIGNAL_TYPE_TRACES, configProperties)
&& !isSignalTargeted(SIGNAL_TYPE_METRICS, configProperties)) {
return resource;
}
String gcpProjectId;
try {
gcpProjectId =
ConfigurableOption.GOOGLE_CLOUD_PROJECT.getConfiguredValue(configProperties);
} catch (ConfigurationException e) {
gcpProjectId = credentialsSupplier.get().getProjectId();
if (gcpProjectId == null || gcpProjectId.isEmpty()) {
throw e;
}
```
##########
sdks/java/extensions/opentelemetry-gcp-auth-extension/src/main/java/org/apache/beam/sdk/extensions/opentelemetry/gcp/auth/GcpAuthAutoConfigurationCustomizerProvider.java:
##########
@@ -96,150 +108,132 @@ public class GcpAuthAutoConfigurationCustomizerProvider
* enable GCP integration.
*
* @param autoConfiguration the AutoConfigurationCustomizer to customize.
+ * @throws GoogleAuthException if there's an error retrieving Google
Application Default
+ * Credentials.
+ * @throws io.opentelemetry.sdk.autoconfigure.spi.ConfigurationException if
required options are
+ * not configured through environment variables or system properties.
*/
@Override
public void customize(@Nonnull AutoConfigurationCustomizer
autoConfiguration) {
+ GoogleCredentials credentials;
+ try {
+ credentials = GoogleCredentials.getApplicationDefault();
+ } catch (IOException e) {
+ throw new GoogleAuthException(Reason.FAILED_ADC_RETRIEVAL, e);
+ }
autoConfiguration
- .addSpanExporterCustomizer(this::customizeSpanExporter)
- .addMetricExporterCustomizer(this::customizeMetricExporter)
- .addResourceCustomizer(this::customizeResource);
+ .addSpanExporterCustomizer(
+ (spanExporter, configProperties) ->
+ customizeSpanExporter(spanExporter, credentials,
configProperties))
+ .addMetricExporterCustomizer(
+ (metricExporter, configProperties) ->
+ customizeMetricExporter(metricExporter, credentials,
configProperties))
+ .addResourceCustomizer(
+ (resource, configProperties) ->
+ customizeResource(resource, credentials, configProperties));
}
@Override
public int order() {
return Integer.MAX_VALUE - 1;
}
- private synchronized GoogleCredentials getCredentials() {
- if (credentials == null) {
- try {
- credentials = GoogleCredentials.getApplicationDefault();
- } catch (IOException e) {
- throw new GoogleAuthException(Reason.FAILED_ADC_RETRIEVAL, e);
- }
- }
- return credentials;
- }
-
- private SpanExporter customizeSpanExporter(
- SpanExporter exporter, ConfigProperties configProperties) {
+ private static SpanExporter customizeSpanExporter(
+ SpanExporter exporter, GoogleCredentials credentials, ConfigProperties
configProperties) {
if (isSignalTargeted(SIGNAL_TYPE_TRACES, configProperties)) {
- return addAuthorizationHeaders(exporter, configProperties);
+ return addAuthorizationHeaders(exporter, credentials, configProperties);
+ } else if (!hasSignal(SIGNAL_TYPE_NONE, configProperties)) {
+ String[] params = {SIGNAL_TYPE_TRACES,
SIGNAL_TARGET_WARNING_FIX_SUGGESTION};
+ logger.log(
+ Level.WARNING,
+ "GCP Authentication Extension is not configured for signal type:
{0}. {1}",
+ params);
}
return exporter;
}
- private MetricExporter customizeMetricExporter(
- MetricExporter exporter, ConfigProperties configProperties) {
+ private static MetricExporter customizeMetricExporter(
+ MetricExporter exporter, GoogleCredentials credentials, ConfigProperties
configProperties) {
if (isSignalTargeted(SIGNAL_TYPE_METRICS, configProperties)) {
- return addAuthorizationHeaders(exporter, configProperties);
+ return addAuthorizationHeaders(exporter, credentials, configProperties);
+ } else if (!hasSignal(SIGNAL_TYPE_NONE, configProperties)) {
+ String[] params = {SIGNAL_TYPE_METRICS,
SIGNAL_TARGET_WARNING_FIX_SUGGESTION};
+ logger.log(
+ Level.WARNING,
+ "GCP Authentication Extension is not configured for signal type:
{0}. {1}",
+ params);
}
return exporter;
}
Review Comment:
Update `customizeMetricExporter` to accept a lazy
`Supplier<GoogleCredentials>` instead of eagerly loaded `GoogleCredentials`.
```suggestion
private static MetricExporter customizeMetricExporter(
MetricExporter exporter,
java.util.function.Supplier<GoogleCredentials> credentialsSupplier,
ConfigProperties configProperties) {
if (isSignalTargeted(SIGNAL_TYPE_METRICS, configProperties)) {
return addAuthorizationHeaders(exporter, credentialsSupplier.get(),
configProperties);
} else if (!hasSignal(SIGNAL_TYPE_NONE, configProperties)) {
String[] params = {SIGNAL_TYPE_METRICS,
SIGNAL_TARGET_WARNING_FIX_SUGGESTION};
logger.log(
Level.WARNING,
"GCP Authentication Extension is not configured for signal type:
{0}. {1}",
params);
}
return exporter;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
