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 3: (12 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: TEST(OtelTest, ProcessRequestIdForAttribute) { This test belongs in otel-trace-manager-test.cc (patch https://gerrit.cloudera.org/c/24084/). 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: return otel_tls_enabled(); : } : : OtlpHttpExporterOptions get_http_exporter_config() { : return http_exporter_config(); : } : : BatchSpanProcessorOptions get_batch_processor_config() { : return batch_processor_config(); : } : : unique_ptr<SpanExporter> get_exporter() { : return init_exporter(); : } : > 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@36 PS1, Line 36: #include "observe/timed-span.h" > Done Done http://gerrit.cloudera.org:8080/#/c/24008/1/be/src/observe/span-manager.cc@59 PS1, Line 59: static constexpr char const* ATTR_QUERY_START_TIME = "QueryStartTime"; > Done Done http://gerrit.cloudera.org:8080/#/c/24008/1/be/src/observe/span-manager.cc@147 PS1, Line 147: string suffix = request_id.substr(last_hyphen + 1); > Yes, I think the protobuf code does copy the string, BUT the copy happens i Ah, I see now. In that case, it would be easier to call root_->SetAttribute() after 'root_' is initialized (if a http request id was specified in the header). 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: """Asserts that when X-Request-Id header is provided via hs2-http protocol, > Ive simplified the tests but I think ImpalaHS2Client is necessary because t I like this simplified approach especially how it automatically matches what Impala shell (and possibly Impyla) is doing. http://gerrit.cloudera.org:8080/#/c/24008/1/tests/custom_cluster/test_otel_trace.py@496 PS1, Line 496: new flags.""" : : @CustomClusterTestSuite.with_args( : impalad_args="-v=2 --cluster_id=select_queued --default_pool_max_requests=1 {}" : .format(TRACE_FLAGS), : cluster_size=1, tmp_dir_placeholders=[OUT_DIR], disable_log_buffering=True) : def test_select_queued(self): : """Asserts a query that is queued in admission control then completes successfully : generates the expected trace.""" : # Launch two queries, the second will be queued until the first completes. : query = "SELECT * FROM functional.alltypes WHERE id = 1" : handle1 = self.client.execute_async("{} AND int_col = SLEEP(5000)".format(query)) : self.client.wait_for_impala_state(handle1, RUNNING, 60) : query_id_1 = self.client.handle_id(handle1) : : handle2 = self.client.execute_async(query) : query_id_2 = self.client.handle_id(handle2) : : self.client.wait_for_impala_state(handle1, FINISHED, 60) > 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: trace = parse_trace_file(self.trace_file_path, query_id) To keep consistent, since all other tests in here use 'assert_trace' as the entrypoint into the functionality container in otel_trace.py, please avoid calling parse_trace_file() from this file. http://gerrit.cloudera.org:8080/#/c/24008/3/tests/custom_cluster/test_otel_trace.py@477 PS3, Line 477: assert "HttpRequestId" in trace.root_span.attributes, \ : "Root span should have HttpRequestId attribute. Available: {}".format( : list(trace.root_span.attributes.keys())) This assertion needs to be done in otel_trace.py since that is where all span attributes are asserted. http://gerrit.cloudera.org:8080/#/c/24008/3/tests/custom_cluster/test_otel_trace.py@481 PS3, Line 481: http_request_id = trace.root_span.attributes["HttpRequestId"].value Since these tests are asserting the values of attributes in the generated spans, the tests cannot use the generated span as the source of expected values. The expected http_request_id needs to come preferably from the client but could also come from reading the Impalad logs. 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: {} Nit: wrap with single quotes -- Expected: '{}', Actual: '{}' http://gerrit.cloudera.org:8080/#/c/24008/3/tests/util/otel_trace.py@705 PS3, Line 705: INITIALIZED, log_file_path, root_span_id) Need to also assert the value of the "HttpRequestId" attribute in the init span matches the value in the 'http_request_id' variable. -- 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: 3 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: Tue, 10 Mar 2026 22:15:35 +0000 Gerrit-HasComments: Yes
