Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/23440 )
Change subject: IMPALA-14455: Cleanup OpenTelemetry Tracing Startup Flags ...................................................................... Patch Set 14: (5 comments) http://gerrit.cloudera.org:8080/#/c/23440/14/be/src/observe/otel-flags-trace.cc File be/src/observe/otel-flags-trace.cc: http://gerrit.cloudera.org:8080/#/c/23440/14/be/src/observe/otel-flags-trace.cc@a64 PS14, Line 64: : : I don't know if we do this for hidden flags, but we have a graveyard for removed flags in be/src/common/global-flags.cc. Mostly that is for flags users might already have used and have specified in their configurations. It logs and ignores them at startup rather than complaining about a flag that doesn't exist. http://gerrit.cloudera.org:8080/#/c/23440/14/be/src/observe/otel-test.cc File be/src/observe/otel-test.cc: http://gerrit.cloudera.org:8080/#/c/23440/14/be/src/observe/otel-test.cc@261 PS14, Line 261: FLAGS_otel_trace_collector_url = "https://foo.com";; : FLAGS_otel_trace_tls_minimum_version = "tlsv1.3"; : FLAGS_otel_trace_timeout_s = 9; : FLAGS_otel_debug = true; : FLAGS_otel_trace_retry_policy_max_attempts = 8; : FLAGS_otel_trace_retry_policy_initial_backoff_s = 7.0; : FLAGS_otel_trace_retry_policy_max_backoff_s = 6; : FLAGS_otel_trace_ssl_ciphers = "override_ssl_ciphers"; : FLAGS_otel_trace_tls_cipher_suites = "override_tls_ciphers"; : FLAGS_otel_trace_tls_insecure_skip_verify = true; : FLAGS_otel_trace_ca_cert_path = "ca_cert_path"; : FLAGS_otel_trace_ca_cert_string = "ca_cert_string"; Nit: Just checking what other flags tune these things: Should we also override otel_trace_retry_policy_backoff_multiplier and otel_trace_compression? We do otel_trace_additional_headers in the subsequent test, so we don't need that here. http://gerrit.cloudera.org:8080/#/c/23440/14/be/src/observe/otel-test.cc@282 PS14, Line 282: EXPECT_EQ(2.0, actual.retry_policy_backoff_multiplier); Nit: This is the default value, right? http://gerrit.cloudera.org:8080/#/c/23440/14/be/src/observe/otel.cc File be/src/observe/otel.cc: http://gerrit.cloudera.org:8080/#/c/23440/14/be/src/observe/otel.cc@202 PS14, Line 202: static OtlpHttpExporterOptions http_exporter_config() { If I'm understanding this, the validation already happened through flag validators, so this wasn't returning not-OK status ever. Seems reasonable to me. http://gerrit.cloudera.org:8080/#/c/23440/14/testdata/bin/otel-collector/otel-config-https.yml File testdata/bin/otel-collector/otel-config-https.yml: http://gerrit.cloudera.org:8080/#/c/23440/14/testdata/bin/otel-collector/otel-config-https.yml@33 PS14, Line 33: insecure: true Just for my understanding: what does this setting do? We have it set both for the http and the https version. -- To view, visit http://gerrit.cloudera.org:8080/23440 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie321fa37c0fd260f783dc6cf47924d53a06d82ea Gerrit-Change-Number: 23440 Gerrit-PatchSet: 14 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Comment-Date: Thu, 20 Nov 2025 19:44:21 +0000 Gerrit-HasComments: Yes
