feat: add fallback support for request and response size by kotharironak · Pull Request #272 · hypertrace/hypertrace-ingester

kotharironak · 2021-10-21T08:07:17Z

Description

Currently, there are following three attributes consider for calculating response size

response_size
http.response.size
http.response_content_length

As part of this PR, we will be adding fallback to calculate using the content of http.response.body if above are abset.

The same will be applied to request size calculation.

request_size
http.request.size
http.request_content_length

So, the fallback will be the size of http.request.body attribute

Testing

Have added unit tests

Checklist:

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been merged and published in downstream modules

codecov · 2021-10-21T08:09:12Z

Codecov Report

Merging #272 (f8387fe) into main (896081c) will increase coverage by 0.24%.
The diff coverage is 94.04%.

@@             Coverage Diff              @@
##               main     #272      +/-   ##
============================================
+ Coverage     80.20%   80.45%   +0.24%     
- Complexity     1181     1209      +28     
============================================
  Files           106      106              
  Lines          4588     4662      +74     
  Branches        424      436      +12     
============================================
+ Hits           3680     3751      +71     
- Misses          709      710       +1     
- Partials        199      201       +2

Flag	Coverage Δ
unit	`80.45% <94.04%> (+0.24%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ormalizer/fieldgenerators/GrpcFieldsGenerator.java	`96.96% <93.47%> (+0.12%)`	⬆️
...ormalizer/fieldgenerators/HttpFieldsGenerator.java	`93.92% <94.73%> (-0.02%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 896081c...f8387fe. Read the comment docs.

kotharironak · 2021-10-21T12:11:47Z

Also added support for tags,

http.request.header.content-length
http.response.header.content-length

avinashkolluru · 2021-10-25T08:43:35Z

-        String requestBody = attributeValueMap.get(RPC_REQUEST_BODY.getValue()).getValue();
-        return Optional.of(requestBody.length());
-      }
+    if (attributeValueMap.get(RawSpanConstants.getValue(ENVOY_REQUEST_SIZE)) != null) {


nit: There are repetitive RawSpanContants.getValue() calls. Can these be extracted out as static variables.

avinashkolluru · 2021-10-25T08:44:08Z


+  private static boolean isGrpcResponseBodyTruncated(
+      Map<String, AttributeValue> attributeValueMap) {
+    if (attributeValueMap.get(RawSpanConstants.getValue(GRPC_RESPONSE_BODY_TRUNCATED)) != null) {


Extract attributeValueMap.get(RawSpanConstants.getValue(GRPC_RESPONSE_BODY_TRUNCATED) out and reuse below

avinashkolluru · 2021-10-25T08:44:19Z

+  }
+
+  private static boolean isRpcResponseBodyTruncated(Map<String, AttributeValue> attributeValueMap) {
+    if (attributeValueMap.get(RPC_RESPONSE_BODY_TRUNCATED.getValue()) != null) {


Same comment as above

avinashkolluru · 2021-10-25T17:45:30Z

+    if (!requestSize.isEmpty()) return requestSize.map(Integer::parseInt);
+
+    if (SpanAttributeUtils.getBooleanAttribute(
+        event, RawSpanConstants.getValue(HTTP_REQUEST_BODY_TRUNCATED))) {


RawSpanConstants.getValue(HTTP_REQUEST_BODY_TRUNCATED can move to static variable

avinashkolluru · 2021-10-25T17:45:42Z

+    if (!responseSize.isEmpty()) return responseSize.map(Integer::parseInt);
+
+    if (SpanAttributeUtils.getBooleanAttribute(
+        event, RawSpanConstants.getValue(HTTP_RESPONSE_BODY_TRUNCATED))) {


move to static variable

avinashkolluru · 2021-10-25T17:46:37Z

  }

+  private static boolean isGrpcRequestBodyTruncated(Map<String, AttributeValue> attributeValueMap) {
+    if (attributeValueMap.get(GRPC_REQUEST_BODY_TRUNCATED_ATTR) != null) {


If the GRPC_REQUEST_BODY_TRUNCATED_ATTR attribute is present in the map, then you end up getting it twice - once for the null check and then for the Boolean parsing below. Please fix.

Most parts of the file were written in a similar manner so had followed the pattern. But, given this is a part of the ingestion pipeline, a small constant time boost compare to stack push/pop operation vs calc hashing twice would be helpful with respect to traffic. Changing this code, and created an issue for going over all the utilities for the same.
Issue for this : hypertrace/hypertrace#316

avinashkolluru · 2021-10-25T17:46:42Z

+  }
+
+  private static boolean isRpcRequestBodyTruncated(Map<String, AttributeValue> attributeValueMap) {
+    if (attributeValueMap.get(RPC_REQUEST_BODY_TRUNCATED_ATTR) != null) {


same as above

avinashkolluru · 2021-10-25T17:47:16Z

-        String requestBody = attributeValueMap.get(RPC_REQUEST_BODY.getValue()).getValue();
-        return Optional.of(requestBody.length());
-      }
+    if (attributeValueMap.get(ENVOY_REQUEST_SIZE_ATTR) != null) {


optimize the multiple get from the map here too

avinashkolluru · 2021-10-25T17:47:36Z

+    if (attributeValueMap.get(ENVOY_REQUEST_SIZE_ATTR) != null) {
+      return Optional.of(
+          Integer.parseInt(attributeValueMap.get(ENVOY_REQUEST_SIZE_ATTR).getValue()));
+    } else if (isRpcSystemGrpc(attributeValueMap)


optimize the multiple get from the map here too

avinashkolluru · 2021-10-25T17:47:52Z

+      return Optional.of(
+          Integer.parseInt(
+              attributeValueMap.get(RPC_REQUEST_METADATA_CONTENT_LENGTH_ATTR).getValue()));
+    } else if (attributeValueMap.get(GRPC_REQUEST_BODY_ATTR) != null


optimize the multiple get from the map here too

avinashkolluru · 2021-10-25T17:48:04Z

+      String requestBody = attributeValueMap.get(GRPC_REQUEST_BODY_ATTR).getValue();
+      return Optional.of(requestBody.length());
+    } else if (isRpcSystemGrpc(attributeValueMap)
+        && attributeValueMap.get(RPC_REQUEST_BODY_ATTR) != null


optimize the multiple get from the map here too

avinashkolluru · 2021-10-25T17:48:20Z


+  private static boolean isGrpcResponseBodyTruncated(
+      Map<String, AttributeValue> attributeValueMap) {
+    if (attributeValueMap.get(GRPC_RESPONSE_BODY_TRUNCATED_ATTR) != null) {


optimize the multiple get from the map here too

avinashkolluru · 2021-10-25T17:48:24Z

+  }
+
+  private static boolean isRpcResponseBodyTruncated(Map<String, AttributeValue> attributeValueMap) {
+    if (attributeValueMap.get(RPC_RESPONSE_BODY_TRUNCATED_ATTR) != null) {


optimize the multiple get from the map here too

kotharironak · 2021-10-26T07:14:41Z

@avinashkolluru Created the issue of multiple read from the map here for handling throughout file - #272 (comment), have the address for them for the exiting PR.

findingrish

LGTM

github-actions · 2021-10-26T10:29:48Z

Unit Test Results

  71 files ±0   71 suites ±0 1m 18s ⏱️ +9s
383 tests +2 383 ✔️ +2 0 💤 ±0 0 ❌ ±0

Results for commit 7147fec. ± Comparison against base commit 896081c.

feat: add fallback support for request and response size

5053c01

kotharironak requested review from a team, avinashkolluru and findingrish October 21, 2021 08:07