feat(bigquery): Add OpenTelemetry support to BQ rpcs #3860
Conversation
product-auto-label
bot
added
size: xl
google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/spi/v2/HttpBigQueryRpc.java
Show resolved
Hide resolved
| } | ||
| Table tableResponse = bqPatchRequest.execute(); | ||
| if (patchTable != null) { | ||
| patchTable.setAttribute("bq.rpc.response", tableResponse.toPrettyString()); |
There was a problem hiding this comment.
You guys mentioned before wanting to capture the response, but it is pretty verbose in the output. Is there anything specific from the responses that you think is most important?
|
A lot of useful data, such as jobId or location, could be captured here in the rpc layer, but it would just be doubling up the attributes that were already captured in the interface layer. What are your thoughts on keeping the rpc layer scoped to unique "rpc relevant" attributes vs capturing some values in both layers? |
google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/spi/v2/HttpBigQueryRpc.java
Show resolved
Hide resolved
google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/spi/v2/HttpBigQueryRpc.java
Outdated
Show resolved
Hide resolved
google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/spi/v2/HttpBigQueryRpc.java
Outdated
Show resolved
Hide resolved
| .setAttribute("bq.rpc.page_token", datasetsListRequest.getPageToken()) | ||
| .setAttribute("bq.rpc.access_token", datasetsListRequest.getAccessToken()) | ||
| .setAttribute("bq.rpc.oauth_token", datasetsListRequest.getOauthToken()) | ||
| .setAttribute("bq.rpc.request_method", datasetsListRequest.getRequestMethod()) |
There was a problem hiding this comment.
why is there both a method and request_method in here? That seems redundant.
There was a problem hiding this comment.
Agreed that it's not very useful, but request method refers to the http method used (GET, POST, PATCH, etc). You and Phong both mentioned this field, so I removed it.
| } | ||
| Table tableResponse = bqCreateRequest.execute(); | ||
| if (createTable != null) { | ||
| createTable.setAttribute("bq.rpc.response", tableResponse.toPrettyString()); |
There was a problem hiding this comment.
My main worry about logging the responses is going to be large responses. In general, should we enforce a configurable limit?
There was a problem hiding this comment.
There is a configurable limit for attribute sizes (default is unlimited), but this is toggled at the SDK level through an environment variable or system property. In my opinion, this is something that should be left up to the user in case they want to have giant attributes for their own tracing or metrics.
It's probably better to just filter for important fields in the response, like id and nextPageToken like Phong suggested
|
I've updated the response attributes to only contain the response id and the The oauth token, access token, and request_method attributes have all been removed. |
google-cloud-bigquery/src/main/java/com/google/cloud/bigquery/spi/v2/HttpBigQueryRpc.java
Outdated
Show resolved
Hide resolved
3 participants
No description provided.