Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/24008 )
Change subject: IMPALA-14332: Add X-Request-Id as HttpRequestId attribute on root OTel span ...................................................................... Patch Set 6: (11 comments) http://gerrit.cloudera.org:8080/#/c/24008/3/be/src/observe/otel-test.cc File be/src/observe/otel-test.cc: http://gerrit.cloudera.org:8080/#/c/24008/3/be/src/observe/otel-test.cc@477 PS3, Line 477: > Done Done http://gerrit.cloudera.org:8080/#/c/24008/1/be/src/observe/span-manager.cc File be/src/observe/span-manager.cc: http://gerrit.cloudera.org:8080/#/c/24008/1/be/src/observe/span-manager.cc@147 PS1, Line 147: > Done Done http://gerrit.cloudera.org:8080/#/c/24008/3/tests/custom_cluster/test_otel_trace.py File tests/custom_cluster/test_otel_trace.py: http://gerrit.cloudera.org:8080/#/c/24008/3/tests/custom_cluster/test_otel_trace.py@475 PS3, Line 475: TCLIService.TStatusCode.SUCCESS_WITH_INFO_STATUS) > Done Done http://gerrit.cloudera.org:8080/#/c/24008/3/tests/custom_cluster/test_otel_trace.py@477 PS3, Line 477: : # Run a GetColumns() operation. : get_cols_req = TCLIService.TGetColumnsReq( > Done Done http://gerrit.cloudera.org:8080/#/c/24008/3/tests/custom_cluster/test_otel_trace.py@481 PS3, Line 481: schemaName="functional", > Since these tests are asserting the values of attributes in the generated s Done http://gerrit.cloudera.org:8080/#/c/24008/6/tests/custom_cluster/test_otel_trace.py File tests/custom_cluster/test_otel_trace.py: http://gerrit.cloudera.org:8080/#/c/24008/6/tests/custom_cluster/test_otel_trace.py@532 PS6, Line 532: # Python implementation of process_request_id_for_attribute : def process_request_id_py(request_id): : if not request_id: : return "" : processed = request_id : last_hyphen = processed.rfind('-') : if last_hyphen != -1 and last_hyphen < len(processed) - 1: : suffix = processed[last_hyphen + 1:] : # UUID last segment is always 12 hex characters : if len(suffix) != 12 and suffix.isdigit(): : processed = processed[:last_hyphen] : return processed I like the thoroughness of this function, but it seems like more than is necessary for a test. I think a better approach would be to keep everything up until the last hyphen. The code would be simpler, and the tests may also surface other issues. http://gerrit.cloudera.org:8080/#/c/24008/3/tests/util/otel_trace.py File tests/util/otel_trace.py: http://gerrit.cloudera.org:8080/#/c/24008/3/tests/util/otel_trace.py@685 PS3, Line 685: > Done Done http://gerrit.cloudera.org:8080/#/c/24008/3/tests/util/otel_trace.py@705 PS3, Line 705: expected_attr_count = 10 if http_request_id is not None else 9 > Done Done http://gerrit.cloudera.org:8080/#/c/24008/6/tests/util/otel_trace.py File tests/util/otel_trace.py: http://gerrit.cloudera.org:8080/#/c/24008/6/tests/util/otel_trace.py@675 PS6, Line 675: if http_request_id is not None: This if statement needs an else assert "HttpRequestId" not in init_span.attributes to ensure that attribute was not set when it should not have been set. http://gerrit.cloudera.org:8080/#/c/24008/6/tests/util/otel_trace.py@680 PS6, Line 680: try: : UUID(actual_http_request_id) : except ValueError as e: : assert False, "HttpRequestId should be a valid UUID, got: {}. Error: {}".format( : actual_http_request_id, str(e)) Please move this validation up into the assert_trace function so that invalid UUIDs get caught as early as possible. http://gerrit.cloudera.org:8080/#/c/24008/6/tests/util/otel_trace.py@721 PS6, Line 721: if http_request_id is not None: This if statement needs an else assert "HttpRequestId" not in init_span.attributes to ensure that attribute was not set when it should not have been set. -- To view, visit http://gerrit.cloudera.org:8080/24008 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e14f5b503ff7379463332bae34c266afc395524 Gerrit-Change-Number: 24008 Gerrit-PatchSet: 6 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Comment-Date: Thu, 12 Mar 2026 20:39:38 +0000 Gerrit-HasComments: Yes
