feat(bigquery): Integrate Otel in client lib

[whuffman36]

Adds OpenTelemetry tracing into the BigQueryImpl class, enabling in the following methods:

• (Dataset, Table, Job, Routine) Create
• (Dataset, Table, Job, Routine, Model, IamPolicy) Get
• (Dataset, Table, Job, Routine, Model, IamPolicy) Update
• (Dataset, Table, Routine, Model) Delete
• List (Datasets, Tables, Jobs, Routines, Models)
• Job Cancel
• insertAll
• testIamMethods
• queryRpc
• getQueryResults

This is one PR of several to fully integrate OTel into the client library, broken up into chunks to make review easier.

[PhongChuong]

In the case you are not aware. There are 2 additional locations where we perform network calls in TableDataWriteChannel.java and Job.java.

[whuffman36]

In the case you are not aware. There are 2 additional locations where we perform network calls in TableDataWriteChannel.java and Job.java.

Yes, I wanted to keep this PR scoped to just the BigQueryImpl file (though with the refactor that has changed). I was planning to include the calls in TableDataWriteChannel.java and Job.java in a separate PR with a few other methods, such as runWithRetries to make it easier to review and reason about.

[shollyman]

My biggest worry with this is the attribute structure here. This PR appears to define every field of every of every API as it's own attribute, which means we're more likely to introduce conflicts for users trying to filter based on attributes downstream.

This leads me to two questions:

1. should we be putting the full contents of the responses into attributes, vs a more constrained approach (critical fields like identifiers, status, etc)?
2. if we do want to capture the API response, should it just be stringified as a single attribute?

[whuffman36]

My biggest worry with this is the attribute structure here. This PR appears to define every field of every of every API as it's own attribute, which means we're more likely to introduce conflicts for users trying to filter based on attributes downstream.

This leads me to two questions:

1. should we be putting the full contents of the responses into attributes, vs a more constrained approach (critical fields like identifiers, status, etc)?
2. if we do want to capture the API response, should it just be stringified as a single attribute?

1. I'm of the opinion that since tracing is opt-in and meant to make debugging easier, the more visibility into the data the better. It's better to have visibility into the full contents and not need it than it is to want that visibility and not have it. What do you think?
2. I'm not convinced we should be capturing the response, as it essentially just passed to the user. The user can decide to capture or log the response once received outside of the library code. This design was to capture just the inputs to the API calls. That being said, I think stringifying a response as a single attribute makes the attribute harder to parse and read when debugging, which can defeat the purpose of creating a better debug system.

[PhongChuong]

My 2 cents regarding attributes:
Recall that we have 2 layers for the span. The upper level interface (BigQueryImpl.java, Job.java, etc.) and the HttpBigQueryRpc level. I believe we should capture the request and response (not in this PR). Specifically, it should be captured as a request/response* attribute at the HttpBigQueryRpc level. For the interface level (this PR), we should capture any metadata information that would help with debugging any issue that may arise and we should capture it in a structured manner so it is visible. Specifically, attributes in the options are great as they are rarely captured elsewhere. Meanwhile, we can be a little more selective with datatype attributes (TableId vs all the fields in TableInfo). And if needed, then the request attribute can be used to deep dive.

On a side note, I think we should also clearly state that "Span names and attributes are subject to change without notice" to allow us the flexibility to change the structure as needed.

• For certain responses such as table row data, we probably won't want to capture those values.

[whuffman36]

I've cleaned up the attributes that we are collecting by removing some fields that are not very useful in debugging. I also added in attributes for language and "db.name" to make the tracing more consistent with python. I've updated the span naming pattern to be more descriptive.

I think it's a good idea to include "Span names and attributes are subject to change without notice", but that seems like it would better to put in the docs rather than the code. What do you guys think?

[PhongChuong]

Thanks for all the changes.
@shollyman , can you take a look at the attributes to see if we missed anything?

[shollyman]

I continue to be wary of replicating many of the API response fields in the span attributes, favoring the "less is more" approach, but I can be convinced otherwise.

[whuffman36]

I've gone through and removed most attributes that we were collecting before, keeping mainly just the OP identifiers and time information such as creationTime, lastModifiedTime, etc. Let me know if you think we should shave off any more

[shollyman]

Did some more reading, two other considerations for these attributes (which may require a lot of callsite touches):

• Should the attributes be namespaced so it's clear they're fields from the BQ API? Right now they're just single term attributes and could conflict with other telemetry.

• I believe otel attribute conventions are snake case rather than camel, so we may want to revisit the multi word attributes.

[shollyman]

status is a submessage. Should we just report status fields individually? status.state (pending/running/done), and error presence in the job?

[whuffman36]

Sounds good to me, though I've decided to move those attributes to the Job class instead since during the create() function, job.status() is not yet populated. The field would always be set to null and thus be practically useless for debugging.

[shollyman]

minor: worth pulling the current page token into an attribute alongside the project for the list RPCs? It'll be visible in the HTTP request below it as part of the URL.

[whuffman36]

If the page token is supplied as a DatasetListOption, it will already be captured in an attribute here. I agree that adding the page token as an attribute is a good idea, though I think it would be better suited to the http layer rather than the interface.

[shollyman]

Do we want BQ schema in the otel attributes at all?

[whuffman36]

It is likely overkill, I went ahead and removed the schema from the attributes.

[whuffman36]

I renamed all the attributes to be snake case and gave them namespace prefixes to remove any overloading of attributes. I went with the namespace "bq" and created further namespaces depending on the objects, such as "bq.dataset", "bq.table", "bq.query", etc.

Let me know if you think a different namespace style would be better.

[shollyman]

Thanks for slogging through all this!