feat(csharp/src/Drivers/BigQuery): Enhanced tracing and large resultset improvements #3022

davidhcoe · 2025-06-26T00:59:43Z

Adds tracing for the BigQuery driver
Modifies the behavior for large result sets:
- Uses a default dataset ID for large result sets if one is not specified.
- The caller can specify their own dataset. If it does not exist, the driver will attempt to create it.

birschick-bq

Generally ...

You won't need the "Action" tag - it's automatically added as the activity name with the TraceActivity call.
Consider tag keys carefully to try to keep "semantic convention" recommendations.

csharp/src/Drivers/BigQuery/BigQueryConnection.cs

csharp/src/Drivers/BigQuery/BigQueryStatement.cs

csharp/src/Drivers/BigQuery/RetryManager.cs

csharp/src/Drivers/BigQuery/BigQueryStatement.cs

birschick-bq · 2025-06-30T23:34:43Z

csharp/src/Drivers/BigQuery/RetryManager.cs

                catch (Exception ex)
                {
+                    activity?.AddBigQueryTag("retry_attempt", retryCount);
+                    activity?.AddException(ex);


Exactly right. A handled exception should still be traced.

CurtHagenlocher

Thanks! I've left some feedback and some questions.

CurtHagenlocher · 2025-07-03T19:50:07Z

csharp/test/Drivers/BigQuery/DriverTests.cs


                AdbcStatement statement = adbcConnection.CreateStatement();
-                statement.SqlQuery = environment.Query;
+                statement.SqlQuery = "select * from mashuptest-154002.NWIND.AdbcAllTypes"; // environment.Query;


Should this be reverted?

Reverted in recent PR

CurtHagenlocher · 2025-07-03T19:52:23Z

csharp/src/Drivers/BigQuery/BigQueryParameters.cs

        // default value per https://pkg.go.dev/cloud.google.com/go/bigquery#section-readme
        public const string DetectProjectId = "*detect-project-id*";
+
+        // What ODBC uses


Consider amending the comment to explain why we would use the same value as ODBC (versus e.g. _bqadbc_temp_tables).

I updated the comment, but I wonder if using _bqadbc_temp_tables might actually be better? On the one hand, assuming the dataset had already been created, it would just reuse that. On the other hand, having a specific one dedicated to ADBC could be useful in some scenarios if people want to understand the usage patterns.

CurtHagenlocher · 2025-07-03T19:54:10Z

csharp/src/Drivers/BigQuery/RetryManager.cs

                        {
+                            activity?.AddBigQueryTag("update_token.status", "Required");
                            await tokenProtectedResource.UpdateToken();
+                            activity?.AddBigQueryTag("update_token.status", "Completed");


What's the behavior when adding the same tag twice? It keeps the most recent one or it keeps all of them?

For AddTag, it will keep all of them. For SetTag, it will override and keep the latest.

CurtHagenlocher · 2025-07-03T19:55:45Z

csharp/src/Drivers/BigQuery/BigQueryUtils.cs

+
+        internal static string GetAssemblyVersion(Type type) => FileVersionInfo.GetVersionInfo(type.Assembly.Location).ProductVersion ?? string.Empty;
+
+        internal static bool TracingToFile() { return false; }


Consider making a property instead of a method.

That is a method because I thought I would have to put some logic in there to determine whether a file listener was active, but I could be misunderstanding how that part will work.

I changed the name and added a TODO in there for now.

CurtHagenlocher · 2025-07-03T19:59:10Z

csharp/src/Drivers/BigQuery/BigQueryConnection.cs

        const string infoVendorName = "BigQuery";
-        const string infoDriverArrowVersion = "19.0.0";
-        const string publicProjectId = "bigquery-public-data";
+        readonly string? infoDriverArrowVersion;


Why is this an instance field while the next two are static fields?

fixed in PR

CurtHagenlocher · 2025-07-03T20:04:24Z

csharp/src/Drivers/BigQuery/BigQueryConnection.cs

            {
-                UpdateClientToken();
-            }
+                activity?.AddTag(key + ".set", value);


It seems like a bad idea to log an access token under any circumstances. Are there other sensitive option values we should also avoid logging (or tie to BigQueryUtils.TracingToFile, which looks like a proxy for "is safe to log sensitive information")?

I added an opt-in list in the BigQueryParameters to indicate which BigQueryParameter is safe to log no matter what the listener is.

csharp/src/Drivers/BigQuery/BigQueryConnection.cs

CurtHagenlocher · 2025-07-03T20:10:23Z

csharp/src/Drivers/BigQuery/BigQueryConnection.cs

+        readonly string? infoDriverArrowVersion;
+
+        private static readonly string s_assemblyName = BigQueryUtils.GetAssemblyName(typeof(BigQueryStatement));
+        private static readonly string s_assemblyVersion = BigQueryUtils.GetAssemblyVersion(typeof(BigQueryStatement));


Instead of capturing these three times in BigQueryConnection, BigQueryStatement and BigQueryStatement.MultiArrowReader, consider storing them in just BigQueryUtils and referencing them from there.

fixed in PR

CurtHagenlocher · 2025-07-03T20:32:30Z

csharp/src/Drivers/BigQuery/BigQueryStatement.cs

        {
            // Ideally we wouldn't need to indirect through a stream, but the necessary APIs in Arrow
            // are internal. (TODO: consider changing Arrow).
+            activity?.AddBigQueryTag("read_stream", streamName);


Is the stream name system-generated or can it expose an arbitrary user identifier?

I'll stop asking this question, but might apply to other things like the destination table.

It's system generated in the form projects/{project_id}/locations/{location}/sessions/{session_id}/streams/{stream_id}. Do you not think that should be logged?

If the ids are GUIDs then that seems unlikely to be a problem. @birschick-bq, is there an OTel convention for marking possibly-sensitive data?

@CurtHagenlocher / @davidhcoe

It seems OTel
has described the need to be careful, but the only concrete action seems to be to have filters/scrubbers on the OTel Collector via a processor.
https://opentelemetry.io/docs/security/handling-sensitive-data/

I believe we'll need to add overloads to scrub/anonymize potential PII data.

I ended up making this a conditional output to only use if wring to a file.

CurtHagenlocher · 2025-07-03T20:38:22Z

csharp/src/Drivers/BigQuery/BigQueryStatement.cs

-                {
-                    string destinationTable = keyValuePair.Value;
+                    case BigQueryParameters.AllowLargeResults:
+                        options.AllowLargeResults = true ? keyValuePair.Value.ToLower().Equals("true") : false;


There are two existing and one new instance of s.ToLower().Equals("true") in this file, which is strictly worse than s.Equals("true", StringComparer.OrdinalIgnoreCase).

CurtHagenlocher

Thanks, looks largely good! Let me know if you want to check-in as-is or adjust the ToLowers first.

CurtHagenlocher · 2025-07-07T15:58:07Z

csharp/src/Drivers/BigQuery/BigQueryParameters.cs

+
+        public static bool IsSafeToLog(string name)
+        {
+            if (safeToLog.Contains(name.ToLower()))


I think it's generally better to declare the HashSet as taking a case-insensitive comparer than to do ToLower as the latter may cause an allocation. (In this case it probably won't because the options are generally defined as lower-case; this is more about the pattern.)

Otherwise, this should probably be ToLowerInvariant() as I don't think we'd want a culture-specific conversion.

(I just noticed that there are at least three other ToLower() in this project that maybe should be ToLowerInvariant() -- though in the case of BigQueryConnection.DescToField I'm 1) not sure why we're changing the case of the column name and 2) we're calling ToLower twice on the same value instead of just once and storing it in a local.)

David Coe added 12 commits June 20, 2025 07:48

pin

369f875

pin after testing partitions

95ce323

add tag for created dataset

57cf1ab

change to projectId after it is detected

3390263

Merge https://github.com/davidhcoe/arrow-adbc into dev/bigquery-tracing

7b63a3b

add tracing; need to validate

bd37b5e

more tracing

1263e24

clarify permissions required

5e74730

add check if writing to file for potentially PII-sensitive data

1abe315

introduce AddConditionalTag

3132960

add more tracing statements

55c6ca5

add additional permission information

19e1970

birschick-bq reviewed Jun 30, 2025

View reviewed changes

David Coe added 2 commits June 30, 2025 19:19

rework tracing

fc6ec14

rework tracing 2

47e0c32

birschick-bq reviewed Jun 30, 2025

View reviewed changes

PR feedback

581a332

davidhcoe marked this pull request as ready for review July 1, 2025 04:32

davidhcoe requested a review from CurtHagenlocher as a code owner July 1, 2025 04:32

github-actions bot added this to the ADBC Libraries 19 milestone Jul 1, 2025

lidavidm modified the milestones: ADBC Libraries 19, ADBC Libraries 20 Jul 2, 2025

CurtHagenlocher requested changes Jul 3, 2025

View reviewed changes

David Coe added 3 commits July 5, 2025 18:18

PR feedback

e4edbfd

reorder logic

391eb2e

cleanup

6641e73

CurtHagenlocher approved these changes Jul 7, 2025

View reviewed changes

more feedback

6ca79a9

CurtHagenlocher approved these changes Jul 7, 2025

View reviewed changes

CurtHagenlocher merged commit 3e481a6 into apache:main Jul 7, 2025
6 checks passed

serramatutu mentioned this pull request Aug 12, 2025

Sync upstream dbt-labs/arrow-adbc#52

Closed

davidhcoe deleted the dev/bigquery-tracing branch October 24, 2025 10:14


		internal static string GetAssemblyVersion(Type type) => FileVersionInfo.GetVersionInfo(type.Assembly.Location).ProductVersion ?? string.Empty;

		internal static bool TracingToFile() { return false; }

feat(csharp/src/Drivers/BigQuery): Enhanced tracing and large resultset improvements #3022

feat(csharp/src/Drivers/BigQuery): Enhanced tracing and large resultset improvements #3022

Uh oh!

Conversation

davidhcoe commented Jun 26, 2025

Uh oh!

birschick-bq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CurtHagenlocher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CurtHagenlocher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects