feat(bigquery-jdbc): add opt-out controls - #14199
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces in-memory aggregation of telemetry events (connection attempts, statement executions, error metrics, and feature usages) in TelemetryBatcher before flushing, along with support for dynamically enabling or disabling telemetry via connection properties or environment variables. The feedback highlights a critical issue with configuring the global TelemetryManager singleton using connection-specific properties, which ignores settings for subsequent connections. Additionally, the reviewer recommends replacing a large switch statement in toStatementType with dynamic enum mapping, copying explicit_bounds when merging duration histograms to prevent invalid payloads, and explicitly handling positive values for the GOOGLE_CLOUD_TELEMETRY_ENABLED environment variable.
| // 1. Connection Properties (lowest precedence) | ||
| if (connectionProperties != null) { | ||
| String propValue = connectionProperties.getProperty("EnableDiagnosticTelemetry"); | ||
| if (propValue == null) { |
There was a problem hiding this comment.
This if assigns exact same value
| propValue = connectionProperties.getProperty("enableDiagnosticTelemetry"); | ||
| } | ||
| if (propValue != null) { | ||
| if ("0".equals(propValue) || "false".equalsIgnoreCase(propValue)) { |
There was a problem hiding this comment.
We have BigQueryJdbcUrlUtility.convertIntToBoolean(), can we reuse it? (or move it to some utils class to use in both places)
| } | ||
| } | ||
|
|
||
| String sysInterval = System.getProperty("GOOGLE_CLOUD_TELEMETRY_UPLOAD_INTERVAL"); |
There was a problem hiding this comment.
nit: we have 2 types here, int & bool. I'd suggest moving it to helper methods to have smth like this
this.uploadIntervalMs = parseInt(System.getProperty("GOOGLE_CLOUD_TELEMETRY_UPLOAD_INTERVAL"), this.UploadIntervalMs);
(Second param for default value)
| if (localRef == null) { | ||
| TelemetryConfiguration config = TelemetryConfiguration.builder().build(); | ||
| TelemetryConfiguration config = | ||
| TelemetryConfiguration.builder().resolveProperties(properties).build(); |
There was a problem hiding this comment.
properties is always null at this point
| return AuthenticationType.AUTHENTICATION_TYPE_SERVICE_ACCOUNT; | ||
| case 1: | ||
| return AuthenticationType.AUTHENTICATION_TYPE_USER_AUTHENTICATION; | ||
| case 2: |
There was a problem hiding this comment.
Where is int coming from? It doesn't match OAuthType connection properties
2 participants
Telemetry Configuration & Resolution:
Telemetry Manager & Recording Engine:
Protobuf Schema Definitions: