feat(bigquery): add HTTP response attribute tracing#12109
Conversation
ldetmer
changed the title
|
|
||
| static void addRequestBodySizeToSpan(HttpRequest request, Span span) { | ||
| try { | ||
| span.setAttribute(HTTP_REQUEST_BODY_SIZE, request.getContent().getLength()); |
There was a problem hiding this comment.
I think some NullPointerExceptions may be expected, as some request.getContent() is expected to be null (i.e. requests without a body). Would it be better to add an explicit null check, so that only real exceptions are caught?
| AttributeKey.longKey("http.response.status_code"); | ||
| public static final AttributeKey<Long> HTTP_REQUEST_RESEND_COUNT = | ||
| AttributeKey.longKey("http.request.resend_count"); | ||
| public static final AttributeKey<Long> HTTP_REQUEST_BODY_SIZE = |
There was a problem hiding this comment.
I noticed HTTP_REQUEST_BODY_SIZE is not populated in this PR. Is that WAI?
There was a problem hiding this comment.
it is populated see, but note:
- we only add it from the response as its empty until execution
- it will only show up in POST requests
| HttpResponseInterceptor originalInterceptor = request.getResponseInterceptor(); | ||
| request.setResponseInterceptor( | ||
| response -> { | ||
| addCommonResponseAttributesToSpan(request, response, span); |
There was a problem hiding this comment.
A general questions regarding the interceptors. Do we know if they runs on the same thread or a different thread? If it is a different thread, there might be a chance that the Span is already ended.
There was a problem hiding this comment.
It runs on the same thread. The client uses a syncronous execute call from apiary. So it will always get intercepted, and processed before it returns to the client.
|
|
||
| static void addRequestBodySizeToSpan(HttpRequest request, Span span) { | ||
| try { | ||
| if (request.getContent() != null) { |
There was a problem hiding this comment.
qq, if there is no content, I assume that would be when we are not sending a request body? For those case, would it be preferred to not record anything or set the body size to 0?
I'm guessing BigQuery probably doesn't really have any of those APIs where there is no body (wondering as I'm assuming this logic will probably be migrated to Gax)
There was a problem hiding this comment.
thanks for checking. I confirmed that we should omit in the case of a GET request (where content is null), encoded content (where we can't get accurate information on the actual size sent as that happens in the the transport layer) and in case of an error.
I was missing the use case for encoded content, so added that check!
lqiu96
left a comment
lqiu96
left a comment
There was a problem hiding this comment.
LGTM, but I'm not the most familiar with tracing so I'll defer approval to the others. logic to record the body and status code looks fine
| span.getAttributes().get(BigQueryTelemetryTracer.RPC_SYSTEM_NAME)); | ||
| assertEquals( | ||
| responseCode, | ||
| span.getAttributes().get(HttpTracingRequestInitializer.HTTP_RESPONSE_STATUS_CODE)); |
There was a problem hiding this comment.
Since this is existing behavior, we can do it as a separate PR. But we should consider test one behavior (attribute) at a time. See Test One Thing At A Time and Keep Tests Focused.
4 participants
This adds generic HTTP response attribute capture via open telemetry to BigQuery. Error attributes are not included in this PR.
example success POST
example failed GET