Repository navigation
feat(storage): add http.client.request.duration metric for HTTP requests - #14744
nidhiii-27 wants to merge 14 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.
…sCode for http.client.request.duration [Generated-by: AI]
…erceptor [Generated-by: AI]
…request-duration [Generated-by: AI]
| * | ||
| * <p>Note: {@code google-http-client} invokes {@link HttpResponseInterceptor#interceptResponse} | ||
| * inside {@link HttpRequest#execute()} when the HTTP response headers arrive, before streaming | ||
| * download bodies are consumed by the caller (matching the Go SDK's {@code |
There was a problem hiding this comment.
Go actually records this metric when the body hits EOF or is closed, not at headers, so this parenthetical is misleading. Can we drop the Go reference and just say download durations end at response headers and exclude body transfer, with a follow-up tracked for recording at body close?
| (req, response, supportsRetry) -> { | ||
| boolean handled = | ||
| existingUnsuccessfulHandler.handleResponse(req, response, supportsRetry); | ||
| if (handled && recorded.compareAndSet(false, true)) { |
There was a problem hiding this comment.
Can we take the end time before delegating to the existing handler here? Otherwise the 401 point includes the token refresh round trip.
| if ("iam".equals(segments[2]) && segments.length > 3) { | ||
| return "/storage/v1/b/{bucket}/iam/" + segments[3]; | ||
| } | ||
| return "/storage/v1/b/{bucket}/" + segments[2]; |
There was a problem hiding this comment.
My earlier comment was imprecise, sorry: only raw ids should not leak; Python keeps {entity} on acl paths and {notification} on notificationConfigs. Fine to restore those so get-one and list don't share a template, otherwise happy to leave as is.
| assertThat( | ||
| StorageHttpMetricsInterceptor.extractUrlTemplate( | ||
| new GenericUrl( | ||
| "https://storage.googleapis.com/storage/v1/b/b1/notifications/notif1"))) |
There was a problem hiding this comment.
The real path is notificationConfigs rather than notifications, can we use that so the test covers the actual template?
| } | ||
|
|
||
| @Test | ||
| public void initialize_404NotFound_recordsErrorType() throws IOException { |
There was a problem hiding this comment.
Worth adding a case where an existing handler returns false on a 404 so we check the point is still recorded exactly once?
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.