Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/23279 )
Change subject: IMPALA-13237: [Patch 8] - OpenTelemetry Traces for DML/DDL Queries and Handle Leading Comments ...................................................................... Patch Set 26: (4 comments) http://gerrit.cloudera.org:8080/#/c/23279/26/be/src/observe/otel.cc File be/src/observe/otel.cc: http://gerrit.cloudera.org:8080/#/c/23279/26/be/src/observe/otel.cc@106 PS26, Line 106: static const regex query_newline( : "(select|alter|compute|create|delete|drop|insert|invalidate|update|with)\\s*" : "(\n|\\s*\\\\*\\/)", regex::icase | regex::optimize | regex::nosubs); : : // Lambda function to check if SQL starts with relevant keywords for tracing : static const function<bool(std::string_view)> is_traceable_sql = : [](std::string_view sql_str) -> bool { : return : LIKELY(boost::algorithm::istarts_with(sql_str, "select ") : || boost::algorithm::istarts_with(sql_str, "alter ") : || boost::algorithm::istarts_with(sql_str, "compute ") : || boost::algorithm::istarts_with(sql_str, "create ") : || boost::algorithm::istarts_with(sql_str, "delete ") : || boost::algorithm::istarts_with(sql_str, "drop ") : || boost::algorithm::istarts_with(sql_str, "insert ") : || boost::algorithm::istarts_with(sql_str, "invalidate ") : || boost::algorithm::istarts_with(sql_str, "update ") : || boost::algorithm::istarts_with(sql_str, "with ")) : || regex_search(sql_str.cbegin(), sql_str.cend(), query_newline); : }; This could potentially be optimized further, basically avoiding regex and multiple conversions and comparisons using boost (even though short circuited): - find the first keyword and convert to lowercase once - compare the keyword against a unordered_map of allowed keywords http://gerrit.cloudera.org:8080/#/c/23279/26/be/src/observe/span-manager.cc File be/src/observe/span-manager.cc: http://gerrit.cloudera.org:8080/#/c/23279/26/be/src/observe/span-manager.cc@294 PS26, Line 294: if (LIKELY(client_request_state_->summary_profile() != nullptr)) { : // The case of a query being cancelled while in the admission queue is handled here : // because the summary profile may not be updated by the time this code runs. : if (UNLIKELY(queued && cause != nullptr && cause->code() == TErrorCode::CANCELLED)) { : adm_result = AdmissionController::PROFILE_INFO_VAL_CANCELLED_IN_QUEUE; : } else{ : const string* profile_adm_res = client_request_state_->summary_profile()-> : GetInfoString("Admission result"); : if (UNLIKELY(profile_adm_res == nullptr)) { : // Handle the case where the query closes during admission control before the : // summary profile is updated with the admission result. : adm_result = ""; : } else { : adm_result = *profile_adm_res; : } : } : } Have we tested this with Single Admission Controller? http://gerrit.cloudera.org:8080/#/c/23279/26/be/src/observe/span-manager.cc@352 PS26, Line 352: } else { Should the else be just for TStmtType::DML? And maybe keep the original else case for others? http://gerrit.cloudera.org:8080/#/c/23279/26/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/23279/26/be/src/service/client-request-state.cc@662 PS26, Line 662: if (otel_trace_query() && !IsCTAS()) { Why are we skipping AC child span for CTAS? It does go though admission control. -- To view, visit http://gerrit.cloudera.org:8080/23279 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie9e83d7f761f3d629f067e0a0602224e42cd7184 Gerrit-Change-Number: 23279 Gerrit-PatchSet: 26 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Tue, 02 Sep 2025 14:14:26 +0000 Gerrit-HasComments: Yes
