From 85fde9915f887fe29074d0f1e12242b0053c49c2 Mon Sep 17 00:00:00 2001 From: benibus Date: Wed, 22 May 2024 19:12:16 -0400 Subject: [PATCH] Address misc review points --- cpp/cmake_modules/ThirdpartyToolchain.cmake | 6 ------ cpp/src/arrow/flight/sql/test_app_cli.cc | 7 ++----- cpp/src/arrow/flight/sql/test_server_cli.cc | 4 ++-- cpp/src/arrow/telemetry/logging.cc | 13 ++++++------- 4 files changed, 10 insertions(+), 20 deletions(-) diff --git a/cpp/cmake_modules/ThirdpartyToolchain.cmake b/cpp/cmake_modules/ThirdpartyToolchain.cmake index 14ebd1609a580..0071eab059b37 100644 --- a/cpp/cmake_modules/ThirdpartyToolchain.cmake +++ b/cpp/cmake_modules/ThirdpartyToolchain.cmake @@ -4694,12 +4694,6 @@ macro(build_opentelemetry) -DWITH_OTLP_GRPC=OFF # Disabled because it seemed to cause linking errors. May be worth a closer look. -DWITH_FUNC_TESTS=OFF - # These options are slated for removal in v1.14 and their features are deemed stable - # as of v1.13. However, setting their corresponding ENABLE_* macros in headers seems - # finicky - resulting in build failures or ABI-related runtime errors during HTTP - # client initialization. There may still be a solution, but we disable them for now. - -DWITH_OTLP_HTTP_SSL_PREVIEW=OFF - -DWITH_OTLP_HTTP_SSL_TLS_PREVIEW=OFF "-DProtobuf_INCLUDE_DIR=${OPENTELEMETRY_PROTOBUF_INCLUDE_DIR}" "-DProtobuf_LIBRARY=${OPENTELEMETRY_PROTOBUF_INCLUDE_DIR}" "-DProtobuf_PROTOC_EXECUTABLE=${OPENTELEMETRY_PROTOC_EXECUTABLE}") diff --git a/cpp/src/arrow/flight/sql/test_app_cli.cc b/cpp/src/arrow/flight/sql/test_app_cli.cc index 20cb30fef1108..6b722fd535be0 100644 --- a/cpp/src/arrow/flight/sql/test_app_cli.cc +++ b/cpp/src/arrow/flight/sql/test_app_cli.cc @@ -108,11 +108,6 @@ class OtelScope { private: opentelemetry::trace::Scope scope_; }; -#else -class OtelScope { - public: - static Result> Make() { return nullptr; } -}; #endif Status PrintResultsForEndpoint(FlightSqlClient& client, @@ -158,7 +153,9 @@ Status PrintResults(FlightSqlClient& client, const FlightCallOptions& call_optio } Status RunMain() { +#ifdef ARROW_TELEMETRY ARROW_ASSIGN_OR_RAISE(auto otel_scope, OtelScope::Make()); +#endif ARROW_ASSIGN_OR_RAISE(auto location, Location::ForGrpcTcp(FLAGS_host, FLAGS_port)); auto client_options = FlightClientOptions::Defaults(); diff --git a/cpp/src/arrow/flight/sql/test_server_cli.cc b/cpp/src/arrow/flight/sql/test_server_cli.cc index 5e2527bbff4c3..13a5a4d36a273 100644 --- a/cpp/src/arrow/flight/sql/test_server_cli.cc +++ b/cpp/src/arrow/flight/sql/test_server_cli.cc @@ -61,12 +61,12 @@ arrow::Status SetupOTel() { return arrow::Status::OK(); } -#else -arrow::Status SetupOTel() { return arrow::Status::OK(); } #endif arrow::Status RunMain() { +#ifdef ARROW_TELEMETRY ARROW_RETURN_NOT_OK(SetupOTel()); +#endif ARROW_ASSIGN_OR_RAISE(auto location, arrow::flight::Location::ForGrpcTcp("0.0.0.0", FLAGS_port)); diff --git a/cpp/src/arrow/telemetry/logging.cc b/cpp/src/arrow/telemetry/logging.cc index 5e36b941a9895..7e9a69afedbb5 100644 --- a/cpp/src/arrow/telemetry/logging.cc +++ b/cpp/src/arrow/telemetry/logging.cc @@ -104,17 +104,17 @@ class OtlpOStreamLogRecordExporter final : public otel::sdk::logs::LogRecordExpo otel::logs::Severity ToOtelSeverity(LogLevel level) { switch (level) { case LogLevel::ARROW_TRACE: - return opentelemetry::logs::Severity::kTrace; + return otel::logs::Severity::kTrace; case LogLevel::ARROW_DEBUG: - return opentelemetry::logs::Severity::kDebug; + return otel::logs::Severity::kDebug; case LogLevel::ARROW_INFO: - return opentelemetry::logs::Severity::kInfo; + return otel::logs::Severity::kInfo; case LogLevel::ARROW_WARNING: - return opentelemetry::logs::Severity::kWarn; + return otel::logs::Severity::kWarn; case LogLevel::ARROW_ERROR: - return opentelemetry::logs::Severity::kError; + return otel::logs::Severity::kError; case LogLevel::ARROW_FATAL: - return opentelemetry::logs::Severity::kFatal; + return otel::logs::Severity::kFatal; } return otel::logs::Severity::kInvalid; } @@ -133,7 +133,6 @@ std::unique_ptr MakeExporter( } break; case ExporterKind::OTLP_HTTP: { namespace otlp = otel::exporter::otlp; - // TODO: Allow user configuration here? otlp::OtlpHttpLogRecordExporterOptions options{}; return std::make_unique(options); } break;