Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/23777 )
Change subject: IMPALA-14370: [Patch 1 of 2] - OpenTelemetry Query Tracing Skips Queries with Leading Comments ...................................................................... Patch Set 20: (9 comments) http://gerrit.cloudera.org:8080/#/c/23777/20/be/src/observe/otel-flags-trace.cc File be/src/observe/otel-flags-trace.cc: http://gerrit.cloudera.org:8080/#/c/23777/20/be/src/observe/otel-flags-trace.cc@217 PS20, Line 217: "" unintentional remove? http://gerrit.cloudera.org:8080/#/c/23777/20/be/src/observe/otel-test.cc File be/src/observe/otel-test.cc: http://gerrit.cloudera.org:8080/#/c/23777/20/be/src/observe/otel-test.cc@a199 PS20, Line 199: I can't find where we address this. Probably we should add this to test_otel_trace.py? http://gerrit.cloudera.org:8080/#/c/23777/20/be/src/observe/span-manager.h File be/src/observe/span-manager.h: http://gerrit.cloudera.org:8080/#/c/23777/20/be/src/observe/span-manager.h@120 PS20, Line 120: std::move nit: don't need std::move() http://gerrit.cloudera.org:8080/#/c/23777/20/be/src/observe/span-manager.cc File be/src/observe/span-manager.cc: http://gerrit.cloudera.org:8080/#/c/23777/20/be/src/observe/span-manager.cc@598 PS20, Line 598: inline nit: I see "inline" is used in several methods like this. It probably won't work (get ignored) for long methods. http://gerrit.cloudera.org:8080/#/c/23777/20/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/23777/20/be/src/service/client-request-state.cc@727 PS20, Line 727: !IsCTAS() Just curious, is CTAS currently not tracked in OTel tracing? http://gerrit.cloudera.org:8080/#/c/23777/20/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/23777/20/be/src/service/impala-server.cc@1374 PS20, Line 1374: (*query_handle)->otel_span_manager()->StartChildSpanSubmitted(); : } : : bool is_external_req = external_exec_request != nullptr; : : if (is_external_req && external_exec_request->remote_submit_time) { : (*query_handle)->SetRemoteSubmitTime(external_exec_request->remote_submit_time); : } : : (*query_handle)->query_events()->MarkEvent("Query submitted"); : : if ((*query_handle)->otel_trace_query()) { : (*query_handle)->otel_span_manager()->EndChildSpanSubmitted(); It's unrelated to this patch. But it seems the SUBMITTED span doesn't track any meaningful work and is always ended shortly. Maybe we can consider removing this span or moving this end point to just above the call on RunFrontendPlanner(). http://gerrit.cloudera.org:8080/#/c/23777/20/be/src/testutil/rand-util.h File be/src/testutil/rand-util.h: http://gerrit.cloudera.org:8080/#/c/23777/20/be/src/testutil/rand-util.h@92 PS20, Line 92: ; nit: don't need ";" http://gerrit.cloudera.org:8080/#/c/23777/18/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/23777/18/fe/src/main/java/org/apache/impala/service/Frontend.java@3342 PS18, Line 3342: || parsedStmt.isInvalidateMetadata() > I cannot recall the exact reason, but it was something to do with refresh h Ack. It's the same behavior before this patch. Probably we can add this in IMPALA-14664. http://gerrit.cloudera.org:8080/#/c/23777/20/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/23777/20/fe/src/main/java/org/apache/impala/service/Frontend.java@3001 PS20, Line 3001: ParsedStatement parsedStmt; : : // Parse stmt and collect/load metadata to populate a stmt-local table cache : try { : parsedStmt = compilerFactory.createParsedStatement(queryCtx); : } catch (ImpalaException e) { : if (planCtx.queryTraced_ == null && BackendConfig.INSTANCE.isOtelTraceEnabled()) { : planCtx.queryTraced_ = Boolean.FALSE; : updateQueryOtelTracingInBE(queryCtx.getQuery_id(), false); : } : throw e; : } : : // Determine whether to enable OpenTelemetry tracing for the query. : if (planCtx.queryTraced_ == null) { : if (!BackendConfig.INSTANCE.isOtelTraceEnabled()) { : planCtx.queryTraced_ = Boolean.FALSE; : } else { : updateQueryOtelTracing(queryCtx.getQuery_id(), planCtx, parsedStmt); : } : } nit: can we wrap these into a method, e.g. parseAndTriggerOtelTracing()? The current doCreateExecRequest() method is a bad example that it has more than 300 lines.. -- To view, visit http://gerrit.cloudera.org:8080/23777 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1425b32006f81586bf75c2e4045d23bab91e1611 Gerrit-Change-Number: 23777 Gerrit-PatchSet: 20 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Thu, 26 Mar 2026 13:37:31 +0000 Gerrit-HasComments: Yes
