RUM Phase 1: stats collection + viewer consent/upload - #8
Open
AviaAv wants to merge 7 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete Real User Monitoring (RUM) subsystem: a new ChangesRUM Telemetry Feature
Sequence Diagram(s)sequenceDiagram
participant viewer as realsense-viewer.cpp
participant privacyUI as viewer.cpp Privacy Tab
participant hooks as rum_hooks/collector
participant config as rum_config
participant uploader as rum_uploader
participant cloud as RUM Cloud Endpoint
Note over viewer: Startup
viewer->>config: rum_cloud_enabled key present?
alt First run
viewer->>viewer: Show ImGui consent modal
viewer->>config: set_cloud_enabled(true/false)
viewer->>uploader: start_saved_upload(cadence, last_ts, callback)
else Already configured
viewer->>uploader: start_saved_upload(cadence, last_ts, callback)
end
uploader->>config: is_cloud_enabled()
uploader->>uploader: saved_report() from rum.json
uploader->>cloud: HTTP POST /v1/rum (libcurl)
uploader->>config: on_uploaded callback(now_unix)
Note over hooks: During session
hooks->>hooks: on_device / on_open / on_set_option / on_filter / on_notification
hooks->>hooks: on_stream_duration → rum_collector record_*
Note over privacyUI: User "Upload now"
privacyUI->>uploader: upload(get_report(), endpoint())
uploader->>cloud: HTTP POST /v1/rum
Note over viewer: Teardown
hooks->>hooks: on_context_closed() → collector.flush()
viewer->>uploader: join_saved_upload()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/rs.cpp (1)
928-974:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRecord the applied option value, not the raw input value.
rs2_set_optionmay coerce the input before setting (e.g., integer truncation), but the telemetry hook currently receives the originalvalue. This can produce inaccurateoptions_changed.last_valuein reports.Suggested fix
void rs2_set_option(const rs2_options* options, rs2_option option, float value, rs2_error** error) BEGIN_API_CALL { @@ - auto range = option_ref.get_range(); + auto range = option_ref.get_range(); + float applied_value = value; @@ case RS2_OPTION_TYPE_INTEGER: @@ - option_ref.set(std::trunc(value)); + applied_value = std::trunc( value ); + option_ref.set( applied_value ); break; @@ case RS2_OPTION_TYPE_BOOLEAN: if (value == 0.f) - option_ref.set_value(false); + { + option_ref.set_value(false); + applied_value = 0.f; + } else if (value == 1.f) - option_ref.set_value(true); + { + option_ref.set_value(true); + applied_value = 1.f; + } else throw invalid_value_exception(rsutils::string::from() << "not a boolean: " << value); break; @@ - librealsense::rum::hooks::on_set_option( *options->options, option, value, range.def ); + librealsense::rum::hooks::on_set_option( *options->options, option, applied_value, range.def ); }🤖 Prompt for 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. In `@src/rs.cpp` around lines 928 - 974, The telemetry hook call to librealsense::rum::hooks::on_set_option at the end of the rs2_set_option function receives the original input value parameter, but the function may coerce this value before setting it (e.g., truncating floats to integers for RS2_OPTION_TYPE_INTEGER, converting to boolean for RS2_OPTION_TYPE_BOOLEAN, or converting to string enum descriptions for RS2_OPTION_TYPE_STRING). Capture the actual coerced value that gets applied in each case within the switch statement and pass that captured value to the on_set_option hook instead of the original value parameter to ensure telemetry accurately records what was actually set.
🧹 Nitpick comments (2)
unit-tests/rum/pytest-rum-config.py (1)
12-16: 💤 Low valueConsider saving and restoring the original consent state.
The test modifies the cloud consent setting without restoring the original value. While pytest test isolation typically prevents cross-test interference, saving and restoring the original state is a best practice for tests that modify persistent settings.
♻️ Proposed enhancement
def test_cloud_consent_round_trips(): + original = rs.rum.is_cloud_enabled() + try: - rs.rum.set_cloud_enabled( True ) - assert rs.rum.is_cloud_enabled() - rs.rum.set_cloud_enabled( False ) - assert not rs.rum.is_cloud_enabled() + rs.rum.set_cloud_enabled( True ) + assert rs.rum.is_cloud_enabled() + rs.rum.set_cloud_enabled( False ) + assert not rs.rum.is_cloud_enabled() + finally: + rs.rum.set_cloud_enabled( original )🤖 Prompt for 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. In `@unit-tests/rum/pytest-rum-config.py` around lines 12 - 16, The test function test_cloud_consent_round_trips modifies the cloud consent setting but does not restore the original state. Capture the original cloud consent state at the beginning of the test by calling rs.rum.is_cloud_enabled() and saving it to a variable, then ensure it is restored to that original value after all assertions complete. Use a try/finally block or restore at the end of the test to guarantee the original state is restored even if an assertion fails.tools/rum-uploader/dev-server/rum_dev_server.py (1)
50-50: ⚡ Quick winRefine exception handling to avoid catching all exceptions.
Catching bare
Exceptioncan mask unexpected errors. Narrow the scope to the specific exceptions expected during JSON decode and UTF-8 decode.♻️ Proposed refinement
- except Exception: + except (ValueError, UnicodeDecodeError): with open(path, "wb") as f: f.write(body)🤖 Prompt for 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. In `@tools/rum-uploader/dev-server/rum_dev_server.py` at line 50, The bare `except Exception:` at line 50 is too broad and can mask unexpected errors. Replace it with specific exception types that are expected during the JSON decode and UTF-8 decode operations. Catch the specific exceptions that can be raised: json.JSONDecodeError for JSON parsing failures and UnicodeDecodeError for UTF-8 decoding issues. This narrows the error handling scope and allows unexpected exceptions to propagate, making debugging easier.
🤖 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 `@common/viewer.cpp`:
- Around line 3166-3169: The _rum_upload_thread.join() call in the render loop
blocks the UI thread if the previous upload is still running. Remove the join()
call from this location. Instead, track an in-progress state for the upload
operation (e.g., a boolean flag or atomic variable), disable or ignore button
interactions while the upload is active, and move the join() call to only
execute during teardown (destructor or cleanup) or after confirming the worker
thread has completed.
- Around line 3176-3179: The config_file::instance() singleton is accessed from
background RUM upload worker threads, creating a data race with the viewer UI
thread. In common/viewer.cpp#L3176-L3179, refactor the manual upload worker to
return the success status and timestamp value instead of directly calling
config_file::instance().set(configurations::privacy::rum_last_upload, ...) from
the worker thread; the caller should then persist rum_last_upload on the viewer
thread. In tools/realsense-viewer/realsense-viewer.cpp#L60-L70, snapshot all
cadence configuration inputs before spawning the boot worker to avoid background
thread config access, and commit the successful upload timestamp result on the
viewer thread or protected by a shared config mutex. This ensures all
config_file writes occur on a single thread or under synchronized access.
In `@src/rs.cpp`:
- Around line 294-295: Hook invocations on core execution paths can throw
exceptions and cause API operations to fail even after primary work succeeds.
Wrap each hook call in a try/catch block that catches exceptions and logs them
at debug level only, ensuring telemetry failures do not affect SDK behavior.
Apply this fail-safe pattern at all hook call sites in src/rs.cpp: the
on_notification hook call at lines 294-295, and the other hook invocations at
lines 415-418, 834-835, 850-851, and 973-974. For each site, place the hook call
inside a try block and catch all exceptions, logging them with processLogger or
appropriate debug logging at a level that does not propagate the error to the
caller.
In `@tools/rum-uploader/dev-server/rum_dev_server.py`:
- Around line 40-42: The _Handler._counter is being incremented and accessed
without thread synchronization, causing a race condition when
ThreadingHTTPServer handles concurrent requests. To fix this, introduce a
threading.Lock at the class level in _Handler, then wrap the counter increment
and path construction (where _Handler._counter is accessed) in a lock context
manager to ensure atomic access to the counter. This prevents simultaneous
reads/writes and ensures unique filenames are generated for each request even
under concurrent load.
In `@tools/rum-uploader/rum-uploader.cpp`:
- Around line 71-75: The `rum_uploader::upload()` function runs on background
threads but libcurl requires process-wide global initialization via
`curl_global_init()` before any thread-safe handle creation. While the mutex
protects `curl_easy_init()` at lines 71-75, it does not satisfy this
requirement. Add a `std::call_once` static guard that calls `curl_global_init()`
once at process startup, before the existing mutex-protected `curl_easy_init()`
call in the curl initialization block. This ensures libcurl is properly
initialized at the process level regardless of which background thread calls
`upload()` first.
In `@unit-tests/rum/pytest-rum-device.py`:
- Around line 11-13: The depth_z16_profile function uses next() without a
default value, which raises a cryptic StopIteration exception if no matching Z16
profile is found. Add a try-except block around the next() call in the
depth_z16_profile function to catch StopIteration and raise a more informative
exception with a clear error message indicating that a Z16 depth format profile
was expected but not found on the device.
---
Outside diff comments:
In `@src/rs.cpp`:
- Around line 928-974: The telemetry hook call to
librealsense::rum::hooks::on_set_option at the end of the rs2_set_option
function receives the original input value parameter, but the function may
coerce this value before setting it (e.g., truncating floats to integers for
RS2_OPTION_TYPE_INTEGER, converting to boolean for RS2_OPTION_TYPE_BOOLEAN, or
converting to string enum descriptions for RS2_OPTION_TYPE_STRING). Capture the
actual coerced value that gets applied in each case within the switch statement
and pass that captured value to the on_set_option hook instead of the original
value parameter to ensure telemetry accurately records what was actually set.
---
Nitpick comments:
In `@tools/rum-uploader/dev-server/rum_dev_server.py`:
- Line 50: The bare `except Exception:` at line 50 is too broad and can mask
unexpected errors. Replace it with specific exception types that are expected
during the JSON decode and UTF-8 decode operations. Catch the specific
exceptions that can be raised: json.JSONDecodeError for JSON parsing failures
and UnicodeDecodeError for UTF-8 decoding issues. This narrows the error
handling scope and allows unexpected exceptions to propagate, making debugging
easier.
In `@unit-tests/rum/pytest-rum-config.py`:
- Around line 12-16: The test function test_cloud_consent_round_trips modifies
the cloud consent setting but does not restore the original state. Capture the
original cloud consent state at the beginning of the test by calling
rs.rum.is_cloud_enabled() and saving it to a variable, then ensure it is
restored to that original value after all assertions complete. Use a try/finally
block or restore at the end of the test to guarantee the original state is
restored even if an assertion fails.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: b673d982-4609-45f7-a192-6275a3e0135e
📒 Files selected for processing (37)
CMake/global_config.cmakeCMake/lrs_options.cmakecommon/device-model.hcommon/viewer.cppcommon/viewer.hdoc/rum.mdinclude/CMakeLists.txtinclude/librealsense2/h/rs_rum.hinclude/librealsense2/hpp/rs_rum.hppinclude/librealsense2/rs.hinclude/librealsense2/rs.hppsrc/CMakeLists.txtsrc/context.cppsrc/proc/synthetic-stream.cppsrc/proc/synthetic-stream.hsrc/realsense.defsrc/rs.cppsrc/rum/CMakeLists.txtsrc/rum/rum-collector.cppsrc/rum/rum-collector.hsrc/rum/rum-config.cppsrc/rum/rum-config.hsrc/rum/rum-hooks.cppsrc/rum/rum-hooks.hsrc/sensor.cppsrc/sensor.htools/realsense-viewer/CMakeLists.txttools/realsense-viewer/realsense-viewer.cpptools/rum-uploader/dev-server/rum_dev_server.pytools/rum-uploader/rum-uploader.cpptools/rum-uploader/rum-uploader.hunit-tests/rum/pytest-rum-config.pyunit-tests/rum/pytest-rum-device.pywrappers/python/CMakeLists.txtwrappers/python/pyrealsense2.cppwrappers/python/pyrealsense2.hwrappers/python/pyrs_rum.cpp
Owner
Author
|
Thanks for the review. Addressed 2, declining 4 with reasoning: Fixed
Declining (with reasons)
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@doc/rum.md`:
- Around line 35-37: The documentation for the `-DENABLED_STATS=OFF` build flag
in the "Disable collection entirely at build time" section (lines 35-37 of
doc/rum.md) incorrectly states "no RUM code runs," which implies the functions
are completely removed. Reword this section to accurately reflect the ABI
contract: clarify that the RUM APIs remain available as no-op functions that
collect and upload nothing, rather than implying symbol removal or zero
execution paths. Keep the existing language about no statistics being collected
but adjust the phrasing to be precise about function availability.
In `@tools/rum-uploader/dev-server/rum_dev_server.py`:
- Around line 45-60: The broad `except Exception` block at line 50 is catching
both JSON parsing errors and file I/O errors, but the handler returns 200 OK in
all cases, masking persistence failures to clients. Replace the single broad
exception handler with two separate exception handlers: catch
`(UnicodeDecodeError, json.JSONDecodeError)` first to handle the non-JSON
fallback path with the existing logic, then catch `OSError` separately to detect
persistence failures and return a 500 status code with an appropriate error
response instead of the 200 OK response. Ensure that the successful 200 OK
response at line 55 onwards is only sent when both JSON parsing (if applicable)
and file I/O operations complete successfully.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: 853741be-7950-4835-8d27-e476cfa3e74a
📒 Files selected for processing (27)
common/device-model.hcommon/viewer.cppcommon/viewer.hdoc/rum.mdinclude/CMakeLists.txtinclude/librealsense2/h/rs_rum.hinclude/librealsense2/hpp/rs_rum.hppinclude/librealsense2/rs.hinclude/librealsense2/rs.hppsrc/context.cppsrc/proc/synthetic-stream.cppsrc/proc/synthetic-stream.hsrc/realsense.defsrc/rs.cppsrc/sensor.cppsrc/sensor.htools/realsense-viewer/CMakeLists.txttools/realsense-viewer/realsense-viewer.cpptools/rum-uploader/dev-server/rum_dev_server.pytools/rum-uploader/rum-uploader.cpptools/rum-uploader/rum-uploader.hunit-tests/rum/pytest-rum-config.pyunit-tests/rum/pytest-rum-device.pywrappers/python/CMakeLists.txtwrappers/python/pyrealsense2.cppwrappers/python/pyrealsense2.hwrappers/python/pyrs_rum.cpp
✅ Files skipped from review due to trivial changes (3)
- tools/realsense-viewer/CMakeLists.txt
- include/librealsense2/rs.h
- src/sensor.h
🚧 Files skipped from review as they are similar to previous changes (21)
- common/device-model.h
- wrappers/python/pyrs_rum.cpp
- wrappers/python/pyrealsense2.cpp
- src/realsense.def
- include/CMakeLists.txt
- common/viewer.h
- wrappers/python/pyrealsense2.h
- src/proc/synthetic-stream.cpp
- wrappers/python/CMakeLists.txt
- src/context.cpp
- tools/rum-uploader/rum-uploader.h
- src/proc/synthetic-stream.h
- include/librealsense2/rs.hpp
- src/sensor.cpp
- tools/realsense-viewer/realsense-viewer.cpp
- tools/rum-uploader/rum-uploader.cpp
- src/rs.cpp
- include/librealsense2/h/rs_rum.h
- unit-tests/rum/pytest-rum-device.py
- unit-tests/rum/pytest-rum-config.py
- common/viewer.cpp
Owner
Author
|
Re-review follow-up:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/rs.cpp`:
- Line 973: The call to librealsense::rum::hooks::on_set_option at line 973
reports the raw input value to telemetry, but integer options are coerced
earlier in the function at line 948. To fix this, capture the actual
coerced/applied value that results from the option processing and pass that to
on_set_option instead of the original raw value parameter. This ensures
telemetry accurately reports what value was actually applied rather than what
was requested, allowing proper classification of default-vs-changed state.
In `@tools/rum-uploader/dev-server/rum_dev_server.py`:
- Around line 7-8: The docstring at the beginning of the file incorrectly claims
the server accepts "HTTPS/HTTP POST" when in reality the implementation uses
plain ThreadingHTTPServer without any TLS context, making it HTTP-only. Update
the docstring to remove the "HTTPS/" reference and accurately state that the
server accepts only "HTTP POST /v1/rum" to prevent confusion during endpoint
setup and debugging.
In `@unit-tests/rum/pytest-rum-config.py`:
- Around line 12-16: The test_cloud_consent_round_trips function modifies global
SDK state by calling set_cloud_enabled without restoring the original value,
which can cause other tests to behave unexpectedly depending on execution order.
Save the original cloud enabled state at the start of the test using
is_cloud_enabled(), then wrap the test logic in a try/finally block to ensure
the original state is restored by calling set_cloud_enabled with the saved value
in the finally clause.
- Around line 41-46: The assertion in
test_processing_block_option_excluded_from_options_changed currently checks for
the global absence of "Min Distance" in the options_changed list, which can fail
if "Min Distance" was recorded by earlier tests. Instead, capture the count of
"Min Distance" entries before calling th.set_option(), then capture the count
again after rs.rum.get_report(), and assert that the count did not increase (the
delta should be zero). This validates that this specific processing-block
operation does not add to the recorded count, rather than checking for absolute
absence.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: 60bd263e-91b6-4eda-8e96-c017eb0e5619
📒 Files selected for processing (27)
common/device-model.hcommon/viewer.cppcommon/viewer.hdoc/rum.mdinclude/CMakeLists.txtinclude/librealsense2/h/rs_rum.hinclude/librealsense2/hpp/rs_rum.hppinclude/librealsense2/rs.hinclude/librealsense2/rs.hppsrc/context.cppsrc/proc/synthetic-stream.cppsrc/proc/synthetic-stream.hsrc/realsense.defsrc/rs.cppsrc/sensor.cppsrc/sensor.htools/realsense-viewer/CMakeLists.txttools/realsense-viewer/realsense-viewer.cpptools/rum-uploader/dev-server/rum_dev_server.pytools/rum-uploader/rum-uploader.cpptools/rum-uploader/rum-uploader.hunit-tests/rum/pytest-rum-config.pyunit-tests/rum/pytest-rum-device.pywrappers/python/CMakeLists.txtwrappers/python/pyrealsense2.cppwrappers/python/pyrealsense2.hwrappers/python/pyrs_rum.cpp
✅ Files skipped from review due to trivial changes (2)
- include/librealsense2/rs.h
- include/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (20)
- include/librealsense2/rs.hpp
- wrappers/python/pyrs_rum.cpp
- wrappers/python/CMakeLists.txt
- src/realsense.def
- src/proc/synthetic-stream.cpp
- common/device-model.h
- tools/rum-uploader/rum-uploader.h
- src/sensor.h
- src/proc/synthetic-stream.h
- include/librealsense2/h/rs_rum.h
- wrappers/python/pyrealsense2.h
- common/viewer.h
- common/viewer.cpp
- wrappers/python/pyrealsense2.cpp
- src/sensor.cpp
- tools/realsense-viewer/realsense-viewer.cpp
- tools/realsense-viewer/CMakeLists.txt
- include/librealsense2/hpp/rs_rum.hpp
- tools/rum-uploader/rum-uploader.cpp
- unit-tests/rum/pytest-rum-device.py
Owner
Author
|
Latest round:
|
AviaAv
force-pushed
the
rum-phase1
branch
2 times, most recently
from
June 21, 2026 11:40
385b16c to
4f68d29
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
common/rum-uploader/rum-uploader.cpp (1)
131-154: 💤 Low valueAssigning to a joinable
std::threadwill callstd::terminate.If
start_saved_upload()is ever called twice without an interveningjoin_saved_upload(), the assignment at line 134 will invokestd::terminate()becausesaved_upload_threadis still joinable.The current viewer correctly guards this with a static
rum_startup_doneflag, so there's no immediate bug. However, the API itself is fragile—consider adding a defensive check or documenting the single-call constraint.🛡️ Optional defensive fix
void start_saved_upload( int cadence_hours, long long last_upload_unix, std::function< void( long long ) > on_uploaded ) { + if( saved_upload_thread.joinable() ) + { + LOG_WARNING( "RUM: start_saved_upload called while upload already in progress; ignoring" ); + return; + } saved_upload_thread = std::thread( [cadence_hours, last_upload_unix, on_uploaded = std::move( on_uploaded )]()🤖 Prompt for 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. In `@common/rum-uploader/rum-uploader.cpp` around lines 131 - 154, The start_saved_upload function directly assigns to saved_upload_thread without checking if it's already joinable, which will call std::terminate if the function is called twice without an intervening join. Add a defensive check in start_saved_upload before the thread assignment to verify if saved_upload_thread is joinable and join it if needed, or alternatively add clear documentation explaining that this function must not be called multiple times without properly joining the previous thread first.
🤖 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 `@CMake/lrs_options.cmake`:
- Line 65: The ENABLED_STATS option in CMake/lrs_options.cmake is currently set
to OFF as the default, which contradicts the PR objective of enabling RUM
usage-statistics collection by default. Change the default value from OFF to ON
in the option definition for ENABLED_STATS to ensure RUM telemetry is enabled in
standard builds unless users explicitly disable it.
In `@doc/rum.md`:
- Around line 35-36: The documentation in the RUM statistics section contradicts
the actual default behavior. Line 35 currently states that collection is "off by
default," but according to this PR's contract, the collection should be ON by
default. Update the wording on line 35 to accurately reflect that the
ENABLED_STATS collection is ON by default at build time, and clarify how the
build flag `-DENABLED_STATS=ON` relates to this default state (whether it
enforces the default or changes behavior). This ensures packagers and operators
have correct information about the default telemetry behavior.
---
Nitpick comments:
In `@common/rum-uploader/rum-uploader.cpp`:
- Around line 131-154: The start_saved_upload function directly assigns to
saved_upload_thread without checking if it's already joinable, which will call
std::terminate if the function is called twice without an intervening join. Add
a defensive check in start_saved_upload before the thread assignment to verify
if saved_upload_thread is joinable and join it if needed, or alternatively add
clear documentation explaining that this function must not be called multiple
times without properly joining the previous thread first.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: 032a040b-9544-4c78-870c-20d85c77b7ed
📒 Files selected for processing (39)
.github/workflows/buildsCI.yamlCMake/global_config.cmakeCMake/lrs_options.cmakecommon/CMakeLists.txtcommon/device-model.hcommon/rum-uploader/rum-uploader.cppcommon/rum-uploader/rum-uploader.hcommon/viewer.cppcommon/viewer.hdoc/rum.mdinclude/CMakeLists.txtinclude/librealsense2/h/rs_rum.hinclude/librealsense2/hpp/rs_rum.hppinclude/librealsense2/rs.hinclude/librealsense2/rs.hppsrc/CMakeLists.txtsrc/context.cppsrc/proc/synthetic-stream.cppsrc/proc/synthetic-stream.hsrc/realsense.defsrc/rs.cppsrc/rum/CMakeLists.txtsrc/rum/rum-collector.cppsrc/rum/rum-collector.hsrc/rum/rum-config.cppsrc/rum/rum-config.hsrc/rum/rum-hooks.cppsrc/rum/rum-hooks.hsrc/sensor.cppsrc/sensor.htools/realsense-viewer/CMakeLists.txttools/realsense-viewer/realsense-viewer.cpptools/rum-uploader/dev-server/rum_dev_server.pyunit-tests/rum/pytest-rum-config.pyunit-tests/rum/pytest-rum-device.pywrappers/python/CMakeLists.txtwrappers/python/pyrealsense2.cppwrappers/python/pyrealsense2.hwrappers/python/pyrs_rum.cpp
✅ Files skipped from review due to trivial changes (6)
- common/device-model.h
- src/rum/CMakeLists.txt
- include/librealsense2/rs.h
- include/librealsense2/hpp/rs_rum.hpp
- include/librealsense2/rs.hpp
- include/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (25)
- CMake/global_config.cmake
- wrappers/python/pyrealsense2.h
- tools/realsense-viewer/CMakeLists.txt
- wrappers/python/pyrealsense2.cpp
- wrappers/python/pyrs_rum.cpp
- src/CMakeLists.txt
- src/sensor.cpp
- src/realsense.def
- src/sensor.h
- unit-tests/rum/pytest-rum-config.py
- wrappers/python/CMakeLists.txt
- src/rum/rum-collector.h
- include/librealsense2/h/rs_rum.h
- src/rum/rum-config.h
- common/CMakeLists.txt
- src/proc/synthetic-stream.cpp
- src/proc/synthetic-stream.h
- unit-tests/rum/pytest-rum-device.py
- common/viewer.h
- common/viewer.cpp
- src/rum/rum-hooks.h
- src/context.cpp
- src/rum/rum-hooks.cpp
- src/rum/rum-collector.cpp
- src/rs.cpp
| option(USE_EXTERNAL_LZ4 "Use externally build LZ4 library instead of building and using the in this repo provided version" OFF) | ||
| option(BUILD_ASAN "Enable AddressSanitizer" OFF) | ||
| option(BUILD_ROSBAG2 "Build and use rosbag2 recording system" ON) # temporary flag, should be removed when deprecated ROSBAG1 recording system is removed | ||
| option(ENABLED_STATS "Enable RUM (Real User Monitoring) usage-statistics collection" OFF) |
There was a problem hiding this comment.
Critical: Default contradicts PR objectives.
The PR summary states the feature is "enabled by default," but this option sets OFF as the default. This means RUM telemetry will be disabled in standard builds unless users explicitly override it.
🔧 Proposed fix to enable by default
-option(ENABLED_STATS "Enable RUM (Real User Monitoring) usage-statistics collection" OFF)
+option(ENABLED_STATS "Enable RUM (Real User Monitoring) usage-statistics collection" ON)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| option(ENABLED_STATS "Enable RUM (Real User Monitoring) usage-statistics collection" OFF) | |
| option(ENABLED_STATS "Enable RUM (Real User Monitoring) usage-statistics collection" ON) |
🤖 Prompt for 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.
In `@CMake/lrs_options.cmake` at line 65, The ENABLED_STATS option in
CMake/lrs_options.cmake is currently set to OFF as the default, which
contradicts the PR objective of enabling RUM usage-statistics collection by
default. Change the default value from OFF to ON in the option definition for
ENABLED_STATS to ensure RUM telemetry is enabled in standard builds unless users
explicitly disable it.
Comment on lines
+35
to
+36
| - **Collection is off by default at build time**: build the SDK with `-DENABLED_STATS=ON` to enable | ||
| it. When off, the `rs2_rum_*` API stays available (ABI-stable) but is a no-op — nothing is |
There was a problem hiding this comment.
Fix contradictory default-state wording for ENABLED_STATS.
Line 35 says collection is “off by default,” but this PR’s contract is default ON. This can mislead packagers/operators about telemetry behavior.
Suggested minimal doc fix
-- **Collection is off by default at build time**: build the SDK with `-DENABLED_STATS=ON` to enable
+- **Collection can be disabled at build time**: build the SDK with `-DENABLED_STATS=OFF` to disable
it. When off, the `rs2_rum_*` API stays available (ABI-stable) but is a no-op — nothing is
collected, persisted, or uploaded.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **Collection is off by default at build time**: build the SDK with `-DENABLED_STATS=ON` to enable | |
| it. When off, the `rs2_rum_*` API stays available (ABI-stable) but is a no-op — nothing is | |
| - **Collection can be disabled at build time**: build the SDK with `-DENABLED_STATS=OFF` to disable | |
| it. When off, the `rs2_rum_*` API stays available (ABI-stable) but is a no-op — nothing is |
🤖 Prompt for 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.
In `@doc/rum.md` around lines 35 - 36, The documentation in the RUM statistics
section contradicts the actual default behavior. Line 35 currently states that
collection is "off by default," but according to this PR's contract, the
collection should be ON by default. Update the wording on line 35 to accurately
reflect that the ENABLED_STATS collection is ON by default at build time, and
clarify how the build flag `-DENABLED_STATS=ON` relates to this default state
(whether it enforces the default or changes behavior). This ensures packagers
and operators have correct information about the default telemetry behavior.
AviaAv
force-pushed
the
rum-phase1
branch
3 times, most recently
from
June 22, 2026 08:25
69114cd to
8098ffc
Compare
AviaAv
force-pushed
the
rum-phase1
branch
2 times, most recently
from
June 22, 2026 09:12
a1b7b54 to
fbac3c7
Compare
…LIBCURL), config-driven cadence, CI test fix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RUM Phase 1 — Real User Monitoring (RSDEV-9259)
Anonymous, opt-in usage statistics. The SDK collects locally (gated by
ENABLED_STATS, on by default); the viewer handles the consent prompt and background upload to a local dev-server stub.What's here
source_id(stored inrum.json), instrumentation hooks (device / stream / option-change / filter / notification), publicrs2_rum_*API + python bindings.Notable deviations from the plan
ENABLED_STATS=OFFkeepsrs2_rum_*exported but a complete no-op (inert, ABI-stable), not symbol-free.Tests
unit-tests/rum/pytest-rum-config.py(non-live): 5/5unit-tests/rum/pytest-rum-device.py(live, D435I): 4/4Jira: RSDEV-9259
🤖 Generated with Claude Code
Summary by CodeRabbit