From f495fcf33c37d0b9f74dc27ada4a9345c8c2ab69 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Wed, 8 Apr 2026 11:50:37 -0400 Subject: [PATCH 01/29] claude prototype --- CMakeLists.txt | 15 ++ conanfile.py | 11 + src/viam/examples/modules/complex/buf.lock | 4 +- src/viam/sdk/CMakeLists.txt | 6 + src/viam/sdk/common/client_helper.cpp | 2 + src/viam/sdk/common/instance.cpp | 2 + .../sdk/common/private/service_helper.hpp | 8 +- src/viam/sdk/common/private/tracing.hpp | 55 +++++ src/viam/sdk/common/tracing.cpp | 196 ++++++++++++++++++ 9 files changed, 296 insertions(+), 3 deletions(-) create mode 100644 src/viam/sdk/common/private/tracing.hpp create mode 100644 src/viam/sdk/common/tracing.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 8184f848e..d5014af40 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -132,6 +132,17 @@ option(VIAMCPPSDK_SANITIZED_BUILD "Build with address and UB sanitizers (less pe # option(VIAMCPPSDK_CLANG_TIDY "Run the clang-tidy linter" OFF) + +# - `VIAMCPPSDK_OPENTELEMETRY_TRACING` +# +# When enabled (default), links against opentelemetry-cpp and compiles in +# W3C Trace Context propagation for all gRPC client and server calls. Spans +# are exported via whatever TracerProvider the application configures at +# runtime; the SDK defaults to the OTel noop provider (no export overhead). +# Disable this if you need to build without an opentelemetry-cpp installation. +# +option(VIAMCPPSDK_OPENTELEMETRY_TRACING "Compile OpenTelemetry tracing into all gRPC calls" ON) + # The following options are only defined if this project is not being included as a subproject if (CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR) option(VIAMCPPSDK_BUILD_EXAMPLES "Build the example executables" ON) @@ -485,6 +496,10 @@ if(VIAMCPPSDK_GRPCXX_VERSION VERSION_LESS 1.43.0) set(VIAMCPPSDK_GRPCXX_NO_DIRECT_DIAL 1) endif() +if (VIAMCPPSDK_OPENTELEMETRY_TRACING) + find_package(opentelemetry-cpp CONFIG REQUIRED COMPONENTS trace) +endif() + include(FetchContent) FetchContent_Declare( diff --git a/conanfile.py b/conanfile.py index 0c6d91015..30d21ab1b 100644 --- a/conanfile.py +++ b/conanfile.py @@ -18,11 +18,13 @@ class ViamCppSdkRecipe(ConanFile): options = { "offline_proto_generation": [True, False], + "opentelemetry_tracing": [True, False], "shared": [True, False] } default_options = { "offline_proto_generation": True, + "opentelemetry_tracing": True, "shared": True } @@ -68,6 +70,9 @@ def requirements(self): self.requires('protobuf/[>=3.17.1 <6.30.0]') self.requires(self._xtensor_requires(), transitive_headers=True) + if self.options.opentelemetry_tracing: + self.requires('opentelemetry-cpp/[>=1.9.0]') + def build_requirements(self): if self.options.offline_proto_generation: self.tool_requires(self._grpc_requires()) @@ -80,6 +85,7 @@ def generate(self): tc = CMakeToolchain(self) tc.cache_variables["VIAMCPPSDK_OFFLINE_PROTO_GENERATION"] = self.options.offline_proto_generation + tc.cache_variables["VIAMCPPSDK_OPENTELEMETRY_TRACING"] = self.options.opentelemetry_tracing tc.cache_variables["VIAMCPPSDK_USE_DYNAMIC_PROTOS"] = True # We don't want to constrain these for conan builds because we @@ -153,6 +159,11 @@ def package_info(self): "viamapi", ]) + if self.options.opentelemetry_tracing: + self.cpp_info.components["viamsdk"].requires.append( + "opentelemetry-cpp::opentelemetry_trace" + ) + self.cpp_info.components["viamsdk"].requires.extend([ "viam_rust_utils" ]) diff --git a/src/viam/examples/modules/complex/buf.lock b/src/viam/examples/modules/complex/buf.lock index 5df8acde6..ab5dd9737 100644 --- a/src/viam/examples/modules/complex/buf.lock +++ b/src/viam/examples/modules/complex/buf.lock @@ -2,5 +2,5 @@ version: v2 deps: - name: buf.build/googleapis/googleapis - commit: 004180b77378443887d3b55cabc00384 - digest: b5:e8f475fe3330f31f5fd86ac689093bcd274e19611a09db91f41d637cb9197881ce89882b94d13a58738e53c91c6e4bae7dc1feba85f590164c975a89e25115dc + commit: 536964a08a534d51b8f30f2d6751f1f9 + digest: b5:3e05d27e797b00c345fadd3c15cf0e16c4cc693036a55059721e66d6ce22a96264a4897658c9243bb0874fa9ca96e437589eb512189d2754604a626c632f6030 diff --git a/src/viam/sdk/CMakeLists.txt b/src/viam/sdk/CMakeLists.txt index b5b129449..ecefe9602 100644 --- a/src/viam/sdk/CMakeLists.txt +++ b/src/viam/sdk/CMakeLists.txt @@ -62,6 +62,7 @@ target_sources(viamsdk common/client_helper.cpp common/exception.cpp common/instance.cpp + common/tracing.cpp common/kinematics.cpp common/linear_algebra.cpp common/mesh.cpp @@ -308,6 +309,11 @@ target_link_libraries(viamsdk PRIVATE Threads::Threads ) +if (VIAMCPPSDK_OPENTELEMETRY_TRACING) + target_compile_definitions(viamsdk PRIVATE VIAMCPPSDK_OPENTELEMETRY_TRACING) + target_link_libraries(viamsdk PRIVATE opentelemetry-cpp::trace) +endif() + # if the `viam_rust_utils` target exists then we should use it. If not then # we're probably on a non-x86_64 windows build or some other platform without # automated `rust-utils` builds, so we should use the stubs instead. diff --git a/src/viam/sdk/common/client_helper.cpp b/src/viam/sdk/common/client_helper.cpp index b7dc1cadf..0449e9cb8 100644 --- a/src/viam/sdk/common/client_helper.cpp +++ b/src/viam/sdk/common/client_helper.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include @@ -36,6 +37,7 @@ ClientContext::ClientContext(const ViamChannel& channel) : ClientContext() { if (channel.auth_token().has_value()) { wrapped_context_->AddMetadata("authorization", "Bearer " + *channel.auth_token()); } + impl::inject_trace_context(wrapped_context_.get()); } ClientContext::~ClientContext() = default; diff --git a/src/viam/sdk/common/instance.cpp b/src/viam/sdk/common/instance.cpp index 06baf29f4..6ea0d4463 100644 --- a/src/viam/sdk/common/instance.cpp +++ b/src/viam/sdk/common/instance.cpp @@ -4,6 +4,7 @@ #include #include +#include #include namespace viam { @@ -28,6 +29,7 @@ Instance::Instance() { impl_ = std::make_unique(); impl_->registry.initialize(); impl_->log_mgr.init_logging(); + impl::initialize_trace_propagator(); } Instance::~Instance() { diff --git a/src/viam/sdk/common/private/service_helper.hpp b/src/viam/sdk/common/private/service_helper.hpp index 52cb4ea2c..eb7fc98dc 100644 --- a/src/viam/sdk/common/private/service_helper.hpp +++ b/src/viam/sdk/common/private/service_helper.hpp @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -30,6 +31,8 @@ class ServiceHelperBase { protected: explicit ServiceHelperBase(const char* method) noexcept : method_{method} {} + const char* method_name() const noexcept { return method_; } + private: const char* method_; }; @@ -56,7 +59,10 @@ class ServiceHelper : public ServiceHelperBase { return failNoResource(request_->name()); } const GrpcContextObserver::Enable enable{*context_}; - return invoke_(std::forward(callable), std::move(resource)); + impl::ServerSpanGuard span_guard{context_, method_name()}; + const auto status = invoke_(std::forward(callable), std::move(resource)); + span_guard.commit(static_cast(status.error_code())); + return status; } catch (const std::exception& xcp) { return failStdException(xcp); } catch (...) { diff --git a/src/viam/sdk/common/private/tracing.hpp b/src/viam/sdk/common/private/tracing.hpp new file mode 100644 index 000000000..635e56666 --- /dev/null +++ b/src/viam/sdk/common/private/tracing.hpp @@ -0,0 +1,55 @@ +#pragma once + +#include + +#include + +namespace viam { +namespace sdk { +namespace impl { + +/// @brief RAII guard that creates a server-side OpenTelemetry span for the duration of a gRPC +/// handler invocation. Extracts W3C trace context from the incoming gRPC request metadata and +/// starts a child span (or a new root span if no traceparent header is present). When +/// destroyed, ends the span and records the final gRPC status. +/// +/// If OpenTelemetry tracing is not compiled in, or no tracer provider has been configured, +/// this is a complete no-op with no runtime overhead beyond a null pointer check. +/// +/// @note Instances must be created and destroyed on the same thread (the gRPC handler thread). +class ServerSpanGuard { + public: + explicit ServerSpanGuard(const GrpcServerContext* ctx, const char* method) noexcept; + ~ServerSpanGuard() noexcept; + + /// @brief Record the final gRPC status code before destruction. + /// + /// Call this when the handler returns normally (without throwing). The @p grpc_status_code + /// should be the integer value of the @c grpc::StatusCode (0 = OK). If @c commit() is not + /// called before destruction (e.g., because the handler threw an exception), the span status + /// defaults to @c StatusCode::kError. + void commit(int grpc_status_code) noexcept; + + ServerSpanGuard(const ServerSpanGuard&) = delete; + ServerSpanGuard& operator=(const ServerSpanGuard&) = delete; + + private: + struct Impl; + std::unique_ptr impl_; +}; + +/// @brief Inject the currently-active OpenTelemetry trace context as W3C @c traceparent / +/// @c tracestate metadata entries into an outgoing gRPC client context. +/// +/// This propagates trace context from any server span that is currently active on the calling +/// thread (e.g., one created by @c ServerSpanGuard) to downstream gRPC calls. It is a no-op +/// when no span is active or when OpenTelemetry tracing is not compiled in. +void inject_trace_context(GrpcClientContext* ctx) noexcept; + +/// @brief Install the W3C Trace Context propagator as the global OpenTelemetry text-map +/// propagator. Called once during @c Instance construction. +void initialize_trace_propagator() noexcept; + +} // namespace impl +} // namespace sdk +} // namespace viam diff --git a/src/viam/sdk/common/tracing.cpp b/src/viam/sdk/common/tracing.cpp new file mode 100644 index 000000000..eea35f409 --- /dev/null +++ b/src/viam/sdk/common/tracing.cpp @@ -0,0 +1,196 @@ +#include + +#include +#include + +#ifdef VIAMCPPSDK_OPENTELEMETRY_TRACING + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace otel_ctx = opentelemetry::context; +namespace otel_prop = opentelemetry::context::propagation; +namespace otel_trace = opentelemetry::trace; + +namespace viam { +namespace sdk { +namespace impl { + +namespace { + +constexpr const char* k_instrumentation_scope = "viam-cpp-sdk"; + +// Carrier for reading W3C trace context from incoming gRPC request metadata (server side). +class GrpcServerCarrier : public otel_prop::TextMapCarrier { + public: + explicit GrpcServerCarrier(const grpc::ServerContext& ctx) noexcept : ctx_(ctx) {} + + opentelemetry::nostd::string_view Get( + opentelemetry::nostd::string_view key) const noexcept override { + const auto& metadata = ctx_.client_metadata(); + const auto it = metadata.find(grpc::string_ref{key.data(), key.size()}); + if (it != metadata.end()) { + return {it->second.data(), it->second.size()}; + } + return {}; + } + + void Set(opentelemetry::nostd::string_view, + opentelemetry::nostd::string_view) noexcept override {} + + private: + const grpc::ServerContext& ctx_; +}; + +// Carrier for writing W3C trace context into outgoing gRPC request metadata (client side). +class GrpcClientCarrier : public otel_prop::TextMapCarrier { + public: + explicit GrpcClientCarrier(grpc::ClientContext* ctx) noexcept : ctx_(ctx) {} + + opentelemetry::nostd::string_view Get( + opentelemetry::nostd::string_view) const noexcept override { + return {}; + } + + void Set(opentelemetry::nostd::string_view key, + opentelemetry::nostd::string_view value) noexcept override { + ctx_->AddMetadata(std::string(key), std::string(value)); + } + + private: + grpc::ClientContext* ctx_; +}; + +// Heap-allocated pair of (span, scope) so that otel_trace::Scope — which is neither +// copyable nor moveable — can be managed via a raw pointer stored in ServerSpanGuard::Impl. +// Constructing SpanAndScope makes the span active on the current thread (via Scope); deleting +// it deactivates the span and then releases the shared_ptr. +// The span member must be declared before scope so that it is fully initialised before the +// Scope constructor runs (Scope holds a reference to it). +struct SpanAndScope { + opentelemetry::nostd::shared_ptr span; + otel_trace::Scope scope; // Must follow span in declaration order + + explicit SpanAndScope(opentelemetry::nostd::shared_ptr s) noexcept + : span(std::move(s)), scope(span) {} +}; + +} // namespace + +// ----- ServerSpanGuard ------------------------------------------------------- + +struct ServerSpanGuard::Impl { + SpanAndScope* span_scope = nullptr; + bool committed = false; +}; + +ServerSpanGuard::ServerSpanGuard(const GrpcServerContext* ctx, const char* method) noexcept { + try { + auto tracer = + otel_trace::Provider::GetTracerProvider()->GetTracer(k_instrumentation_scope); + + otel_trace::StartSpanOptions opts; + opts.kind = otel_trace::SpanKind::kServer; + + if (ctx) { + GrpcServerCarrier carrier{*ctx}; + auto current_ctx = otel_ctx::RuntimeContext::GetCurrent(); + const auto extracted = + otel_prop::GlobalTextMapPropagator::GetGlobalPropagator()->Extract( + carrier, current_ctx); + opts.parent = otel_trace::GetSpan(extracted)->GetContext(); + } + + auto span = tracer->StartSpan(method, opts); + span->SetAttribute("rpc.system", "grpc"); + + impl_ = std::make_unique(); + impl_->span_scope = new SpanAndScope(std::move(span)); + } catch (...) { + // Tracing is best-effort; never let failures here affect the gRPC call. + impl_.reset(); + } +} + +ServerSpanGuard::~ServerSpanGuard() noexcept { + if (!impl_ || !impl_->span_scope) { + return; + } + if (!impl_->committed) { + impl_->span_scope->span->SetStatus(otel_trace::StatusCode::kError, + "handler threw an exception"); + } + impl_->span_scope->span->End(); + delete impl_->span_scope; + impl_->span_scope = nullptr; +} + +void ServerSpanGuard::commit(int grpc_status_code) noexcept { + if (!impl_ || !impl_->span_scope) { + return; + } + impl_->committed = true; + if (grpc_status_code == 0 /* grpc::StatusCode::OK */) { + impl_->span_scope->span->SetStatus(otel_trace::StatusCode::kOk); + } else { + impl_->span_scope->span->SetStatus(otel_trace::StatusCode::kError); + impl_->span_scope->span->SetAttribute("rpc.grpc.status_code", grpc_status_code); + } +} + +// ----- free functions -------------------------------------------------------- + +void inject_trace_context(GrpcClientContext* ctx) noexcept { + try { + GrpcClientCarrier carrier{ctx}; + otel_prop::GlobalTextMapPropagator::GetGlobalPropagator()->Inject( + carrier, otel_ctx::RuntimeContext::GetCurrent()); + } catch (...) { + } +} + +void initialize_trace_propagator() noexcept { + try { + otel_prop::GlobalTextMapPropagator::SetGlobalPropagator( + opentelemetry::nostd::shared_ptr( + new opentelemetry::trace::propagation::HttpTraceContext())); + } catch (...) { + } +} + +} // namespace impl +} // namespace sdk +} // namespace viam + +#else // VIAMCPPSDK_OPENTELEMETRY_TRACING — provide no-op implementations + +namespace viam { +namespace sdk { +namespace impl { + +// Provide a minimal Impl so that the unique_ptr destructor is well-defined. +struct ServerSpanGuard::Impl {}; + +ServerSpanGuard::ServerSpanGuard(const GrpcServerContext*, const char*) noexcept + : impl_(nullptr) {} + +ServerSpanGuard::~ServerSpanGuard() noexcept = default; + +void ServerSpanGuard::commit(int) noexcept {} + +void inject_trace_context(GrpcClientContext*) noexcept {} + +void initialize_trace_propagator() noexcept {} + +} // namespace impl +} // namespace sdk +} // namespace viam + +#endif // VIAMCPPSDK_OPENTELEMETRY_TRACING From 4cf739f2598f70fc8434eef8c51b3539a1b43df3 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Wed, 29 Apr 2026 16:31:55 -0400 Subject: [PATCH 02/29] put cpp file in private too Co-Authored-By: Claude Sonnet 4.6 --- CMakeLists.txt | 2 +- src/viam/sdk/CMakeLists.txt | 4 ++-- src/viam/sdk/common/{ => private}/tracing.cpp | 0 3 files changed, 3 insertions(+), 3 deletions(-) rename src/viam/sdk/common/{ => private}/tracing.cpp (100%) diff --git a/CMakeLists.txt b/CMakeLists.txt index 9d83f4f5c..f608dfde3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -497,7 +497,7 @@ if(VIAMCPPSDK_GRPCXX_VERSION VERSION_LESS 1.43.0) endif() if (VIAMCPPSDK_OPENTELEMETRY_TRACING) - find_package(opentelemetry-cpp CONFIG REQUIRED COMPONENTS trace) + find_package(opentelemetry-cpp CONFIG REQUIRED COMPONENTS sdk) endif() include(FetchContent) diff --git a/src/viam/sdk/CMakeLists.txt b/src/viam/sdk/CMakeLists.txt index ecefe9602..f9761eb21 100644 --- a/src/viam/sdk/CMakeLists.txt +++ b/src/viam/sdk/CMakeLists.txt @@ -62,7 +62,6 @@ target_sources(viamsdk common/client_helper.cpp common/exception.cpp common/instance.cpp - common/tracing.cpp common/kinematics.cpp common/linear_algebra.cpp common/mesh.cpp @@ -72,6 +71,7 @@ target_sources(viamsdk common/version_metadata.cpp common/world_state.cpp common/private/service_helper.cpp + common/private/tracing.cpp components/arm.cpp components/audio_in.cpp components/audio_out.cpp @@ -311,7 +311,7 @@ target_link_libraries(viamsdk if (VIAMCPPSDK_OPENTELEMETRY_TRACING) target_compile_definitions(viamsdk PRIVATE VIAMCPPSDK_OPENTELEMETRY_TRACING) - target_link_libraries(viamsdk PRIVATE opentelemetry-cpp::trace) + target_link_libraries(viamsdk PRIVATE opentelemetry-cpp::sdk) endif() # if the `viam_rust_utils` target exists then we should use it. If not then diff --git a/src/viam/sdk/common/tracing.cpp b/src/viam/sdk/common/private/tracing.cpp similarity index 100% rename from src/viam/sdk/common/tracing.cpp rename to src/viam/sdk/common/private/tracing.cpp From e28403950f2502354caa301c1210383f1fc42f8f Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Thu, 30 Apr 2026 15:31:06 -0400 Subject: [PATCH 03/29] use unique_ptr Co-Authored-By: Claude Sonnet 4.6 --- src/viam/sdk/common/private/tracing.cpp | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/viam/sdk/common/private/tracing.cpp b/src/viam/sdk/common/private/tracing.cpp index eea35f409..89a067685 100644 --- a/src/viam/sdk/common/private/tracing.cpp +++ b/src/viam/sdk/common/private/tracing.cpp @@ -69,14 +69,14 @@ class GrpcClientCarrier : public otel_prop::TextMapCarrier { }; // Heap-allocated pair of (span, scope) so that otel_trace::Scope — which is neither -// copyable nor moveable — can be managed via a raw pointer stored in ServerSpanGuard::Impl. -// Constructing SpanAndScope makes the span active on the current thread (via Scope); deleting +// copyable nor moveable — can be owned by ServerSpanGuard::Impl via unique_ptr. +// Constructing SpanAndScope makes the span active on the current thread (via Scope); destroying // it deactivates the span and then releases the shared_ptr. // The span member must be declared before scope so that it is fully initialised before the // Scope constructor runs (Scope holds a reference to it). struct SpanAndScope { opentelemetry::nostd::shared_ptr span; - otel_trace::Scope scope; // Must follow span in declaration order + otel_trace::Scope scope; explicit SpanAndScope(opentelemetry::nostd::shared_ptr s) noexcept : span(std::move(s)), scope(span) {} @@ -87,14 +87,13 @@ struct SpanAndScope { // ----- ServerSpanGuard ------------------------------------------------------- struct ServerSpanGuard::Impl { - SpanAndScope* span_scope = nullptr; + std::unique_ptr span_scope; bool committed = false; }; ServerSpanGuard::ServerSpanGuard(const GrpcServerContext* ctx, const char* method) noexcept { try { - auto tracer = - otel_trace::Provider::GetTracerProvider()->GetTracer(k_instrumentation_scope); + auto tracer = otel_trace::Provider::GetTracerProvider()->GetTracer(k_instrumentation_scope); otel_trace::StartSpanOptions opts; opts.kind = otel_trace::SpanKind::kServer; @@ -103,8 +102,8 @@ ServerSpanGuard::ServerSpanGuard(const GrpcServerContext* ctx, const char* metho GrpcServerCarrier carrier{*ctx}; auto current_ctx = otel_ctx::RuntimeContext::GetCurrent(); const auto extracted = - otel_prop::GlobalTextMapPropagator::GetGlobalPropagator()->Extract( - carrier, current_ctx); + otel_prop::GlobalTextMapPropagator::GetGlobalPropagator()->Extract(carrier, + current_ctx); opts.parent = otel_trace::GetSpan(extracted)->GetContext(); } @@ -112,7 +111,7 @@ ServerSpanGuard::ServerSpanGuard(const GrpcServerContext* ctx, const char* metho span->SetAttribute("rpc.system", "grpc"); impl_ = std::make_unique(); - impl_->span_scope = new SpanAndScope(std::move(span)); + impl_->span_scope = std::make_unique(std::move(span)); } catch (...) { // Tracing is best-effort; never let failures here affect the gRPC call. impl_.reset(); @@ -128,8 +127,6 @@ ServerSpanGuard::~ServerSpanGuard() noexcept { "handler threw an exception"); } impl_->span_scope->span->End(); - delete impl_->span_scope; - impl_->span_scope = nullptr; } void ServerSpanGuard::commit(int grpc_status_code) noexcept { @@ -178,8 +175,7 @@ namespace impl { // Provide a minimal Impl so that the unique_ptr destructor is well-defined. struct ServerSpanGuard::Impl {}; -ServerSpanGuard::ServerSpanGuard(const GrpcServerContext*, const char*) noexcept - : impl_(nullptr) {} +ServerSpanGuard::ServerSpanGuard(const GrpcServerContext*, const char*) noexcept : impl_(nullptr) {} ServerSpanGuard::~ServerSpanGuard() noexcept = default; From 11dd1758301429c27f4012607cbce0cef775cef8 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Thu, 30 Apr 2026 18:17:36 -0400 Subject: [PATCH 04/29] claude cleanup --- src/viam/sdk/common/private/tracing.cpp | 97 +++++++++---------------- 1 file changed, 35 insertions(+), 62 deletions(-) diff --git a/src/viam/sdk/common/private/tracing.cpp b/src/viam/sdk/common/private/tracing.cpp index 89a067685..55a535290 100644 --- a/src/viam/sdk/common/private/tracing.cpp +++ b/src/viam/sdk/common/private/tracing.cpp @@ -68,98 +68,72 @@ class GrpcClientCarrier : public otel_prop::TextMapCarrier { grpc::ClientContext* ctx_; }; -// Heap-allocated pair of (span, scope) so that otel_trace::Scope — which is neither -// copyable nor moveable — can be owned by ServerSpanGuard::Impl via unique_ptr. -// Constructing SpanAndScope makes the span active on the current thread (via Scope); destroying -// it deactivates the span and then releases the shared_ptr. -// The span member must be declared before scope so that it is fully initialised before the -// Scope constructor runs (Scope holds a reference to it). -struct SpanAndScope { - opentelemetry::nostd::shared_ptr span; - otel_trace::Scope scope; - - explicit SpanAndScope(opentelemetry::nostd::shared_ptr s) noexcept - : span(std::move(s)), scope(span) {} -}; - } // namespace -// ----- ServerSpanGuard ------------------------------------------------------- - +// Constructing Impl makes the span active on the current thread (via Scope); destroying it +// deactivates the span and then releases the shared_ptr. struct ServerSpanGuard::Impl { - std::unique_ptr span_scope; + opentelemetry::nostd::shared_ptr span; + otel_trace::Scope scope; bool committed = false; + + explicit Impl(opentelemetry::nostd::shared_ptr s) noexcept + : span(std::move(s)), scope(span) {} }; ServerSpanGuard::ServerSpanGuard(const GrpcServerContext* ctx, const char* method) noexcept { - try { - auto tracer = otel_trace::Provider::GetTracerProvider()->GetTracer(k_instrumentation_scope); - - otel_trace::StartSpanOptions opts; - opts.kind = otel_trace::SpanKind::kServer; - - if (ctx) { - GrpcServerCarrier carrier{*ctx}; - auto current_ctx = otel_ctx::RuntimeContext::GetCurrent(); - const auto extracted = - otel_prop::GlobalTextMapPropagator::GetGlobalPropagator()->Extract(carrier, - current_ctx); - opts.parent = otel_trace::GetSpan(extracted)->GetContext(); - } + auto tracer = otel_trace::Provider::GetTracerProvider()->GetTracer(k_instrumentation_scope); - auto span = tracer->StartSpan(method, opts); - span->SetAttribute("rpc.system", "grpc"); + otel_trace::StartSpanOptions opts; + opts.kind = otel_trace::SpanKind::kServer; - impl_ = std::make_unique(); - impl_->span_scope = std::make_unique(std::move(span)); - } catch (...) { - // Tracing is best-effort; never let failures here affect the gRPC call. - impl_.reset(); + if (ctx) { + GrpcServerCarrier carrier{*ctx}; + auto current_ctx = otel_ctx::RuntimeContext::GetCurrent(); + const auto extracted = otel_prop::GlobalTextMapPropagator::GetGlobalPropagator()->Extract( + carrier, current_ctx); + opts.parent = otel_trace::GetSpan(extracted)->GetContext(); } + + auto span = tracer->StartSpan(method, opts); + span->SetAttribute("rpc.system", "grpc"); + + impl_ = std::make_unique(std::move(span)); } ServerSpanGuard::~ServerSpanGuard() noexcept { - if (!impl_ || !impl_->span_scope) { + if (!impl_) { return; } if (!impl_->committed) { - impl_->span_scope->span->SetStatus(otel_trace::StatusCode::kError, - "handler threw an exception"); + impl_->span->SetStatus(otel_trace::StatusCode::kError, "handler threw an exception"); } - impl_->span_scope->span->End(); + impl_->span->End(); } void ServerSpanGuard::commit(int grpc_status_code) noexcept { - if (!impl_ || !impl_->span_scope) { + if (!impl_) { return; } impl_->committed = true; if (grpc_status_code == 0 /* grpc::StatusCode::OK */) { - impl_->span_scope->span->SetStatus(otel_trace::StatusCode::kOk); + impl_->span->SetStatus(otel_trace::StatusCode::kOk); } else { - impl_->span_scope->span->SetStatus(otel_trace::StatusCode::kError); - impl_->span_scope->span->SetAttribute("rpc.grpc.status_code", grpc_status_code); + impl_->span->SetStatus(otel_trace::StatusCode::kError); + impl_->span->SetAttribute("rpc.grpc.status_code", grpc_status_code); } } -// ----- free functions -------------------------------------------------------- - void inject_trace_context(GrpcClientContext* ctx) noexcept { - try { - GrpcClientCarrier carrier{ctx}; - otel_prop::GlobalTextMapPropagator::GetGlobalPropagator()->Inject( - carrier, otel_ctx::RuntimeContext::GetCurrent()); - } catch (...) { - } + GrpcClientCarrier carrier{ctx}; + otel_prop::GlobalTextMapPropagator::GetGlobalPropagator()->Inject( + carrier, otel_ctx::RuntimeContext::GetCurrent()); } void initialize_trace_propagator() noexcept { - try { - otel_prop::GlobalTextMapPropagator::SetGlobalPropagator( - opentelemetry::nostd::shared_ptr( - new opentelemetry::trace::propagation::HttpTraceContext())); - } catch (...) { - } + otel_prop::GlobalTextMapPropagator::SetGlobalPropagator( + opentelemetry::nostd::shared_ptr( + new opentelemetry::trace::propagation::HttpTraceContext())); } } // namespace impl @@ -172,10 +146,9 @@ namespace viam { namespace sdk { namespace impl { -// Provide a minimal Impl so that the unique_ptr destructor is well-defined. struct ServerSpanGuard::Impl {}; -ServerSpanGuard::ServerSpanGuard(const GrpcServerContext*, const char*) noexcept : impl_(nullptr) {} +ServerSpanGuard::ServerSpanGuard(const GrpcServerContext*, const char*) noexcept {} ServerSpanGuard::~ServerSpanGuard() noexcept = default; From b7205c2344fca4cf25ef9ae8d3740aaf30409b80 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Fri, 1 May 2026 10:36:52 -0400 Subject: [PATCH 05/29] more claude cleanup --- CMakeLists.txt | 5 +---- src/viam/sdk/common/private/service_helper.hpp | 4 +--- src/viam/sdk/common/private/tracing.cpp | 16 +++++++++------- src/viam/sdk/common/private/tracing.hpp | 13 +++++-------- 4 files changed, 16 insertions(+), 22 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index f608dfde3..be075227c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -136,10 +136,7 @@ option(VIAMCPPSDK_CLANG_TIDY "Run the clang-tidy linter" OFF) # - `VIAMCPPSDK_OPENTELEMETRY_TRACING` # # When enabled (default), links against opentelemetry-cpp and compiles in -# W3C Trace Context propagation for all gRPC client and server calls. Spans -# are exported via whatever TracerProvider the application configures at -# runtime; the SDK defaults to the OTel noop provider (no export overhead). -# Disable this if you need to build without an opentelemetry-cpp installation. +# W3C Trace Context propagation for all gRPC client and server calls. # option(VIAMCPPSDK_OPENTELEMETRY_TRACING "Compile OpenTelemetry tracing into all gRPC calls" ON) diff --git a/src/viam/sdk/common/private/service_helper.hpp b/src/viam/sdk/common/private/service_helper.hpp index eb7fc98dc..f98b6f73a 100644 --- a/src/viam/sdk/common/private/service_helper.hpp +++ b/src/viam/sdk/common/private/service_helper.hpp @@ -60,9 +60,7 @@ class ServiceHelper : public ServiceHelperBase { } const GrpcContextObserver::Enable enable{*context_}; impl::ServerSpanGuard span_guard{context_, method_name()}; - const auto status = invoke_(std::forward(callable), std::move(resource)); - span_guard.commit(static_cast(status.error_code())); - return status; + return span_guard.commit(invoke_(std::forward(callable), std::move(resource))); } catch (const std::exception& xcp) { return failStdException(xcp); } catch (...) { diff --git a/src/viam/sdk/common/private/tracing.cpp b/src/viam/sdk/common/private/tracing.cpp index 55a535290..1e4be4e26 100644 --- a/src/viam/sdk/common/private/tracing.cpp +++ b/src/viam/sdk/common/private/tracing.cpp @@ -70,8 +70,7 @@ class GrpcClientCarrier : public otel_prop::TextMapCarrier { } // namespace -// Constructing Impl makes the span active on the current thread (via Scope); destroying it -// deactivates the span and then releases the shared_ptr. +// Constructing Impl makes the span active on the current thread struct ServerSpanGuard::Impl { opentelemetry::nostd::shared_ptr span; otel_trace::Scope scope; @@ -111,17 +110,18 @@ ServerSpanGuard::~ServerSpanGuard() noexcept { impl_->span->End(); } -void ServerSpanGuard::commit(int grpc_status_code) noexcept { +::grpc::Status ServerSpanGuard::commit(::grpc::Status status) noexcept { if (!impl_) { - return; + return status; } impl_->committed = true; - if (grpc_status_code == 0 /* grpc::StatusCode::OK */) { + if (status.error_code() == ::grpc::StatusCode::OK) { impl_->span->SetStatus(otel_trace::StatusCode::kOk); } else { impl_->span->SetStatus(otel_trace::StatusCode::kError); - impl_->span->SetAttribute("rpc.grpc.status_code", grpc_status_code); + impl_->span->SetAttribute("rpc.grpc.status_code", static_cast(status.error_code())); } + return status; } void inject_trace_context(GrpcClientContext* ctx) noexcept { @@ -152,7 +152,9 @@ ServerSpanGuard::ServerSpanGuard(const GrpcServerContext*, const char*) noexcept ServerSpanGuard::~ServerSpanGuard() noexcept = default; -void ServerSpanGuard::commit(int) noexcept {} +::grpc::Status ServerSpanGuard::commit(::grpc::Status status) noexcept { + return status; +} void inject_trace_context(GrpcClientContext*) noexcept {} diff --git a/src/viam/sdk/common/private/tracing.hpp b/src/viam/sdk/common/private/tracing.hpp index 635e56666..6451c40bb 100644 --- a/src/viam/sdk/common/private/tracing.hpp +++ b/src/viam/sdk/common/private/tracing.hpp @@ -2,6 +2,8 @@ #include +#include + #include namespace viam { @@ -14,7 +16,7 @@ namespace impl { /// destroyed, ends the span and records the final gRPC status. /// /// If OpenTelemetry tracing is not compiled in, or no tracer provider has been configured, -/// this is a complete no-op with no runtime overhead beyond a null pointer check. +/// uses a no-op implementation. /// /// @note Instances must be created and destroyed on the same thread (the gRPC handler thread). class ServerSpanGuard { @@ -22,13 +24,8 @@ class ServerSpanGuard { explicit ServerSpanGuard(const GrpcServerContext* ctx, const char* method) noexcept; ~ServerSpanGuard() noexcept; - /// @brief Record the final gRPC status code before destruction. - /// - /// Call this when the handler returns normally (without throwing). The @p grpc_status_code - /// should be the integer value of the @c grpc::StatusCode (0 = OK). If @c commit() is not - /// called before destruction (e.g., because the handler threw an exception), the span status - /// defaults to @c StatusCode::kError. - void commit(int grpc_status_code) noexcept; + /// @brief Record the final gRPC status before destruction and return it unchanged. + ::grpc::Status commit(::grpc::Status status) noexcept; ServerSpanGuard(const ServerSpanGuard&) = delete; ServerSpanGuard& operator=(const ServerSpanGuard&) = delete; From bda835a00ac8ff67dee86ad479347a68b04cb815 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Fri, 1 May 2026 17:15:15 -0400 Subject: [PATCH 06/29] refactor tracing components --- CMakeLists.txt | 2 +- conanfile.py | 7 +- src/viam/sdk/CMakeLists.txt | 8 +- src/viam/sdk/common/client_helper.cpp | 2 +- src/viam/sdk/common/instance.cpp | 2 +- src/viam/sdk/common/instance.hpp | 5 + src/viam/sdk/common/private/instance.hpp | 2 + .../sdk/common/private/service_helper.hpp | 3 +- src/viam/sdk/module/service.cpp | 4 + .../private/span_guard.cpp} | 12 +- .../private/span_guard.hpp} | 4 - src/viam/sdk/tracing/private/tracer.cpp | 120 ++++++++++++++++++ src/viam/sdk/tracing/private/tracer.hpp | 43 +++++++ 13 files changed, 191 insertions(+), 23 deletions(-) rename src/viam/sdk/{common/private/tracing.cpp => tracing/private/span_guard.cpp} (93%) rename src/viam/sdk/{common/private/tracing.hpp => tracing/private/span_guard.hpp} (90%) create mode 100644 src/viam/sdk/tracing/private/tracer.cpp create mode 100644 src/viam/sdk/tracing/private/tracer.hpp diff --git a/CMakeLists.txt b/CMakeLists.txt index be075227c..b3fb6d9e3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -494,7 +494,7 @@ if(VIAMCPPSDK_GRPCXX_VERSION VERSION_LESS 1.43.0) endif() if (VIAMCPPSDK_OPENTELEMETRY_TRACING) - find_package(opentelemetry-cpp CONFIG REQUIRED COMPONENTS sdk) + find_package(opentelemetry-cpp CONFIG REQUIRED COMPONENTS sdk exporters_otlp_grpc) endif() include(FetchContent) diff --git a/conanfile.py b/conanfile.py index e75955bc0..30e73ad3a 100644 --- a/conanfile.py +++ b/conanfile.py @@ -161,9 +161,10 @@ def package_info(self): ]) if self.options.opentelemetry_tracing: - self.cpp_info.components["viamsdk"].requires.append( - "opentelemetry-cpp::opentelemetry_trace" - ) + self.cpp_info.components["viamsdk"].requires.extend([ + "opentelemetry-cpp::opentelemetry_trace", + "opentelemetry-cpp::opentelemetry_exporter_otlp_grpc", + ]) self.cpp_info.components["viamsdk"].requires.extend([ "viam_rust_utils" diff --git a/src/viam/sdk/CMakeLists.txt b/src/viam/sdk/CMakeLists.txt index f9761eb21..68faf0954 100644 --- a/src/viam/sdk/CMakeLists.txt +++ b/src/viam/sdk/CMakeLists.txt @@ -71,7 +71,8 @@ target_sources(viamsdk common/version_metadata.cpp common/world_state.cpp common/private/service_helper.cpp - common/private/tracing.cpp + tracing/private/span_guard.cpp + tracing/private/tracer.cpp components/arm.cpp components/audio_in.cpp components/audio_out.cpp @@ -311,7 +312,10 @@ target_link_libraries(viamsdk if (VIAMCPPSDK_OPENTELEMETRY_TRACING) target_compile_definitions(viamsdk PRIVATE VIAMCPPSDK_OPENTELEMETRY_TRACING) - target_link_libraries(viamsdk PRIVATE opentelemetry-cpp::sdk) + target_link_libraries(viamsdk + PRIVATE opentelemetry-cpp::sdk + PRIVATE opentelemetry-cpp::otlp_grpc_exporter + ) endif() # if the `viam_rust_utils` target exists then we should use it. If not then diff --git a/src/viam/sdk/common/client_helper.cpp b/src/viam/sdk/common/client_helper.cpp index 0449e9cb8..816f79cc8 100644 --- a/src/viam/sdk/common/client_helper.cpp +++ b/src/viam/sdk/common/client_helper.cpp @@ -5,9 +5,9 @@ #include #include -#include #include #include +#include namespace viam { namespace sdk { diff --git a/src/viam/sdk/common/instance.cpp b/src/viam/sdk/common/instance.cpp index 6ea0d4463..cf0d0a982 100644 --- a/src/viam/sdk/common/instance.cpp +++ b/src/viam/sdk/common/instance.cpp @@ -4,8 +4,8 @@ #include #include -#include #include +#include namespace viam { namespace sdk { diff --git a/src/viam/sdk/common/instance.hpp b/src/viam/sdk/common/instance.hpp index de10ae896..979eab9b8 100644 --- a/src/viam/sdk/common/instance.hpp +++ b/src/viam/sdk/common/instance.hpp @@ -5,6 +5,10 @@ namespace viam { namespace sdk { +namespace impl { +class Tracer; +} // namespace impl + /// @brief Instance management for Viam C++ SDK applications. /// This is a single instance class which is responsible for global setup and teardown related to /// the SDK. An Instance must be constructed before doing anything else in a program, and it must @@ -29,6 +33,7 @@ class Instance { private: friend class Registry; friend class LogManager; + friend class impl::Tracer; struct Impl; std::unique_ptr impl_; diff --git a/src/viam/sdk/common/private/instance.hpp b/src/viam/sdk/common/private/instance.hpp index cb09480d9..dc4b176e8 100644 --- a/src/viam/sdk/common/private/instance.hpp +++ b/src/viam/sdk/common/private/instance.hpp @@ -3,6 +3,7 @@ #include #include #include +#include namespace viam { namespace sdk { @@ -10,6 +11,7 @@ namespace sdk { struct Instance::Impl { Registry registry; LogManager log_mgr; + impl::Tracer tracer; }; } // namespace sdk diff --git a/src/viam/sdk/common/private/service_helper.hpp b/src/viam/sdk/common/private/service_helper.hpp index f98b6f73a..fae4262c8 100644 --- a/src/viam/sdk/common/private/service_helper.hpp +++ b/src/viam/sdk/common/private/service_helper.hpp @@ -6,10 +6,9 @@ #include #include -#include - #include #include +#include namespace viam { namespace sdk { diff --git a/src/viam/sdk/module/service.cpp b/src/viam/sdk/module/service.cpp index ad7a61117..fcbf5e5ad 100644 --- a/src/viam/sdk/module/service.cpp +++ b/src/viam/sdk/module/service.cpp @@ -44,6 +44,7 @@ #include #include #include +#include namespace viam { namespace sdk { @@ -210,6 +211,7 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { .set_reconnect_every_interval(std::chrono::seconds{1}); parent.parent_ = RobotClient::at_local_socket(parent.parent_addr_, opts); parent.parent_->connect_logging(); + impl::Tracer::get().initialize_provider(parent.parent_addr_); } } @@ -280,6 +282,8 @@ ModuleService::~ModuleService() { VIAM_SDK_LOG(info) << "Shutting down gracefully."; server_->shutdown(); + impl::Tracer::get().shutdown_provider(); + if (parent_) { try { parent_.reset(); diff --git a/src/viam/sdk/common/private/tracing.cpp b/src/viam/sdk/tracing/private/span_guard.cpp similarity index 93% rename from src/viam/sdk/common/private/tracing.cpp rename to src/viam/sdk/tracing/private/span_guard.cpp index 1e4be4e26..02dafa38d 100644 --- a/src/viam/sdk/common/private/tracing.cpp +++ b/src/viam/sdk/tracing/private/span_guard.cpp @@ -1,10 +1,12 @@ -#include +#include #include #include #ifdef VIAMCPPSDK_OPENTELEMETRY_TRACING +#include + #include #include #include @@ -130,12 +132,6 @@ void inject_trace_context(GrpcClientContext* ctx) noexcept { carrier, otel_ctx::RuntimeContext::GetCurrent()); } -void initialize_trace_propagator() noexcept { - otel_prop::GlobalTextMapPropagator::SetGlobalPropagator( - opentelemetry::nostd::shared_ptr( - new opentelemetry::trace::propagation::HttpTraceContext())); -} - } // namespace impl } // namespace sdk } // namespace viam @@ -158,8 +154,6 @@ ::grpc::Status ServerSpanGuard::commit(::grpc::Status status) noexcept { void inject_trace_context(GrpcClientContext*) noexcept {} -void initialize_trace_propagator() noexcept {} - } // namespace impl } // namespace sdk } // namespace viam diff --git a/src/viam/sdk/common/private/tracing.hpp b/src/viam/sdk/tracing/private/span_guard.hpp similarity index 90% rename from src/viam/sdk/common/private/tracing.hpp rename to src/viam/sdk/tracing/private/span_guard.hpp index 6451c40bb..046a266d1 100644 --- a/src/viam/sdk/common/private/tracing.hpp +++ b/src/viam/sdk/tracing/private/span_guard.hpp @@ -43,10 +43,6 @@ class ServerSpanGuard { /// when no span is active or when OpenTelemetry tracing is not compiled in. void inject_trace_context(GrpcClientContext* ctx) noexcept; -/// @brief Install the W3C Trace Context propagator as the global OpenTelemetry text-map -/// propagator. Called once during @c Instance construction. -void initialize_trace_propagator() noexcept; - } // namespace impl } // namespace sdk } // namespace viam diff --git a/src/viam/sdk/tracing/private/tracer.cpp b/src/viam/sdk/tracing/private/tracer.cpp new file mode 100644 index 000000000..346fa1bf2 --- /dev/null +++ b/src/viam/sdk/tracing/private/tracer.cpp @@ -0,0 +1,120 @@ +#include + +#include +#include + +#ifdef VIAMCPPSDK_OPENTELEMETRY_TRACING + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace otel_prop = opentelemetry::context::propagation; +namespace otel_trace = opentelemetry::trace; +namespace otel_otlp = opentelemetry::exporter::otlp; +namespace otel_sdk_resource = opentelemetry::sdk::resource; +namespace otel_sdk_trace = opentelemetry::sdk::trace; + +namespace viam { +namespace sdk { +namespace impl { + +namespace { + +constexpr const char* k_instrumentation_scope = "viam-cpp-sdk"; + +} // namespace + +void initialize_trace_propagator() noexcept { + otel_prop::GlobalTextMapPropagator::SetGlobalPropagator( + opentelemetry::nostd::shared_ptr( + new opentelemetry::trace::propagation::HttpTraceContext())); +} + +// Holds the SDK provider so Shutdown() can be called explicitly during teardown — the +// global Provider only exposes the abstract trace::TracerProvider interface. +struct Tracer::Impl { + std::shared_ptr sdk_provider; +}; + +Tracer::Tracer() : impl_(std::make_unique()) {} +Tracer::~Tracer() { + shutdown_provider(); +} + +Tracer& Tracer::get() { + return Instance::current(Instance::Creation::open_existing).impl_->tracer; +} + +void Tracer::initialize_provider(const std::string& endpoint) noexcept { + shutdown_provider(); + + otel_otlp::OtlpGrpcExporterOptions exporter_opts; + exporter_opts.endpoint = endpoint; + auto exporter = otel_otlp::OtlpGrpcExporterFactory::Create(exporter_opts); + + auto processor = otel_sdk_trace::BatchSpanProcessorFactory::Create( + std::move(exporter), otel_sdk_trace::BatchSpanProcessorOptions{}); + + auto resource = otel_sdk_resource::Resource::Create({ + {"service.name", std::string{k_instrumentation_scope}}, + }); + + impl_->sdk_provider = std::shared_ptr( + otel_sdk_trace::TracerProviderFactory::Create(std::move(processor), resource)); + + std::shared_ptr base_provider = impl_->sdk_provider; + otel_trace::Provider::SetTracerProvider( + opentelemetry::nostd::shared_ptr(std::move(base_provider))); +} + +void Tracer::shutdown_provider() noexcept { + if (!impl_->sdk_provider) { + return; + } + impl_->sdk_provider->Shutdown(); + otel_trace::Provider::SetTracerProvider( + opentelemetry::nostd::shared_ptr( + new otel_trace::NoopTracerProvider())); + impl_->sdk_provider.reset(); +} + +} // namespace impl +} // namespace sdk +} // namespace viam + +#else // VIAMCPPSDK_OPENTELEMETRY_TRACING — provide no-op implementations + +namespace viam { +namespace sdk { +namespace impl { + +void initialize_trace_propagator() noexcept {} + +struct Tracer::Impl {}; + +Tracer::Tracer() = default; +Tracer::~Tracer() = default; + +Tracer& Tracer::get() { + return Instance::current(Instance::Creation::open_existing).impl_->tracer; +} + +void Tracer::initialize_provider(const std::string&) noexcept {} +void Tracer::shutdown_provider() noexcept {} + +} // namespace impl +} // namespace sdk +} // namespace viam + +#endif // VIAMCPPSDK_OPENTELEMETRY_TRACING diff --git a/src/viam/sdk/tracing/private/tracer.hpp b/src/viam/sdk/tracing/private/tracer.hpp new file mode 100644 index 000000000..ffa6a765e --- /dev/null +++ b/src/viam/sdk/tracing/private/tracer.hpp @@ -0,0 +1,43 @@ +#pragma once + +#include +#include + +namespace viam { +namespace sdk { +namespace impl { + +/// @brief Install the W3C Trace Context propagator as the global OpenTelemetry text-map +/// propagator. Called once during @c Instance construction. +void initialize_trace_propagator() noexcept; + +/// @brief Holds the SDK-side OpenTelemetry tracer provider, owned by @c Instance. +/// Spans created by @c ServerSpanGuard go nowhere until @c initialize_provider has +/// installed an exporter. +class Tracer { + public: + Tracer(); + ~Tracer(); + + Tracer(const Tracer&) = delete; + Tracer& operator=(const Tracer&) = delete; + + /// @brief Returns the @c Tracer owned by the current @c Instance. + static Tracer& get(); + + /// @brief Install a tracer provider that exports spans via OTLP/gRPC to @p endpoint. + /// Replaces any previously-installed provider. No-op when tracing is not compiled in. + void initialize_provider(const std::string& endpoint) noexcept; + + /// @brief Flush and shut down the installed tracer provider. No-op when tracing is + /// not compiled in. + void shutdown_provider() noexcept; + + private: + struct Impl; + std::unique_ptr impl_; +}; + +} // namespace impl +} // namespace sdk +} // namespace viam From 8cdb36960a3f46cd466ed3c89f3123b7f635240b Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Tue, 5 May 2026 16:32:04 -0400 Subject: [PATCH 07/29] fix duplicate registration Co-Authored-By: Claude Sonnet 4.6 --- CMakeLists.txt | 2 +- src/viam/api/CMakeLists.txt | 26 ++++++++++++++++++++------ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index b3fb6d9e3..04aa3cec2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -494,7 +494,7 @@ if(VIAMCPPSDK_GRPCXX_VERSION VERSION_LESS 1.43.0) endif() if (VIAMCPPSDK_OPENTELEMETRY_TRACING) - find_package(opentelemetry-cpp CONFIG REQUIRED COMPONENTS sdk exporters_otlp_grpc) + find_package(opentelemetry-cpp CONFIG REQUIRED) endif() include(FetchContent) diff --git a/src/viam/api/CMakeLists.txt b/src/viam/api/CMakeLists.txt index 50f297819..c310ee1ce 100644 --- a/src/viam/api/CMakeLists.txt +++ b/src/viam/api/CMakeLists.txt @@ -361,12 +361,6 @@ target_sources(viamapi ${PROTO_GEN_DIR}/google/api/http.pb.cc ${PROTO_GEN_DIR}/google/api/httpbody.pb.cc ${PROTO_GEN_DIR}/google/rpc/status.pb.cc - ${PROTO_GEN_DIR}/opentelemetry/proto/common/v1/common.grpc.pb.cc - ${PROTO_GEN_DIR}/opentelemetry/proto/common/v1/common.pb.cc - ${PROTO_GEN_DIR}/opentelemetry/proto/resource/v1/resource.grpc.pb.cc - ${PROTO_GEN_DIR}/opentelemetry/proto/resource/v1/resource.pb.cc - ${PROTO_GEN_DIR}/opentelemetry/proto/trace/v1/trace.grpc.pb.cc - ${PROTO_GEN_DIR}/opentelemetry/proto/trace/v1/trace.pb.cc ${PROTO_GEN_DIR}/module/v1/module.grpc.pb.cc ${PROTO_GEN_DIR}/module/v1/module.pb.cc ${PROTO_GEN_DIR}/robot/v1/robot.grpc.pb.cc @@ -499,6 +493,26 @@ target_link_libraries(viamapi PRIVATE Threads::Threads ) +# `robot.proto` imports `opentelemetry/proto/trace/v1/trace.proto`, so the +# OTel proto descriptors are part of viamapi's ABI. When tracing is enabled, +# get them from the system `opentelemetry-cpp::proto` library to avoid +# double-registering them alongside `opentelemetry-cpp::otlp_grpc_exporter` +# (which links the same library transitively into viamsdk). When tracing is +# disabled, compile our locally-generated copies into viamapi instead. +if (VIAMCPPSDK_OPENTELEMETRY_TRACING) + target_link_libraries(viamapi PUBLIC opentelemetry-cpp::proto) +else() + target_sources(viamapi + PRIVATE + ${PROTO_GEN_DIR}/opentelemetry/proto/common/v1/common.grpc.pb.cc + ${PROTO_GEN_DIR}/opentelemetry/proto/common/v1/common.pb.cc + ${PROTO_GEN_DIR}/opentelemetry/proto/resource/v1/resource.grpc.pb.cc + ${PROTO_GEN_DIR}/opentelemetry/proto/resource/v1/resource.pb.cc + ${PROTO_GEN_DIR}/opentelemetry/proto/trace/v1/trace.grpc.pb.cc + ${PROTO_GEN_DIR}/opentelemetry/proto/trace/v1/trace.pb.cc + ) +endif() + install( TARGETS viamapi EXPORT viamapi From a21823fb81be2b3df53e209e8286a2e972bc2875 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Wed, 6 May 2026 18:34:55 -0400 Subject: [PATCH 08/29] fix url --- src/viam/sdk/tracing/private/tracer.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/viam/sdk/tracing/private/tracer.cpp b/src/viam/sdk/tracing/private/tracer.cpp index 346fa1bf2..3658322b6 100644 --- a/src/viam/sdk/tracing/private/tracer.cpp +++ b/src/viam/sdk/tracing/private/tracer.cpp @@ -59,8 +59,12 @@ Tracer& Tracer::get() { void Tracer::initialize_provider(const std::string& endpoint) noexcept { shutdown_provider(); + // the incoming endpoint is unix:/path/to/socket but opentel's url parser expects + // a :// for protocol + std::string ep = "unix://" + endpoint.substr(endpoint.find('/')); + otel_otlp::OtlpGrpcExporterOptions exporter_opts; - exporter_opts.endpoint = endpoint; + exporter_opts.endpoint = ep; auto exporter = otel_otlp::OtlpGrpcExporterFactory::Create(exporter_opts); auto processor = otel_sdk_trace::BatchSpanProcessorFactory::Create( From e0e4d5288824302d79febf20653df18d3f17c17b Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Thu, 7 May 2026 16:29:24 -0400 Subject: [PATCH 09/29] add sendtraces exporter Co-Authored-By: Claude Sonnet 4.6 --- src/viam/sdk/module/service.cpp | 2 +- src/viam/sdk/robot/client.cpp | 15 +++++ src/viam/sdk/robot/client.hpp | 12 +++- src/viam/sdk/tracing/private/tracer.cpp | 76 ++++++++++++++++++++----- src/viam/sdk/tracing/private/tracer.hpp | 21 +++---- 5 files changed, 99 insertions(+), 27 deletions(-) diff --git a/src/viam/sdk/module/service.cpp b/src/viam/sdk/module/service.cpp index fcbf5e5ad..cc55cb4b2 100644 --- a/src/viam/sdk/module/service.cpp +++ b/src/viam/sdk/module/service.cpp @@ -211,7 +211,7 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { .set_reconnect_every_interval(std::chrono::seconds{1}); parent.parent_ = RobotClient::at_local_socket(parent.parent_addr_, opts); parent.parent_->connect_logging(); - impl::Tracer::get().initialize_provider(parent.parent_addr_); + parent.parent_->connect_tracing(); } } diff --git a/src/viam/sdk/robot/client.cpp b/src/viam/sdk/robot/client.cpp index 67e9697ab..21ca1c936 100644 --- a/src/viam/sdk/robot/client.cpp +++ b/src/viam/sdk/robot/client.cpp @@ -30,6 +30,7 @@ #include #include #include +#include namespace viam { namespace sdk { @@ -97,6 +98,7 @@ struct RobotClient::impl { boost::log::core::get()->remove_sink(log_sink); LogManager::get().enable_console_logging(); } + sdk::impl::Tracer::get().shutdown_provider(); } template @@ -336,6 +338,19 @@ void RobotClient::log(const std::string& name, } } +bool RobotClient::send_traces(const robot::v1::SendTracesRequest* req) { + if (!impl_) { + return false; + } + robot::v1::SendTracesResponse resp; + ClientContext ctx; + return impl_->stub->SendTraces(ctx, *req, &resp).ok(); +} + +void RobotClient::connect_tracing() { + sdk::impl::Tracer::get().initialize_provider(this); +} + std::shared_ptr RobotClient::with_channel(ViamChannel channel, const Options& options) { auto robot = std::make_shared(std::move(channel)); diff --git a/src/viam/sdk/robot/client.hpp b/src/viam/sdk/robot/client.hpp index 519569e56..8b5e0dfdb 100644 --- a/src/viam/sdk/robot/client.hpp +++ b/src/viam/sdk/robot/client.hpp @@ -25,6 +25,7 @@ namespace v1 { class FrameSystemConfig; class Operation; +class SendTracesRequest; } // namespace v1 } // namespace robot @@ -33,7 +34,8 @@ namespace sdk { namespace impl { struct LogBackend; -} +class ParentSendTracesExporter; +} // namespace impl /// @defgroup Robot Classes related to a Robot representation. @@ -186,12 +188,16 @@ class RobotClient { private: friend class ModuleService; friend struct impl::LogBackend; + friend class impl::ParentSendTracesExporter; void log(const std::string& name, const std::string& level, const std::string& message, time_pt time); + // Ships a batch of OTLP traces to the parent. Returns true on success. + bool send_traces(const robot::v1::SendTracesRequest* req); + // Makes this RobotClient manage logging by sending logs over grpc to viam-server. // This is private and only ever called by ModuleService; in other words it is only called when // viam-server is running a Viam C++ SDK application as a module. @@ -199,6 +205,10 @@ class RobotClient { // re-enabled on destruction. void connect_logging(); + // Installs the SDK tracer provider so spans flow back to the parent over this connection. + // Only called by ModuleService when running as a module. + void connect_tracing(); + void refresh_every(); void check_connection(); diff --git a/src/viam/sdk/tracing/private/tracer.cpp b/src/viam/sdk/tracing/private/tracer.cpp index 3658322b6..eb5467f98 100644 --- a/src/viam/sdk/tracing/private/tracer.cpp +++ b/src/viam/sdk/tracing/private/tracer.cpp @@ -5,20 +5,31 @@ #ifdef VIAMCPPSDK_OPENTELEMETRY_TRACING +#include +#include #include +#include #include -#include -#include +#include +#include +#include +#include #include #include #include +#include +#include #include #include #include #include #include +#include +#include + +namespace otel_common = opentelemetry::sdk::common; namespace otel_prop = opentelemetry::context::propagation; namespace otel_trace = opentelemetry::trace; namespace otel_otlp = opentelemetry::exporter::otlp; @@ -35,14 +46,55 @@ constexpr const char* k_instrumentation_scope = "viam-cpp-sdk"; } // namespace +// Ships OTLP-encoded spans to the parent process via RobotClient::send_traces. +class ParentSendTracesExporter final : public otel_sdk_trace::SpanExporter { + public: + explicit ParentSendTracesExporter(RobotClient* client) noexcept : client_(client) {} + + std::unique_ptr MakeRecordable() noexcept override { + return std::unique_ptr(new otel_otlp::OtlpRecordable()); + } + + otel_common::ExportResult Export( + const opentelemetry::nostd::span>& + spans) noexcept override { + if (is_shutdown_.load(std::memory_order_acquire)) { + return otel_common::ExportResult::kFailure; + } + if (spans.empty()) { + return otel_common::ExportResult::kSuccess; + } + + opentelemetry::proto::collector::trace::v1::ExportTraceServiceRequest otlp_req; + otel_otlp::OtlpRecordableUtils::PopulateRequest(spans, &otlp_req); + + viam::robot::v1::SendTracesRequest req; + req.mutable_resource_spans()->Swap(otlp_req.mutable_resource_spans()); + + return client_->send_traces(&req) ? otel_common::ExportResult::kSuccess + : otel_common::ExportResult::kFailure; + } + + bool ForceFlush(std::chrono::microseconds /*timeout*/) noexcept override { + return true; + } + + bool Shutdown(std::chrono::microseconds /*timeout*/) noexcept override { + is_shutdown_.store(true, std::memory_order_release); + return true; + } + + private: + RobotClient* client_; + std::atomic is_shutdown_{false}; +}; + void initialize_trace_propagator() noexcept { otel_prop::GlobalTextMapPropagator::SetGlobalPropagator( opentelemetry::nostd::shared_ptr( new opentelemetry::trace::propagation::HttpTraceContext())); } -// Holds the SDK provider so Shutdown() can be called explicitly during teardown — the -// global Provider only exposes the abstract trace::TracerProvider interface. struct Tracer::Impl { std::shared_ptr sdk_provider; }; @@ -56,16 +108,11 @@ Tracer& Tracer::get() { return Instance::current(Instance::Creation::open_existing).impl_->tracer; } -void Tracer::initialize_provider(const std::string& endpoint) noexcept { +void Tracer::initialize_provider(RobotClient* client) noexcept { shutdown_provider(); - // the incoming endpoint is unix:/path/to/socket but opentel's url parser expects - // a :// for protocol - std::string ep = "unix://" + endpoint.substr(endpoint.find('/')); - - otel_otlp::OtlpGrpcExporterOptions exporter_opts; - exporter_opts.endpoint = ep; - auto exporter = otel_otlp::OtlpGrpcExporterFactory::Create(exporter_opts); + auto exporter = + std::unique_ptr(new ParentSendTracesExporter(client)); auto processor = otel_sdk_trace::BatchSpanProcessorFactory::Create( std::move(exporter), otel_sdk_trace::BatchSpanProcessorOptions{}); @@ -101,6 +148,9 @@ void Tracer::shutdown_provider() noexcept { namespace viam { namespace sdk { + +class RobotClient; + namespace impl { void initialize_trace_propagator() noexcept {} @@ -114,7 +164,7 @@ Tracer& Tracer::get() { return Instance::current(Instance::Creation::open_existing).impl_->tracer; } -void Tracer::initialize_provider(const std::string&) noexcept {} +void Tracer::initialize_provider(RobotClient*) noexcept {} void Tracer::shutdown_provider() noexcept {} } // namespace impl diff --git a/src/viam/sdk/tracing/private/tracer.hpp b/src/viam/sdk/tracing/private/tracer.hpp index ffa6a765e..99b3a610a 100644 --- a/src/viam/sdk/tracing/private/tracer.hpp +++ b/src/viam/sdk/tracing/private/tracer.hpp @@ -1,19 +1,19 @@ #pragma once #include -#include namespace viam { namespace sdk { + +class RobotClient; + namespace impl { -/// @brief Install the W3C Trace Context propagator as the global OpenTelemetry text-map -/// propagator. Called once during @c Instance construction. +/// @brief Install the W3C Trace Context propagator. Called once during @c Instance construction. void initialize_trace_propagator() noexcept; -/// @brief Holds the SDK-side OpenTelemetry tracer provider, owned by @c Instance. -/// Spans created by @c ServerSpanGuard go nowhere until @c initialize_provider has -/// installed an exporter. +/// @brief Holds the SDK-side OpenTelemetry tracer provider. Spans go nowhere until +/// @c initialize_provider installs an exporter that ships them to the parent. class Tracer { public: Tracer(); @@ -25,12 +25,9 @@ class Tracer { /// @brief Returns the @c Tracer owned by the current @c Instance. static Tracer& get(); - /// @brief Install a tracer provider that exports spans via OTLP/gRPC to @p endpoint. - /// Replaces any previously-installed provider. No-op when tracing is not compiled in. - void initialize_provider(const std::string& endpoint) noexcept; - - /// @brief Flush and shut down the installed tracer provider. No-op when tracing is - /// not compiled in. + /// @brief Install an exporter sourced from @p client. The caller must keep @p client alive + /// until @c shutdown_provider returns. + void initialize_provider(RobotClient* client) noexcept; void shutdown_provider() noexcept; private: From 05e7293957089762b3cd047381a85dd884cd938d Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Fri, 8 May 2026 14:07:48 -0400 Subject: [PATCH 10/29] add env var false helper --- src/viam/sdk/common/utils.cpp | 14 ++++++++++++++ src/viam/sdk/common/utils.hpp | 4 ++++ 2 files changed, 18 insertions(+) diff --git a/src/viam/sdk/common/utils.cpp b/src/viam/sdk/common/utils.cpp index 15eaa7c62..c19aa1d67 100644 --- a/src/viam/sdk/common/utils.cpp +++ b/src/viam/sdk/common/utils.cpp @@ -190,5 +190,19 @@ bool is_env_var_true(const char* var) { return false; } +bool is_env_var_false(const char* var) { + if (const auto& val = get_env(var)) { + for (const char* untruth : {"false", "no", "0", "FALSE", "NO"}) { + if (*val == untruth) { + return true; + } + } + + return false; + } else { + return true; + } +} + } // namespace sdk } // namespace viam diff --git a/src/viam/sdk/common/utils.hpp b/src/viam/sdk/common/utils.hpp index 747e91a55..7c8a7229e 100644 --- a/src/viam/sdk/common/utils.hpp +++ b/src/viam/sdk/common/utils.hpp @@ -125,5 +125,9 @@ boost::optional get_env(const char* var); /// "true", "yes", "1", "TRUE", or "YES" bool is_env_var_true(const char* var); +/// @brief Returns whether the environment variable named @param var is unset, or set and equal to +/// "false", "no", "0", "FALSE", or "NO" +bool is_env_var_false(const char* var); + } // namespace sdk } // namespace viam From c09d61268e301584b2d3886d956ba12c486f7763 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Fri, 8 May 2026 14:08:14 -0400 Subject: [PATCH 11/29] do runtime check to not initialize env var Co-Authored-By: Claude Sonnet 4.6 --- src/viam/sdk/tracing/private/tracer.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/viam/sdk/tracing/private/tracer.cpp b/src/viam/sdk/tracing/private/tracer.cpp index eb5467f98..82b9d6ca4 100644 --- a/src/viam/sdk/tracing/private/tracer.cpp +++ b/src/viam/sdk/tracing/private/tracer.cpp @@ -2,6 +2,7 @@ #include #include +#include #ifdef VIAMCPPSDK_OPENTELEMETRY_TRACING @@ -111,6 +112,10 @@ Tracer& Tracer::get() { void Tracer::initialize_provider(RobotClient* client) noexcept { shutdown_provider(); + if (is_env_var_false("VIAM_MODULE_TRACING")) { + return; + } + auto exporter = std::unique_ptr(new ParentSendTracesExporter(client)); From 7dac5e0a00c7e75a651006d4acec89a8f1b991bc Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Fri, 8 May 2026 15:38:00 -0400 Subject: [PATCH 12/29] opentel is conan only off by default --- CMakeLists.txt | 4 ++-- conanfile.py | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 04aa3cec2..9756c3d21 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -135,10 +135,10 @@ option(VIAMCPPSDK_CLANG_TIDY "Run the clang-tidy linter" OFF) # - `VIAMCPPSDK_OPENTELEMETRY_TRACING` # -# When enabled (default), links against opentelemetry-cpp and compiles in +# When enabled, links against opentelemetry-cpp and compiles in # W3C Trace Context propagation for all gRPC client and server calls. # -option(VIAMCPPSDK_OPENTELEMETRY_TRACING "Compile OpenTelemetry tracing into all gRPC calls" ON) +option(VIAMCPPSDK_OPENTELEMETRY_TRACING "Compile OpenTelemetry tracing into all gRPC calls" OFF) # The following options are only defined if this project is not being included as a subproject if (CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR) diff --git a/conanfile.py b/conanfile.py index 30e73ad3a..02737124a 100644 --- a/conanfile.py +++ b/conanfile.py @@ -24,7 +24,7 @@ class ViamCppSdkRecipe(ConanFile): default_options = { "offline_proto_generation": True, - "opentelemetry_tracing": True, + "opentelemetry_tracing": False, "shared": True } @@ -71,7 +71,8 @@ def requirements(self): self.requires(self._xtensor_requires(), transitive_headers=True) if self.options.opentelemetry_tracing: - self.requires('opentelemetry-cpp/[>=1.9.0]') + # Oldest maintained conan package and first version with proper CMake support + self.requires('opentelemetry-cpp/[>=1.21.0]') def build_requirements(self): if self.options.offline_proto_generation: @@ -162,7 +163,7 @@ def package_info(self): if self.options.opentelemetry_tracing: self.cpp_info.components["viamsdk"].requires.extend([ - "opentelemetry-cpp::opentelemetry_trace", + "opentelemetry-cpp::opentelemetry_sdk", "opentelemetry-cpp::opentelemetry_exporter_otlp_grpc", ]) From 0496ac0a15bc6555b6d3e54e57dd25e4d0369a31 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Fri, 8 May 2026 15:43:37 -0400 Subject: [PATCH 13/29] explain more --- CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index 9756c3d21..95cad8a6d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -137,6 +137,7 @@ option(VIAMCPPSDK_CLANG_TIDY "Run the clang-tidy linter" OFF) # # When enabled, links against opentelemetry-cpp and compiles in # W3C Trace Context propagation for all gRPC client and server calls. +# This is off by default and effectively conan-only, because apt packages are not generally available. # option(VIAMCPPSDK_OPENTELEMETRY_TRACING "Compile OpenTelemetry tracing into all gRPC calls" OFF) From b7efc9ddbee5ae83f29fcb6af7c53c103b789eb5 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Fri, 8 May 2026 15:45:24 -0400 Subject: [PATCH 14/29] conan default --- conanfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conanfile.py b/conanfile.py index 02737124a..21af693bf 100644 --- a/conanfile.py +++ b/conanfile.py @@ -24,7 +24,7 @@ class ViamCppSdkRecipe(ConanFile): default_options = { "offline_proto_generation": True, - "opentelemetry_tracing": False, + "opentelemetry_tracing": True, "shared": True } From 26fed90f8f654378afbd38b8cdd8317fa6bbe64c Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Fri, 8 May 2026 15:46:04 -0400 Subject: [PATCH 15/29] space --- src/viam/sdk/common/private/service_helper.hpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/viam/sdk/common/private/service_helper.hpp b/src/viam/sdk/common/private/service_helper.hpp index fae4262c8..799335ca2 100644 --- a/src/viam/sdk/common/private/service_helper.hpp +++ b/src/viam/sdk/common/private/service_helper.hpp @@ -6,6 +6,7 @@ #include #include + #include #include #include @@ -30,7 +31,9 @@ class ServiceHelperBase { protected: explicit ServiceHelperBase(const char* method) noexcept : method_{method} {} - const char* method_name() const noexcept { return method_; } + const char* method_name() const noexcept { + return method_; + } private: const char* method_; From 56db4582181847c95323a150a809cc6ff9169bd9 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Mon, 11 May 2026 16:05:53 -0400 Subject: [PATCH 16/29] nolint the no-op impl --- conanfile.py | 13 ++++++++++++- src/viam/sdk/CMakeLists.txt | 4 ++-- src/viam/sdk/tracing/private/span_guard.cpp | 3 ++- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/conanfile.py b/conanfile.py index 35d8fa716..2de8bc230 100644 --- a/conanfile.py +++ b/conanfile.py @@ -1,4 +1,5 @@ from conan import ConanFile +from conan.errors import ConanInvalidConfiguration from conan.tools.cmake import CMake, CMakeDeps, CMakeToolchain, cmake_layout from conan.tools.build import valid_max_cppstd from conan.tools.files import load @@ -38,6 +39,9 @@ def configure(self): # Workaround an unfortunately long-standing boost/conan issue which breaks C++20 builds self.options["boost"].without_cobalt = True + if self.options.opentelemetry_tracing: + self.options["opentelemetry-cpp"].with_otlp_grpc = True + if self.options.shared: # See https://github.com/conan-io/conan-center-index/issues/25107 self.options["grpc"].secure = True @@ -48,6 +52,13 @@ def configure(self): for lib in ["grpc", "protobuf", "abseil"]: self.options[lib].shared = True + def validate(self): + if self.options.opentelemetry_tracing: + if not self.dependencies["opentelemetry-cpp"].options.with_otlp_grpc: + raise ConanInvalidConfiguration("opentelemetry_tracing option requires opentelemetry-cpp/*:with_otlp_grpc") + + + def _xtensor_requires(self): if valid_max_cppstd(self, 14, False): return 'xtensor/[>=0.24.3 <0.26.0]' @@ -166,7 +177,7 @@ def package_info(self): if self.options.opentelemetry_tracing: self.cpp_info.components["viamsdk"].requires.extend([ - "opentelemetry-cpp::opentelemetry_sdk", + "opentelemetry-cpp::opentelemetry_trace", "opentelemetry-cpp::opentelemetry_exporter_otlp_grpc", ]) diff --git a/src/viam/sdk/CMakeLists.txt b/src/viam/sdk/CMakeLists.txt index 68faf0954..e349cbe91 100644 --- a/src/viam/sdk/CMakeLists.txt +++ b/src/viam/sdk/CMakeLists.txt @@ -313,8 +313,8 @@ target_link_libraries(viamsdk if (VIAMCPPSDK_OPENTELEMETRY_TRACING) target_compile_definitions(viamsdk PRIVATE VIAMCPPSDK_OPENTELEMETRY_TRACING) target_link_libraries(viamsdk - PRIVATE opentelemetry-cpp::sdk - PRIVATE opentelemetry-cpp::otlp_grpc_exporter + PRIVATE opentelemetry-cpp::trace + PRIVATE opentelemetry-cpp::exporter_otlp_grpc ) endif() diff --git a/src/viam/sdk/tracing/private/span_guard.cpp b/src/viam/sdk/tracing/private/span_guard.cpp index 02dafa38d..7bde2aaa3 100644 --- a/src/viam/sdk/tracing/private/span_guard.cpp +++ b/src/viam/sdk/tracing/private/span_guard.cpp @@ -148,7 +148,8 @@ ServerSpanGuard::ServerSpanGuard(const GrpcServerContext*, const char*) noexcept ServerSpanGuard::~ServerSpanGuard() noexcept = default; -::grpc::Status ServerSpanGuard::commit(::grpc::Status status) noexcept { +::grpc::Status ServerSpanGuard::commit( + ::grpc::Status status) noexcept { // NOLINT(readability-convert-member-functions-to-static) return status; } From 87ce7c574d517a81ad1154a3ef1404ab2ecb8d56 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Mon, 11 May 2026 16:14:25 -0400 Subject: [PATCH 17/29] rewrite truth/false tests --- src/viam/sdk/common/utils.cpp | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/viam/sdk/common/utils.cpp b/src/viam/sdk/common/utils.cpp index c19aa1d67..1d30dec7e 100644 --- a/src/viam/sdk/common/utils.cpp +++ b/src/viam/sdk/common/utils.cpp @@ -179,26 +179,26 @@ boost::optional get_env(const char* var) { } bool is_env_var_true(const char* var) { + static constexpr const std::array truth_vals{ + {"true", "yes", "1", "TRUE", "YES"}}; + if (const auto& val = get_env(var)) { - for (const char* truth : {"true", "yes", "1", "TRUE", "YES"}) { - if (*val == truth) { - return true; - } - } + return std::any_of(truth_vals.begin(), truth_vals.end(), [&val](const char* truth) { + return *val == truth; + }); } return false; } bool is_env_var_false(const char* var) { - if (const auto& val = get_env(var)) { - for (const char* untruth : {"false", "no", "0", "FALSE", "NO"}) { - if (*val == untruth) { - return true; - } - } + static constexpr const std::array false_vals{ + {"false", "no", "0", "FALSE", "NO"}}; - return false; + if (const auto& val = get_env(var)) { + return std::any_of(false_vals.begin(), false_vals.end(), [&val](const char* untruth) { + return *val == untruth; + }); } else { return true; } From 4497008e8cad7ba46254624e69f818f8f945cf43 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Mon, 11 May 2026 16:26:07 -0400 Subject: [PATCH 18/29] move the nolint --- src/viam/sdk/tracing/private/span_guard.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/viam/sdk/tracing/private/span_guard.cpp b/src/viam/sdk/tracing/private/span_guard.cpp index 7bde2aaa3..218e7d4fd 100644 --- a/src/viam/sdk/tracing/private/span_guard.cpp +++ b/src/viam/sdk/tracing/private/span_guard.cpp @@ -148,8 +148,8 @@ ServerSpanGuard::ServerSpanGuard(const GrpcServerContext*, const char*) noexcept ServerSpanGuard::~ServerSpanGuard() noexcept = default; -::grpc::Status ServerSpanGuard::commit( - ::grpc::Status status) noexcept { // NOLINT(readability-convert-member-functions-to-static) +::grpc::Status ServerSpanGuard::commit( // NOLINT(readability-convert-member-functions-to-static) + ::grpc::Status status) noexcept { return status; } From 058d74a7eff00e0efc174b0bb88890318d33d121 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Mon, 11 May 2026 16:49:41 -0400 Subject: [PATCH 19/29] disable opentel on windows PR builds --- .github/workflows/test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 8ed58ab62..cf4dc5842 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -130,7 +130,7 @@ jobs: Import-Module $env:ChocolateyInstall\helpers\chocolateyProfile.psm1 refreshenv conan profile detect - conan install . --build=missing -o "&:shared=False" + conan install . --build=missing -o "&:shared=False" -o "&:opentelemetry_tracing=False" cmake . --preset conan-default -DVIAMCPPSDK_BUILD_EXAMPLES=ON cmake --build --preset=conan-release --target ALL_BUILD install -j 8 env: From fde2120f18d4a85b2164a183f23d884c683c392f Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Tue, 12 May 2026 12:19:25 -0400 Subject: [PATCH 20/29] conan build fixes Co-Authored-By: Claude Opus 4.7 --- conanfile.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/conanfile.py b/conanfile.py index 2de8bc230..54e48ff2a 100644 --- a/conanfile.py +++ b/conanfile.py @@ -41,6 +41,7 @@ def configure(self): if self.options.opentelemetry_tracing: self.options["opentelemetry-cpp"].with_otlp_grpc = True + self.options["opentelemetry-cpp"].with_otlp_http = False if self.options.shared: # See https://github.com/conan-io/conan-center-index/issues/25107 @@ -148,6 +149,11 @@ def package_info(self): self.cpp_info.components["viamapi"].includedirs.append("include/viam/api") + if self.options.opentelemetry_tracing: + self.cpp_info.components["viamapi"].requires.append( + "opentelemetry-cpp::proto" + ) + if self.options.shared: self.cpp_info.components["viamapi"].libs = ["viamapi"] else: From 8929c0ba23a5e183353a8d6329174d185726da68 Mon Sep 17 00:00:00 2001 From: lia <167905060+lia-viam@users.noreply.github.com> Date: Tue, 12 May 2026 13:02:01 -0400 Subject: [PATCH 21/29] Update conanfile.py --- conanfile.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conanfile.py b/conanfile.py index 54e48ff2a..a5f26cf87 100644 --- a/conanfile.py +++ b/conanfile.py @@ -151,7 +151,7 @@ def package_info(self): if self.options.opentelemetry_tracing: self.cpp_info.components["viamapi"].requires.append( - "opentelemetry-cpp::proto" + "opentelemetry-cpp::opentelemetry_proto" ) if self.options.shared: From 94cc725461ac76c71dfd24f416291e7df36ca21b Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Tue, 12 May 2026 18:27:34 -0400 Subject: [PATCH 22/29] conan changes Co-Authored-By: Claude Opus 4.7 --- conanfile.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/conanfile.py b/conanfile.py index a5f26cf87..c5614a80d 100644 --- a/conanfile.py +++ b/conanfile.py @@ -41,7 +41,15 @@ def configure(self): if self.options.opentelemetry_tracing: self.options["opentelemetry-cpp"].with_otlp_grpc = True + # Disable every HTTP-based exporter so libcurl never enters the + # dependency graph; we only use OTLP/gRPC. self.options["opentelemetry-cpp"].with_otlp_http = False + self.options["opentelemetry-cpp"].with_zipkin = False + self.options["opentelemetry-cpp"].with_elasticsearch = False + # Match opentelemetry-cpp's shared-ness to the SDK to avoid DLL + # export mismatches for the protobuf-generated symbols in + # opentelemetry_proto on Windows. + self.options["opentelemetry-cpp"].shared = self.options.shared if self.options.shared: # See https://github.com/conan-io/conan-center-index/issues/25107 From 8f95e33ed0487b287b9e94de0e09294ce3968440 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Wed, 13 May 2026 08:35:27 -0400 Subject: [PATCH 23/29] remove shared match --- conanfile.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/conanfile.py b/conanfile.py index c5614a80d..73b53abf6 100644 --- a/conanfile.py +++ b/conanfile.py @@ -46,10 +46,6 @@ def configure(self): self.options["opentelemetry-cpp"].with_otlp_http = False self.options["opentelemetry-cpp"].with_zipkin = False self.options["opentelemetry-cpp"].with_elasticsearch = False - # Match opentelemetry-cpp's shared-ness to the SDK to avoid DLL - # export mismatches for the protobuf-generated symbols in - # opentelemetry_proto on Windows. - self.options["opentelemetry-cpp"].shared = self.options.shared if self.options.shared: # See https://github.com/conan-io/conan-center-index/issues/25107 From d21e6b78b5cd8dda6e38f6de966cc812591d0670 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Wed, 13 May 2026 09:45:21 -0400 Subject: [PATCH 24/29] unconditionally match sharedness Co-Authored-By: Claude Opus 4.7 --- conanfile.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/conanfile.py b/conanfile.py index 73b53abf6..9165ae14f 100644 --- a/conanfile.py +++ b/conanfile.py @@ -51,11 +51,11 @@ def configure(self): # See https://github.com/conan-io/conan-center-index/issues/25107 self.options["grpc"].secure = True - # From some experiments it seems that the shared-ness of these packages - # should match that of the SDK recipe. Failure to do so can cause linker - # errors while compiling, or static initialization errors at runtime for modules. - for lib in ["grpc", "protobuf", "abseil"]: - self.options[lib].shared = True + # From some experiments it seems that the shared-ness of these packages + # should match that of the SDK recipe. Failure to do so can cause linker + # errors while compiling, or static initialization errors at runtime for modules. + for lib in ["grpc", "protobuf", "abseil"]: + self.options[lib].shared = self.options.shared def validate(self): if self.options.opentelemetry_tracing: From 495e267080225c1992edf110f946be9cea6e95ae Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Wed, 13 May 2026 10:20:40 -0400 Subject: [PATCH 25/29] revert shared block and bound windows Co-Authored-By: Claude Opus 4.7 --- conanfile.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/conanfile.py b/conanfile.py index 9165ae14f..4a394976a 100644 --- a/conanfile.py +++ b/conanfile.py @@ -51,11 +51,11 @@ def configure(self): # See https://github.com/conan-io/conan-center-index/issues/25107 self.options["grpc"].secure = True - # From some experiments it seems that the shared-ness of these packages - # should match that of the SDK recipe. Failure to do so can cause linker - # errors while compiling, or static initialization errors at runtime for modules. - for lib in ["grpc", "protobuf", "abseil"]: - self.options[lib].shared = self.options.shared + # From some experiments it seems that the shared-ness of these packages + # should match that of the SDK recipe. Failure to do so can cause linker + # errors while compiling, or static initialization errors at runtime for modules. + for lib in ["grpc", "protobuf", "abseil"]: + self.options[lib].shared = True def validate(self): if self.options.opentelemetry_tracing: @@ -91,7 +91,12 @@ def requirements(self): if self.options.opentelemetry_tracing: # Oldest maintained conan package and first version with proper CMake support - self.requires('opentelemetry-cpp/[>=1.21.0]') + if self.settings.os == "Windows": + # v1.22+ builds opentelemetry_proto as a DLL on Windows without + # exporting its protobuf-generated globals, breaking consumer link. + self.requires('opentelemetry-cpp/[>=1.21.0 <1.22.0]') + else: + self.requires('opentelemetry-cpp/[>=1.21.0]') def build_requirements(self): if self.options.offline_proto_generation: From b93360c3f2e6a93ec82eb063a42d6db52e68e02e Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Wed, 13 May 2026 10:29:56 -0400 Subject: [PATCH 26/29] higher range Co-Authored-By: Claude Opus 4.7 --- conanfile.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/conanfile.py b/conanfile.py index 4a394976a..c88c601f5 100644 --- a/conanfile.py +++ b/conanfile.py @@ -92,9 +92,9 @@ def requirements(self): if self.options.opentelemetry_tracing: # Oldest maintained conan package and first version with proper CMake support if self.settings.os == "Windows": - # v1.22+ builds opentelemetry_proto as a DLL on Windows without + # v1.25+ builds opentelemetry_proto as a DLL on Windows without # exporting its protobuf-generated globals, breaking consumer link. - self.requires('opentelemetry-cpp/[>=1.21.0 <1.22.0]') + self.requires('opentelemetry-cpp/[>=1.21.0 <1.25.0]') else: self.requires('opentelemetry-cpp/[>=1.21.0]') From cea40821f3820de138cf7eceb8e7ce1d70a35da0 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Thu, 14 May 2026 16:43:44 -0400 Subject: [PATCH 27/29] fix use cases not covered by helpers Co-Authored-By: Claude Opus 4.7 --- .../sdk/components/private/board_server.cpp | 34 ++++++---- src/viam/sdk/module/service.cpp | 63 +++++++++++-------- 2 files changed, 60 insertions(+), 37 deletions(-) diff --git a/src/viam/sdk/components/private/board_server.cpp b/src/viam/sdk/components/private/board_server.cpp index 223b85efc..788b6a884 100644 --- a/src/viam/sdk/components/private/board_server.cpp +++ b/src/viam/sdk/components/private/board_server.cpp @@ -117,14 +117,17 @@ ::grpc::Status BoardServer::ReadAnalogReader( ::grpc::ServerContext* context, const ::viam::component::board::v1::ReadAnalogReaderRequest* request, ::viam::component::board::v1::ReadAnalogReaderResponse* response) { + ServerSpanGuard span_guard{context, "BoardServer::ReadAnalogReader"}; + if (!request) { - return ::grpc::Status(::grpc::StatusCode::INVALID_ARGUMENT, - "Called [Board::ReadAnalogReader] without a request"); + return span_guard.commit(::grpc::Status(::grpc::StatusCode::INVALID_ARGUMENT, + "Called [Board::ReadAnalogReader] without a request")); }; const std::shared_ptr rb = resource_manager()->resource(request->board_name()); if (!rb) { - return grpc::Status(grpc::UNKNOWN, "resource not found: " + request->board_name()); + return span_guard.commit( + grpc::Status(grpc::UNKNOWN, "resource not found: " + request->board_name())); } const std::shared_ptr board = std::dynamic_pointer_cast(rb); @@ -141,21 +144,24 @@ ::grpc::Status BoardServer::ReadAnalogReader( response->set_max_range(result.max_range); response->set_step_size(result.step_size); - return ::grpc::Status(); + return span_guard.commit(::grpc::Status()); } ::grpc::Status BoardServer::WriteAnalog( ::grpc::ServerContext* context, const ::viam::component::board::v1::WriteAnalogRequest* request, ::viam::component::board::v1::WriteAnalogResponse*) { + ServerSpanGuard span_guard{context, "BoardServer::WriteAnalog"}; + if (!request) { - return ::grpc::Status(::grpc::StatusCode::INVALID_ARGUMENT, - "Called [Board::WriteAnalog] without a request"); + return span_guard.commit(::grpc::Status(::grpc::StatusCode::INVALID_ARGUMENT, + "Called [Board::WriteAnalog] without a request")); }; const std::shared_ptr rb = resource_manager()->resource(request->name()); if (!rb) { - return grpc::Status(grpc::UNKNOWN, "resource not found: " + request->name()); + return span_guard.commit( + grpc::Status(grpc::UNKNOWN, "resource not found: " + request->name())); } const std::shared_ptr board = std::dynamic_pointer_cast(rb); @@ -168,21 +174,25 @@ ::grpc::Status BoardServer::WriteAnalog( const GrpcContextObserver::Enable enable{*context}; board->write_analog(request->pin(), request->value(), extra); - return ::grpc::Status(); + return span_guard.commit(::grpc::Status()); } ::grpc::Status BoardServer::GetDigitalInterruptValue( ::grpc::ServerContext* context, const ::viam::component::board::v1::GetDigitalInterruptValueRequest* request, ::viam::component::board::v1::GetDigitalInterruptValueResponse* response) { + ServerSpanGuard span_guard{context, "BoardServer::GetDigitalInterruptValue"}; + if (!request) { - return ::grpc::Status(::grpc::StatusCode::INVALID_ARGUMENT, - "Called [Board::GetDigitalInterruptValue] without a request"); + return span_guard.commit( + ::grpc::Status(::grpc::StatusCode::INVALID_ARGUMENT, + "Called [Board::GetDigitalInterruptValue] without a request")); }; const std::shared_ptr rb = resource_manager()->resource(request->board_name()); if (!rb) { - return grpc::Status(grpc::UNKNOWN, "resource not found: " + request->board_name()); + return span_guard.commit( + grpc::Status(grpc::UNKNOWN, "resource not found: " + request->board_name())); } const std::shared_ptr board = std::dynamic_pointer_cast(rb); @@ -197,7 +207,7 @@ ::grpc::Status BoardServer::GetDigitalInterruptValue( board->read_digital_interrupt(request->digital_interrupt_name(), extra); response->set_value(result); - return ::grpc::Status(); + return span_guard.commit(::grpc::Status()); } ::grpc::Status BoardServer::StreamTicks( diff --git a/src/viam/sdk/module/service.cpp b/src/viam/sdk/module/service.cpp index d977f83b9..6fca4a49e 100644 --- a/src/viam/sdk/module/service.cpp +++ b/src/viam/sdk/module/service.cpp @@ -44,6 +44,7 @@ #include #include #include +#include #include namespace viam { @@ -69,6 +70,8 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { ::grpc::Status AddResource(::grpc::ServerContext* ctx, const ::viam::module::v1::AddResourceRequest* request, ::viam::module::v1::AddResourceResponse*) override { + impl::ServerSpanGuard span_guard{ctx, "AddResource"}; + const viam::app::v1::ComponentConfig& proto = request->config(); const ResourceConfig cfg = from_proto(proto); const std::lock_guard lock(parent.lock_); @@ -82,23 +85,25 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { res = reg->construct_resource(deps, cfg); res->set_log_level(cfg.get_log_level()); } catch (const std::exception& exc) { - return grpc::Status(::grpc::INTERNAL, exc.what()); + return span_guard.commit(grpc::Status(::grpc::INTERNAL, exc.what())); } } try { parent.server_->add_resource(res, ctx->deadline()); } catch (const std::exception& exc) { - return grpc::Status(::grpc::INTERNAL, exc.what()); + return span_guard.commit(grpc::Status(::grpc::INTERNAL, exc.what())); } - return grpc::Status(); + return span_guard.commit(grpc::Status()); } ::grpc::Status ReconfigureResource( - ::grpc::ServerContext*, + ::grpc::ServerContext* ctx, const ::viam::module::v1::ReconfigureResourceRequest* request, ::viam::module::v1::ReconfigureResourceResponse*) override { + impl::ServerSpanGuard span_guard{ctx, "ReconfigureResource"}; + const viam::app::v1::ComponentConfig& proto = request->config(); ResourceConfig cfg = from_proto(proto); @@ -106,8 +111,8 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { auto resource_server = parent.server_->lookup_resource_server(cfg.api()); if (!resource_server) { - return grpc::Status(grpc::UNKNOWN, - "no rpc service for config: " + cfg.api().to_string()); + return span_guard.commit(grpc::Status( + grpc::UNKNOWN, "no rpc service for config: " + cfg.api().to_string())); } auto manager = resource_server->resource_manager(); @@ -117,9 +122,10 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { { const std::shared_ptr res = manager->resource(cfg.resource_name().name()); if (!res) { - return grpc::Status(grpc::UNKNOWN, - "unable to stop resource " + cfg.resource_name().name() + - " as it doesn't exist."); + return span_guard.commit( + grpc::Status(grpc::UNKNOWN, + "unable to stop resource " + cfg.resource_name().name() + + " as it doesn't exist.")); } if (auto stoppable = std::dynamic_pointer_cast(res)) { @@ -131,8 +137,8 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { Registry::get().lookup_model(cfg.api(), cfg.model()); if (!reg) { - return grpc::Status(::grpc::INTERNAL, - "Unable to rebuild resource: model registration not found"); + return span_guard.commit(grpc::Status( + ::grpc::INTERNAL, "Unable to rebuild resource: model registration not found")); } try { @@ -140,24 +146,27 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { return reg->construct_resource(deps, cfg); }); } catch (const std::exception& exc) { - return grpc::Status(::grpc::INTERNAL, exc.what()); + return span_guard.commit(grpc::Status(::grpc::INTERNAL, exc.what())); } - return grpc::Status(); + return span_guard.commit(grpc::Status()); } - ::grpc::Status ValidateConfig(::grpc::ServerContext*, + ::grpc::Status ValidateConfig(::grpc::ServerContext* ctx, const ::viam::module::v1::ValidateConfigRequest* request, ::viam::module::v1::ValidateConfigResponse* response) override { + impl::ServerSpanGuard span_guard{ctx, "ValidateConfig"}; + const viam::app::v1::ComponentConfig& proto = request->config(); ResourceConfig cfg = from_proto(proto); const std::shared_ptr reg = Registry::get().lookup_model(cfg.api(), cfg.model()); if (!reg) { - return grpc::Status(grpc::UNKNOWN, - "unable to validate resource " + cfg.resource_name().name() + - " as it hasn't been registered."); + return span_guard.commit( + grpc::Status(grpc::UNKNOWN, + "unable to validate resource " + cfg.resource_name().name() + + " as it hasn't been registered.")); } try { const std::vector implicit_deps = reg->validate(cfg); @@ -165,26 +174,30 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { response->add_dependencies(dep); } } catch (const std::exception& err) { - return grpc::Status(grpc::UNKNOWN, - "validation failure in resource " + cfg.name() + ": " + err.what()); + return span_guard.commit(grpc::Status( + grpc::UNKNOWN, + "validation failure in resource " + cfg.name() + ": " + err.what())); } - return grpc::Status(); + return span_guard.commit(grpc::Status()); } - ::grpc::Status RemoveResource(::grpc::ServerContext*, + ::grpc::Status RemoveResource(::grpc::ServerContext* ctx, const ::viam::module::v1::RemoveResourceRequest* request, ::viam::module::v1::RemoveResourceResponse*) override { + impl::ServerSpanGuard span_guard{ctx, "RemoveResource"}; + auto name = Name::from_string(request->name()); auto resource_server = parent.server_->lookup_resource_server(name.api()); if (!resource_server) { - return grpc::Status(grpc::UNKNOWN, "no grpc service for " + name.api().to_string()); + return span_guard.commit( + grpc::Status(grpc::UNKNOWN, "no grpc service for " + name.api().to_string())); } const std::shared_ptr manager = resource_server->resource_manager(); const std::shared_ptr res = manager->resource(name.name()); if (!res) { - return grpc::Status( + return span_guard.commit(grpc::Status( grpc::UNKNOWN, - "unable to remove resource " + name.to_string() + " as it doesn't exist."); + "unable to remove resource " + name.to_string() + " as it doesn't exist.")); } if (auto stoppable = std::dynamic_pointer_cast(res)) { @@ -192,7 +205,7 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { } manager->remove(name); - return grpc::Status(); + return span_guard.commit(grpc::Status()); } ::grpc::Status Ready(::grpc::ServerContext*, From ac24dafd7b072dd5c624bdd632cd47c5a71bd529 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Thu, 14 May 2026 17:22:46 -0400 Subject: [PATCH 28/29] remove pimpl in private headers Co-Authored-By: Claude Opus 4.7 --- src/viam/sdk/tracing/private/span_guard.cpp | 44 +++++++-------------- src/viam/sdk/tracing/private/span_guard.hpp | 18 +++++++-- src/viam/sdk/tracing/private/tracer.cpp | 18 +++------ src/viam/sdk/tracing/private/tracer.hpp | 9 ++++- 4 files changed, 42 insertions(+), 47 deletions(-) diff --git a/src/viam/sdk/tracing/private/span_guard.cpp b/src/viam/sdk/tracing/private/span_guard.cpp index 218e7d4fd..92f4eba50 100644 --- a/src/viam/sdk/tracing/private/span_guard.cpp +++ b/src/viam/sdk/tracing/private/span_guard.cpp @@ -5,7 +5,7 @@ #ifdef VIAMCPPSDK_OPENTELEMETRY_TRACING -#include +#include #include #include @@ -72,17 +72,8 @@ class GrpcClientCarrier : public otel_prop::TextMapCarrier { } // namespace -// Constructing Impl makes the span active on the current thread -struct ServerSpanGuard::Impl { - opentelemetry::nostd::shared_ptr span; - otel_trace::Scope scope; - bool committed = false; - - explicit Impl(opentelemetry::nostd::shared_ptr s) noexcept - : span(std::move(s)), scope(span) {} -}; - -ServerSpanGuard::ServerSpanGuard(const GrpcServerContext* ctx, const char* method) noexcept { +opentelemetry::nostd::shared_ptr ServerSpanGuard::start_span_( + const GrpcServerContext* ctx, const char* method) noexcept { auto tracer = otel_trace::Provider::GetTracerProvider()->GetTracer(k_instrumentation_scope); otel_trace::StartSpanOptions opts; @@ -98,30 +89,27 @@ ServerSpanGuard::ServerSpanGuard(const GrpcServerContext* ctx, const char* metho auto span = tracer->StartSpan(method, opts); span->SetAttribute("rpc.system", "grpc"); - - impl_ = std::make_unique(std::move(span)); + return span; } +// Constructing `scope_` makes the span active on the current thread. +ServerSpanGuard::ServerSpanGuard(const GrpcServerContext* ctx, const char* method) noexcept + : span_(start_span_(ctx, method)), scope_(span_) {} + ServerSpanGuard::~ServerSpanGuard() noexcept { - if (!impl_) { - return; - } - if (!impl_->committed) { - impl_->span->SetStatus(otel_trace::StatusCode::kError, "handler threw an exception"); + if (!committed_) { + span_->SetStatus(otel_trace::StatusCode::kError, "handler threw an exception"); } - impl_->span->End(); + span_->End(); } ::grpc::Status ServerSpanGuard::commit(::grpc::Status status) noexcept { - if (!impl_) { - return status; - } - impl_->committed = true; + committed_ = true; if (status.error_code() == ::grpc::StatusCode::OK) { - impl_->span->SetStatus(otel_trace::StatusCode::kOk); + span_->SetStatus(otel_trace::StatusCode::kOk); } else { - impl_->span->SetStatus(otel_trace::StatusCode::kError); - impl_->span->SetAttribute("rpc.grpc.status_code", static_cast(status.error_code())); + span_->SetStatus(otel_trace::StatusCode::kError); + span_->SetAttribute("rpc.grpc.status_code", static_cast(status.error_code())); } return status; } @@ -142,8 +130,6 @@ namespace viam { namespace sdk { namespace impl { -struct ServerSpanGuard::Impl {}; - ServerSpanGuard::ServerSpanGuard(const GrpcServerContext*, const char*) noexcept {} ServerSpanGuard::~ServerSpanGuard() noexcept = default; diff --git a/src/viam/sdk/tracing/private/span_guard.hpp b/src/viam/sdk/tracing/private/span_guard.hpp index 046a266d1..a95ede7af 100644 --- a/src/viam/sdk/tracing/private/span_guard.hpp +++ b/src/viam/sdk/tracing/private/span_guard.hpp @@ -1,11 +1,14 @@ #pragma once -#include - #include #include +#ifdef VIAMCPPSDK_OPENTELEMETRY_TRACING +#include +#include +#endif + namespace viam { namespace sdk { namespace impl { @@ -30,9 +33,16 @@ class ServerSpanGuard { ServerSpanGuard(const ServerSpanGuard&) = delete; ServerSpanGuard& operator=(const ServerSpanGuard&) = delete; +#ifdef VIAMCPPSDK_OPENTELEMETRY_TRACING private: - struct Impl; - std::unique_ptr impl_; + // Builds the span and makes it active; runs before `scope_` in the constructor init list. + static opentelemetry::nostd::shared_ptr start_span_( + const GrpcServerContext* ctx, const char* method) noexcept; + + opentelemetry::nostd::shared_ptr span_; + opentelemetry::trace::Scope scope_; + bool committed_ = false; +#endif }; /// @brief Inject the currently-active OpenTelemetry trace context as W3C @c traceparent / diff --git a/src/viam/sdk/tracing/private/tracer.cpp b/src/viam/sdk/tracing/private/tracer.cpp index 82b9d6ca4..6da1d9161 100644 --- a/src/viam/sdk/tracing/private/tracer.cpp +++ b/src/viam/sdk/tracing/private/tracer.cpp @@ -96,11 +96,7 @@ void initialize_trace_propagator() noexcept { new opentelemetry::trace::propagation::HttpTraceContext())); } -struct Tracer::Impl { - std::shared_ptr sdk_provider; -}; - -Tracer::Tracer() : impl_(std::make_unique()) {} +Tracer::Tracer() = default; Tracer::~Tracer() { shutdown_provider(); } @@ -126,23 +122,23 @@ void Tracer::initialize_provider(RobotClient* client) noexcept { {"service.name", std::string{k_instrumentation_scope}}, }); - impl_->sdk_provider = std::shared_ptr( + sdk_provider_ = std::shared_ptr( otel_sdk_trace::TracerProviderFactory::Create(std::move(processor), resource)); - std::shared_ptr base_provider = impl_->sdk_provider; + std::shared_ptr base_provider = sdk_provider_; otel_trace::Provider::SetTracerProvider( opentelemetry::nostd::shared_ptr(std::move(base_provider))); } void Tracer::shutdown_provider() noexcept { - if (!impl_->sdk_provider) { + if (!sdk_provider_) { return; } - impl_->sdk_provider->Shutdown(); + sdk_provider_->Shutdown(); otel_trace::Provider::SetTracerProvider( opentelemetry::nostd::shared_ptr( new otel_trace::NoopTracerProvider())); - impl_->sdk_provider.reset(); + sdk_provider_.reset(); } } // namespace impl @@ -160,8 +156,6 @@ namespace impl { void initialize_trace_propagator() noexcept {} -struct Tracer::Impl {}; - Tracer::Tracer() = default; Tracer::~Tracer() = default; diff --git a/src/viam/sdk/tracing/private/tracer.hpp b/src/viam/sdk/tracing/private/tracer.hpp index 99b3a610a..c112d5805 100644 --- a/src/viam/sdk/tracing/private/tracer.hpp +++ b/src/viam/sdk/tracing/private/tracer.hpp @@ -1,7 +1,11 @@ #pragma once +#ifdef VIAMCPPSDK_OPENTELEMETRY_TRACING #include +#include +#endif + namespace viam { namespace sdk { @@ -30,9 +34,10 @@ class Tracer { void initialize_provider(RobotClient* client) noexcept; void shutdown_provider() noexcept; +#ifdef VIAMCPPSDK_OPENTELEMETRY_TRACING private: - struct Impl; - std::unique_ptr impl_; + std::shared_ptr sdk_provider_; +#endif }; } // namespace impl From 00ed4cb7d81df40da6467c79a48dabd45eff564e Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Thu, 14 May 2026 17:33:11 -0400 Subject: [PATCH 29/29] static initialize_propagator Co-Authored-By: Claude Opus 4.7 --- src/viam/sdk/common/instance.cpp | 2 +- src/viam/sdk/tracing/private/tracer.cpp | 4 ++-- src/viam/sdk/tracing/private/tracer.hpp | 7 ++++--- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/viam/sdk/common/instance.cpp b/src/viam/sdk/common/instance.cpp index cf0d0a982..a9e91ff2a 100644 --- a/src/viam/sdk/common/instance.cpp +++ b/src/viam/sdk/common/instance.cpp @@ -29,7 +29,7 @@ Instance::Instance() { impl_ = std::make_unique(); impl_->registry.initialize(); impl_->log_mgr.init_logging(); - impl::initialize_trace_propagator(); + impl::Tracer::initialize_propagator(); } Instance::~Instance() { diff --git a/src/viam/sdk/tracing/private/tracer.cpp b/src/viam/sdk/tracing/private/tracer.cpp index 6da1d9161..dc6c932ee 100644 --- a/src/viam/sdk/tracing/private/tracer.cpp +++ b/src/viam/sdk/tracing/private/tracer.cpp @@ -90,7 +90,7 @@ class ParentSendTracesExporter final : public otel_sdk_trace::SpanExporter { std::atomic is_shutdown_{false}; }; -void initialize_trace_propagator() noexcept { +void Tracer::initialize_propagator() noexcept { otel_prop::GlobalTextMapPropagator::SetGlobalPropagator( opentelemetry::nostd::shared_ptr( new opentelemetry::trace::propagation::HttpTraceContext())); @@ -154,7 +154,7 @@ class RobotClient; namespace impl { -void initialize_trace_propagator() noexcept {} +void Tracer::initialize_propagator() noexcept {} Tracer::Tracer() = default; Tracer::~Tracer() = default; diff --git a/src/viam/sdk/tracing/private/tracer.hpp b/src/viam/sdk/tracing/private/tracer.hpp index c112d5805..65932ba95 100644 --- a/src/viam/sdk/tracing/private/tracer.hpp +++ b/src/viam/sdk/tracing/private/tracer.hpp @@ -13,9 +13,6 @@ class RobotClient; namespace impl { -/// @brief Install the W3C Trace Context propagator. Called once during @c Instance construction. -void initialize_trace_propagator() noexcept; - /// @brief Holds the SDK-side OpenTelemetry tracer provider. Spans go nowhere until /// @c initialize_provider installs an exporter that ships them to the parent. class Tracer { @@ -29,6 +26,10 @@ class Tracer { /// @brief Returns the @c Tracer owned by the current @c Instance. static Tracer& get(); + /// @brief Install the W3C Trace Context propagator. Called once during @c Instance + /// construction. + static void initialize_propagator() noexcept; + /// @brief Install an exporter sourced from @p client. The caller must keep @p client alive /// until @c shutdown_provider returns. void initialize_provider(RobotClient* client) noexcept;