feat(bigquery): Integrate Otel into retries, jobs, and more #3842
Conversation
product-auto-label
bot
added
size: l
| @@ -476,7 +476,9 @@ private BigQueryResult queryRpc( | |||
| bigQueryOptions.getRetrySettings(), | |||
| BigQueryBaseService.BIGQUERY_EXCEPTION_HANDLER, | |||
| bigQueryOptions.getClock(), | |||
| retryConfig); | |||
| retryConfig, | |||
There was a problem hiding this comment.
Since we're not supporting open telemetry in the Connection interface. I think it might be better to pass in (false, null) instead of (bigQueryOptions.isOpenTelemetryTracingEnabled(), and bigQueryOptions.getOpenTelemetryTracer()), respectively, since the later would create spans in the child calls without the encapsulating parent span in ConnectionImpl.
We probably should also explicitly call out that open telemetry is not supported in the Connection Interface in the further sample + README.
There was a problem hiding this comment.
Fixed, making a note to explicitly state that otel is not supported in the connection interface for when I do the sample. Are you thinking of stating that in the root README or the samples/README file?
There was a problem hiding this comment.
I think stating it in:
- Root README
- As a comment in the open telemetry sample.
Would be sufficient.
| runWithRetries = | ||
| openTelemetryTracer | ||
| .spanBuilder("com.google.cloud.bigquery.BigQueryRetryHelper.runWithRetries") | ||
| .setAttribute("bq.retry.retry_settings", retrySettings.toString()) |
There was a problem hiding this comment.
I'm a little iffy on this attribute, but I could be convinced. It seems like retrySettings comes from gax rather than this library, and may communicate settings outsite the scope of our particular usages.
With this work users will see the runWithRetries as a single parent span containing the child attempts? That should provide insight into when retries occurred and their individual durations (modulo all the things we do inside runWithRetries being instruments).
There was a problem hiding this comment.
Correct, the structure itself should be sufficient - I removed the attribute. How do you feel about the retrySettings attributes in waitForJob?
| @@ -476,7 +476,9 @@ private BigQueryResult queryRpc( | |||
| bigQueryOptions.getRetrySettings(), | |||
| BigQueryBaseService.BIGQUERY_EXCEPTION_HANDLER, | |||
| bigQueryOptions.getClock(), | |||
| retryConfig); | |||
| retryConfig, | |||
There was a problem hiding this comment.
I think stating it in:
- Root README
- As a comment in the open telemetry sample.
Would be sufficient.
The more here refers to
TableDataWriteChannel. This PR also changes the signature ofBigQueryRetryHelper.runWithRetries()to include abooleanand OtelTraceras parameters.