[jira] [Assigned] (HDDS-16742) GrpcMetricsServerRequestInterceptor renders every request to protobuf text format to read one field

["Andrey Yarovoy (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/HDDS-16742?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Andrey Yarovoy reassigned HDDS-16742:
-------------------------------------

    Assignee: Andrey Yarovoy

> GrpcMetricsServerRequestInterceptor renders every request to protobuf text format to read one field
> ---------------------------------------------------------------------------------------------------
>
>                 Key: HDDS-16742
>                 URL: https://issues.apache.org/jira/browse/HDDS-16742
>             Project: Apache Ozone
>          Issue Type: Improvement
>          Components: OM
>            Reporter: Andrey Yarovoy
>            Assignee: Andrey Yarovoy
>            Priority: Major
>
> To populate the `LatestRequestType` tag, `GrpcMetricsServerRequestInterceptor.onMessage` does:
> {code}
> String[] messageFields = message.toString()
> .split(System.lineSeparator());
> // messageFields[0] should be in the format
> // cmdType: "type"
> String[] cmdTypeLine = messageFields[0].split(":");
> // Get only the type and remove any leading spaces
> grpcMetrics.setRequestType(cmdTypeLine[1].trim());
> {code}
> `AbstractMessage.toString()` is a full `TextFormat` render of the *entire* request. On this server the
> request is an `OMRequest`, so every call allocates a text rendering of the whole message — including
> nested key, ACL and list payloads — splits it into an array of lines, and then uses exactly one line
> before discarding all of it. For a large request this costs far more than everything else the metrics
> interceptors do combined.
> `cmdType` is field 1 of `OMRequest` and is directly readable. Keeping the interceptor generic over
> `ReqT` (and avoiding a `hadoop-hdds/framework` → `hadoop-ozone/interface-client` dependency), the
> descriptor is enough:
> {code}
> Descriptors.FieldDescriptor cmdType =
> parsedMessage.getDescriptorForType().findFieldByName("cmdType");
> if (cmdType != null && parsedMessage.hasField(cmdType)) {
> grpcMetrics.setRequestType(parsedMessage.getField(cmdType).toString());
> }
> {code}
> The current parsing is also fragile: it assumes `cmdType` is rendered first, that the value contains no
> `:`, and that `messageFields[0]` splits into at least two parts. A request whose first line is not
> `cmdType: …` yields `cmdTypeLine` of length 1 and throws `ArrayIndexOutOfBoundsException` from inside a
> server interceptor. Reading the field removes all three assumptions.
> Two related issues in the same area, in scope for this Jira since they concern the same field:
> - `GrpcMetrics.getMetrics()` is `synchronized` but `setRequestType()` is not, so the synchronization
> does not establish the happens-before it looks like it does for `requestType`.
> - `requestType` is a single server-wide field, so `LatestRequestType` is inherently whichever concurrent
> call wrote last. That may be acceptable for a tag literally named "latest", but it should be made
> deliberate — at minimum `volatile` — rather than left as an unsynchronized race.
> ### Testing
> Unit test on the interceptor with a mocked `ServerCall` / `ServerCallHandler`: drive `onMessage` with an
> `OMRequest` and assert `GrpcMetrics.getRequestType()` returns the `cmdType` value, including for a
> request whose other fields contain `:` and newlines, which the current string parsing gets wrong.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org