build(clp_s::ffi::sfa): Add search dependencies and make OpenTelemetry and Libarchive optional for downstream WebAssembly. - #2474
Conversation
WalkthroughThe build adds an OpenTelemetry C++ option for search. Search telemetry now uses either real or no-op implementations. Dependency wiring, FFI SFA linkage, KQL validation, and libarchive include placement are updated. ChangesSearch telemetry build
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BuildOption as Search telemetry build option
participant SearchCMake as clp_s search CMake
participant RealTelemetry as Real telemetry sources
participant NoopTelemetry as No-op telemetry sources
BuildOption->>SearchCMake: Select telemetry mode
SearchCMake->>RealTelemetry: Compile when enabled
SearchCMake->>NoopTelemetry: Compile when disabled
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CLP_BUILD_CLP_S_SEARCH_ENABLE_OPENTELEMETRY_CPP build option to support downstream WebAssembly compatibility.CLP_BUILD_CLP_S_SEARCH_ENABLE_OPENTELEMETRY_CPP build option to support downstream WebAssembly compatibility.
CLP_BUILD_CLP_S_SEARCH_ENABLE_OPENTELEMETRY_CPP build option to support downstream WebAssembly compatibility.There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/core/cmake/Options/options.cmake`:
- Around line 414-419: Update
set_clp_s_search_enable_opentelemetry_cpp_dependencies so it sets
CLP_NEED_OPENTELEMETRY_CPP and CLP_NEED_XXHASH only when CLP_BUILD_CLP_S_SEARCH
is enabled; preserve the existing dependency flags for search builds while
leaving them unset for unrelated builds.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e8af5db7-2b89-415f-b049-1c73ee454e08
📒 Files selected for processing (6)
components/core/cmake/Options/options.cmakecomponents/core/src/clp_s/InputConfig.cppcomponents/core/src/clp_s/ffi/CMakeLists.txtcomponents/core/src/clp_s/search/CMakeLists.txtcomponents/core/src/clp_s/search/SearchTelemetryNoop.cppcomponents/core/src/clp_s/search/TelemetryContextNoop.cpp
gibber9809
left a comment
There was a problem hiding this comment.
Could we also use the CLP_BUILD_CLP_S_ENABLE_OPENTELEMETRY flag in clp_s/CommandLineArguments.cpp to remove the enable-telemetry flag using some #if macros when open telemetry is disabled in the build?
| CLP_BUILD_CLP_S_SEARCH_ENABLE_OPENTELEMETRY_CPP | ||
| "Include open telemetry support for clp_s::search." |
There was a problem hiding this comment.
| CLP_BUILD_CLP_S_SEARCH_ENABLE_OPENTELEMETRY_CPP | |
| "Include open telemetry support for clp_s::search." | |
| CLP_BUILD_CLP_S_ENABLE_OPENTELEMETRY | |
| "Include open telemetry support for clp-s." |
Could we rename the flag (and other corresponding functions, etc.) to this? If we did add opentelemetry support to other parts of clp-s we'd probably want to share the same flag, instead of relying on a different flag for each sub-component, so we might as well just create a flag at the clp-s level now.
| clp_s_ffi_sfa | ||
| PUBLIC | ||
| clp_s::archive_reader | ||
| clp_s::search |
There was a problem hiding this comment.
| clp_s::search |
Not used in this PR? Should probably be part of the PR where you add search to the ffi.
| SearchTelemetryNoop.cpp | ||
| TelemetryContextNoop.cpp |
There was a problem hiding this comment.
Instead of adding new cpp files let's use the more standard approach of doing:
target_compile_definitions(
clp_s_search
PRIVATE
CLP_BUILD_CLP_S_ENABLE_OPENTELEMETRY=$<BOOL:${CLP_BUILD_CLP_S_ENABLE_OPENTELEMETRY}>
)
Then use #if ... macros to stub out implementations in the existing cpp files.
Description
This change prepares
clp_s::ffi::sfafor downstream WebAssembly search functionalities by providing flags to disable OpenTelemetry and Libarchive support. Downstreamclp-ffi-jswill link againstclp_s::searchwithout the need to cross-compile these two libraries.Checklist
breaking change.
Validation performed
Summary by CodeRabbit