Arnab Karmakar 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 1: (6 comments) Thanks for the comments http://gerrit.cloudera.org:8080/#/c/24008/1/be/src/observe/otel.cc File be/src/observe/otel.cc: http://gerrit.cloudera.org:8080/#/c/24008/1/be/src/observe/otel.cc@391 PS1, Line 391: size_t last_hyphen = processed.rfind('-'); : if (last_hyphen != string::npos && last_hyphen < processed.length() - 1) { : // Check if everything after the last hyphen is a digit : bool all_digits = true; : for (size_t i = last_hyphen + 1; i < processed.length(); ++i) { : if (!isdigit(processed[i])) { : all_digits = false; : break; : } : } : // If all digits after the last hyphen, remove it (it's the iteration count) : if (all_digits) { : processed = processed.substr(0, last_hyphen); : } : } > There is a potential issue here. UUIDs are not guaranteed to have letters 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@36 PS1, Line 36: #include "observe/otel.h" > I don't want the span-manager.h/.cc files to have a dependency on the otel. Done http://gerrit.cloudera.org:8080/#/c/24008/1/be/src/observe/span-manager.cc@59 PS1, Line 59: static constexpr char const* ATTR_HTTP_REQUEST_ID = "HttpRequestId"; > This attribute also needs to be set on the Init span. Done http://gerrit.cloudera.org:8080/#/c/24008/1/be/src/observe/span-manager.cc@147 PS1, Line 147: // Store all string attributes outside the map to ensure string lifetime. > Was there a specific issue you ran into that necessitated this code change? Yes, I think the protobuf code does copy the string, BUT the copy happens inside the TimedSpan constructor when StartSpan is called. Between the time we create the map and when we call StartSpan, the temporaries are getting corrupted if we use a named variable for the map. I feel we CANNOT use a named variable for the map if it contains temporaries - it must be an inline temporary that lives for the entire full expression. In the original code: Temporaries created -> Map constructed -> Passed immediately to TimedSpan constructor -> Protobuf copies -> Temporaries destroyed With the new changes, where we need a named variable for conditionally adding HttpRequestId: OtelAttributesMap root_attributes = { /* temporaries */ }; // <- temporaries destroyed HERE if (!processed_request_id.empty()) { root_attributes[ATTR_HTTP_REQUEST_ID] = ...; // <- accessing map with corrupted string_views } root_ = make_shared<TimedSpan>(..., std::move(root_attributes), ...); // <- protobuf copies from dangling pointers http://gerrit.cloudera.org:8080/#/c/24008/1/tests/custom_cluster/test_otel_trace.py File tests/custom_cluster/test_otel_trace.py: http://gerrit.cloudera.org:8080/#/c/24008/1/tests/custom_cluster/test_otel_trace.py@448 PS1, Line 448: def test_http_request_id_attribute(self): > Most of the code in this class could be replaced by `calling self.execute_q Ive simplified the tests but I think ImpalaHS2Client is necessary because the built-in hs2_http_client doesn't support http_tracing=True http://gerrit.cloudera.org:8080/#/c/24008/1/tests/custom_cluster/test_otel_trace.py@496 PS1, Line 496: : http_request_id = trace.root_span.attributes["HttpRequestId"].value : : # Verify UUID format (8-4-4-4-12 hex digits with hyphens, 36 chars total) : assert len(http_request_id) == 36, \ : "HttpRequestId should be 36 characters, got: {}".format(len(http_request_id)) : : parts = http_request_id.split('-') : expected_parts_lengths = [8, 4, 4, 4, 12] : assert len(parts) == len(expected_parts_lengths), \ : "HttpRequestId should have {} parts, got: {}".format( : len(expected_parts_lengths), len(parts)) : : for i, expected_len in enumerate(expected_parts_lengths): : assert len(parts[i]) == expected_len, \ : "HttpRequestId part {} should have length {}, got: {}".format( : i, expected_len, len(parts[i])) : assert all(c in '0123456789abcdefABCDEF' for c in parts[i]), \ : "HttpRequestId part {} should contain only hex digits".format(i) > Please investigate if the uuid library's `UUID` function can do this valida Done -- 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: 1 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: Mon, 09 Mar 2026 17:43:15 +0000 Gerrit-HasComments: Yes
