Jason Fehr 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 m Thanks! Added to IMPALA-14371: https://issues.apache.org/jira/browse/IMPALA-14371?focusedCommentId=18017707&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-18017707 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? No, I have not. Opened IMPALA-14380 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? Originally, I wanted to check and see if a drop table/database statement recorded the number of rows deleted and didn't get back to check. Tested just now, and the drop DDLs do not record number of rows deleted. Thus, this could be modified. Will make a note to address it next patch. 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 con Good question. IIRC, I had issues with CTAS queries and needed to figure out a different place to start/stop admission control. Opened IMPALA-14381 to address this. -- 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 <jf...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Jason Fehr <jf...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: gaurav singh <gsi...@cloudera.com> Gerrit-Comment-Date: Tue, 02 Sep 2025 16:01:43 +0000 Gerrit-HasComments: Yes
