Repository navigation
feat(storage): add http.client.request.duration metric for HTTP requests - #14744
nidhiii-27 wants to merge 11 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the StorageHttpMetricsInterceptor class to record HTTP client metrics, such as request duration, using OpenTelemetry. It integrates this interceptor into StorageOptions and HttpStorageRpc to decorate request initializers, and adds comprehensive unit tests. Feedback on the changes suggests initializing the recorded AtomicBoolean to true instead of false to prevent recording metrics for requests that fail before execution begins.
…on metrics [Generated-by: AI]
Merge updated feat/storage-rpc-call-duration base branch, use OpenTelemetryBootstrappingUtils.extractServerAddress, pass String status codes to KEY_HTTP_RESPONSE_STATUS_CODE, wire batchRequestInitializer on overflow batches, and refine StorageHttpMetricsInterceptor Javadoc. [Generated-by: AI]
…uestDuration to StorageMetricsContext [Generated-by: AI]
…Interceptor - Merge updated base branch feat/storage-rpc-call-duration (including #14694) - Update HttpIOExceptionHandler error type to StorageErrorTypeMapper.STATUS_CONNECTION_ERROR - Return "/storage/v1/b" in extractV1UrlTemplate when rest is "b" (CreateBucket / ListBuckets) [Generated-by: AI]
… mapping After merging StorageErrorTypeMapper updates from #14694, Status.DEADLINE_EXCEEDED maps to DEADLINE_EXCEEDED while SocketTimeoutException maps to TIMEOUT. Update recordAttemptError_specificExceptions_mappedToCanonicalTypes to assert 1 count each for DEADLINE_EXCEEDED, TIMEOUT, and DNS_FAILURE. [Generated-by: AI]
cpriti-os
left a comment
cpriti-os
left a comment
There was a problem hiding this comment.
Thanks for the fixes, a few follow-ups inline.
| if (existingResp != null) { | ||
| existingResp.interceptResponse(response); | ||
| } | ||
| if (recorded.compareAndSet(false, true)) { |
There was a problem hiding this comment.
This records when headers arrive, so download durations exclude the body transfer. Can we record when the body is fully read (e.g. on the read channel close), or note the limitation in the javadoc?
| // HttpIOExceptionHandler does not receive the thrown IOException instance from | ||
| // google-http-client, so transport-level IOExceptions default to STATUS_CONNECTION_ERROR here | ||
| // (specific subtypes like TIMEOUT, DNS_FAILURE, and TLS_FAILURE are mapped in Retrying). | ||
| request.setIOExceptionHandler( |
There was a problem hiding this comment.
HttpRequest.execute() only runs the response interceptor once after its internal retry loop, so a 401-refresh retry drops the first attempt. Can we also record from the unsuccessful-response handler when it returns true?
| statusCode >= 400 | ||
| ? StorageErrorTypeMapper.mapHttpResponseCode(statusCode) | ||
| : StorageErrorTypeMapper.STATUS_OK; | ||
| recordDuration(request, startTimeNanos.get(), String.valueOf(statusCode), errorType); |
There was a problem hiding this comment.
Question: should http.response.status_code be an integer label rather than a string? Go emits it as Int64 and the OTel HTTP convention is int; the key is defined in #14728 so it'd be a change there too.
There was a problem hiding this comment.
| if (url == null) { | ||
| return "/"; | ||
| } | ||
| if (url.getFirst("upload_id") != null) { |
There was a problem hiding this comment.
Is the upload_id check needed? The /upload/storage/v1/b/ branch already handles JSON resumable PUTs, and this mislabels XML signed-URL chunks (/{bucket}/{object}?upload_id=).
| && segments.length >= 8 | ||
| && "b".equals(segments[5]) | ||
| && "o".equals(segments[7])) { | ||
| return "/storage/v1/b/{bucket}/o/{object}/" |
There was a problem hiding this comment.
Nit: Go and Python only use {bucket}/{object} placeholders (and {access_id}); can we match that so url.template values line up across SDKs?
| } | ||
|
|
||
| @Test | ||
| public void initialize_networkIOException_recordsErrorTypeWithoutStatusCode() throws IOException { |
There was a problem hiding this comment.
Can we add a test where the first attempt throws IOException and the retry succeeds, to cover two separate points?
…sCode for http.client.request.duration [Generated-by: AI]
Instruments HTTP/JSON storage requests to record
http.client.request.durationOpenTelemetry histogram metrics.StorageHttpMetricsInterceptorto measure per-request duration and attachrpc.system.name,http.request.method,http.response.status_code,network.path,url.template,server.address, anderror.typeattributes across normal responses andIOExceptionfailures.StorageHttpMetricsInterceptorviaStorageOptions.decorateWithMetricsandHttpStorageRpc.StorageHttpMetricsInterceptorTest.Stacked on #14743.