Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/23440 )
Change subject: IMPALA-14455: Cleanup OpenTelemetry Tracing Startup Flags ...................................................................... Patch Set 16: (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 re I considered putting the flag in the graveyard but decided against that since the queries as OTel traces feature is tech preview and this flag was hidden. Additionally, users are unaware of this flag unless they dug through the code. For those two reasons, I felt comfortable with removing the flag instead of graveyarding it. 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: EXPECT_EQ(2.0, actual.retry_policy_backoff_multiplier); : EXPECT_EQ("zlib", actual.compression); : EXPECT_EQ("1.0", actual.ssl_min_tls); : EXPECT_EQ("1.3", actual.ssl_max_tls); : EXPECT_EQ("ssl_ciphers", actual.ssl_cipher); : EXPECT_EQ("tls_ciphers", actual.ssl_cipher_suite); : EXPECT_EQ(false, actual.ssl_insecure_skip_verify); : EXPECT_EQ("", actual.ssl_ca_cert_path); : EXPECT_EQ("", actual.ssl_ca_cert_string); : EXPECT_EQ(0, actual.http_headers.size()); : EXPECT_TRUE(actual.http_headers.empty()); : } > Nit: Just checking what other flags tune these things: Should we also overr Good catch. They need to be asserted as well. http://gerrit.cloudera.org:8080/#/c/23440/14/be/src/observe/otel-test.cc@282 PS14, Line 282: FLAGS_otel_trace_retry_policy_initial_backoff_s = 7.0; > Nit: This is the default value, right? Yes. Fixed to use a non-default value. 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: // Configure OTLP HTTP exporter > If I'm understanding this, the validation already happened through flag val Yes, that is exactly the case. I added verbiage to the commit message explaining this change 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 f Exporters define the endpoint where the OTel collector forwards telemetry data. If that endpoint presents a TLS certificate, setting insecure: true says to skip validation of the cert that was presented. The difference between otel-config-http.yml and otel-config-https.yml is the port that the OTel collector listens on. This port is where Impala sends its telemetry data to. In the -http.yml file, that port is not TLS encrypted. In the -https.yml file, that port is TLS encrypted. -- 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: 16 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Comment-Date: Thu, 20 Nov 2025 23:35:51 +0000 Gerrit-HasComments: Yes
