Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/22924 )
Change subject: IMPALA-13237: [Patch 5] - Implement OpenTelemetry Traces for Select Queries Tracking ...................................................................... Patch Set 86: (8 comments) http://gerrit.cloudera.org:8080/#/c/22924/77/be/src/observe/otel.cc File be/src/observe/otel.cc: http://gerrit.cloudera.org:8080/#/c/22924/77/be/src/observe/otel.cc@243 PS77, Line 243: } else { : exporter = init_exporter_file(); : } : VLOG(2) << "OpenTelemetry exporter: " << FLAGS_otel_trace_exporter; : > Are we sure that OtlpFileExporterFactory::Create() always returns a non-nul The OtlpFileExporterFactory::Create() function ends up at https://github.com/open-telemetry/opentelemetry-cpp/blob/5f6d99f0898be265e36f966f8915d8a99c516738/exporters/otlp/src/otlp_file_exporter_factory.cc#L33-L35. The new keyword shouldn't return nullptr unless maybe if the memory allocation fails (I'm not sure if an exception will be thrown or not). http://gerrit.cloudera.org:8080/#/c/22924/84/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/22924/84/be/src/runtime/coordinator.cc@155 PS84, Line 155: VLOG_QUERY << "Exec() query_id=" << PrintId(query_id()) > Can we remove this TODO? Done http://gerrit.cloudera.org:8080/#/c/22924/84/be/src/runtime/coordinator.cc@1497 PS84, Line 1497: vector<NetworkAddressPB> host_addrs; > Since all the other events are in the past tense, should we call this one " Yeah, it should be past tense. fixed. http://gerrit.cloudera.org:8080/#/c/22924/84/be/src/scheduling/local-admission-control-client.h File be/src/scheduling/local-admission-control-client.h: http://gerrit.cloudera.org:8080/#/c/22924/84/be/src/scheduling/local-admission-control-client.h@55 PS84, Line 55: bool was_queued_; > why do we need to persist this state in the client? I did not see it being It's used when ending the admission control child span -- https://gerrit.cloudera.org/c/22924/85/be/src/observe/span-manager.cc#286. The other alternative was string parsing of the admission control result. http://gerrit.cloudera.org:8080/#/c/22924/84/be/src/service/client-request-state.cc File be/src/service/client-request-state.cc: http://gerrit.cloudera.org:8080/#/c/22924/84/be/src/service/client-request-state.cc@1206 PS84, Line 1206: otel_span_manager_->AddChildSpanEvent("QueryUnregistered"); > Should this be "UnregisteredQuery" or "QueryUnregistered" ? past tense, and I think "QueryUnregistered" sounds better http://gerrit.cloudera.org:8080/#/c/22924/84/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/22924/84/be/src/service/impala-server.cc@3610 PS84, Line 3610: if (FLAGS_is_coordinator && otel_trace_enabled()) { > shutdown condition needs to be same as initialization: good catch! fixed http://gerrit.cloudera.org:8080/#/c/22924/84/tests/common/custom_cluster_test_suite.py File tests/common/custom_cluster_test_suite.py: http://gerrit.cloudera.org:8080/#/c/22924/84/tests/common/custom_cluster_test_suite.py@692 PS84, Line 692: : Returns: > no service in the param, maybe can remove? done, leftover from previous design http://gerrit.cloudera.org:8080/#/c/22924/84/tests/common/custom_cluster_test_suite.py@716 PS84, Line 716: > unreachable break removed, strange that flake8 did not catch that -- To view, visit http://gerrit.cloudera.org:8080/22924 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie40b5cd33274df13f3005bf7a704299ebfff8a5b Gerrit-Change-Number: 22924 Gerrit-PatchSet: 86 Gerrit-Owner: gaurav singh <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Sat, 09 Aug 2025 04:14:04 +0000 Gerrit-HasComments: Yes
