Improve Sentry product telemetry - #912
Conversation
📝 WalkthroughWalkthroughQtMeshEditor adds centralized Sentry telemetry with anonymous session identity, sanitization, allow-listed events, and test capture support. Instrumentation covers application workflows, CLI/MCP operations, AI model management, and GUI editing interactions. ChangesTelemetry reporter and privacy controls
Application lifecycle and workflows
AI model management
CLI and MCP operations
GUI editing interactions
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 review |
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fadf0405a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/TransformOperator.cpp (1)
2104-2106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the nested ternary operator into a helper function.
The nested ternary logic to map
mTransformStateto a string is duplicated across four locations and triggers static analysis warnings for complexity. Please consider extracting this mapping into a standalone helper function (e.g., using aswitchstatement in an anonymous namespace) to eliminate the duplication and improve readability.
src/TransformOperator.cpp#L2104-L2106: replace the duplicated nested ternary with a call to the extracted helper function.src/TransformOperator.cpp#L2153-L2155: replace the duplicated nested ternary with a call to the helper function.src/TransformOperator.cpp#L2204-L2206: replace the duplicated nested ternary with a call to the helper function.src/TransformOperator.cpp#L2303-L2305: replace the duplicated nested ternary with a call to the helper function.🤖 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/TransformOperator.cpp` around lines 2104 - 2106, The transform-state-to-string mapping is duplicated as nested ternaries, increasing complexity. Add a standalone helper in the anonymous namespace using a switch over mTransformState, preserving translate, rotate, scale, and other mappings, then replace the duplicated expressions in src/TransformOperator.cpp at lines 2104-2106, 2153-2155, 2204-2206, and 2303-2305 with calls to that helper.Source: Linters/SAST tools
src/AIModelCatalog.cpp (1)
83-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant error sanitization call.
captureModelTelemetryalready appliesSentryReporter::sanitizedErrorCategoryinternally to thefailureCategoryparameter (at line 419). Applying it here before passing the argument results in a redundant double-sanitization.♻️ Proposed refactor
- SentryReporter::sanitizedErrorCategory(error)); + error);🤖 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/AIModelCatalog.cpp` around lines 83 - 92, Remove the SentryReporter::sanitizedErrorCategory call from the captureModelTelemetry invocation in the download-failure handling block, and pass error directly as the failureCategory argument. Leave captureModelTelemetry responsible for sanitization and preserve the existing telemetry fields and breadcrumb behavior.src/CLIPipeline.cpp (1)
1573-1577: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
configureSessioncall.
SentryReporter::configureSession(QStringLiteral("cli"))is called twice. The second call is redundant and should be removed.♻️ Proposed fix
if (SentryReporter::isEnabled()) { SentryReporter::configureSession(QStringLiteral("cli")); SentryReporter::initialize(); - SentryReporter::configureSession(QStringLiteral("cli")); }🤖 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/CLIPipeline.cpp` around lines 1573 - 1577, Remove the duplicate second SentryReporter::configureSession(QStringLiteral("cli")) call in the initialization block, leaving the initial configuration before SentryReporter::initialize() unchanged.src/SentryReporter.h (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
captureFileWorkflowEvent's 11-parameter signature is error-prone and flagged by static analysis. Two adjacent same-typedintparameters (modelCount,animationCount) are easy to transpose at call sites (already called positionally 4+ times inmainwindow.cpp), and SonarCloud flags the function for exceeding the 7-parameter guideline.
src/SentryReporter.h#L59-68: update the declaration to accept a small params struct/builder instead of 11 positional arguments.src/SentryReporter.cpp#L352-382: update the matching definition and internal field access accordingly.🤖 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/SentryReporter.h` at line 1, Replace the 11 positional arguments of captureFileWorkflowEvent in SentryReporter.h with a small parameter struct or builder, using named fields for modelCount, animationCount, and the remaining values. Update the matching SentryReporter.cpp definition and all call sites to construct and pass this object, then access its fields internally while preserving the existing event behavior.Source: Linters/SAST tools
59-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
captureFileWorkflowEventhas 11 parameters — high risk of positional mix-ups.SonarCloud flags this on the definition side too. Two consecutive
intparams (modelCount,animationCount) are easy to transpose at call sites, and this signature is already called with many positional args inmainwindow.cpp. Consider a small params struct/builder instead of continuing to grow the positional list.🤖 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/SentryReporter.h` around lines 59 - 68, Replace the positional argument list of SentryReporter::captureFileWorkflowEvent with a dedicated parameter struct or builder, including modelCount and animationCount as named fields. Update the declaration, definition, and all call sites such as mainwindow.cpp to construct and pass this object, preserving the existing defaults and event behavior.Source: Linters/SAST tools
🤖 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/AIModelCatalog.cpp`:
- Around line 499-511: Rename the bulk-download startup telemetry event in the
surrounding download initialization flow from ai.model_download.started to
ai.model_download_all.started, while preserving its existing payload and
breadcrumb. Do not alter single-model download events or the completion event in
startNextQueuedFile.
- Around line 530-533: Update the deletion-start logic in AIModelCatalog’s flow
near deleteStartedMs and the existing SentryReporter::addBreadcrumb call to emit
the ai.model_delete.started telemetry event, not just a breadcrumb. Preserve the
existing timestamp and breadcrumb recording, and use the same telemetry
mechanism and event conventions as the completed and failed deletion outcomes.
- Around line 104-107: Update the cancellation handler in AIModelCatalog to emit
the ai.model_download.canceled telemetry event alongside the existing
SentryReporter breadcrumb, before clearing m_pendingFiles. Preserve the current
cancellation cleanup and match the established telemetry behavior used for the
corresponding failed and completed download events.
In `@src/CLIPipeline.cpp`:
- Around line 10100-10108: Move the QElapsedTimer initialization and
segmentation.started telemetry block from its current position to immediately
before MeshSegmenter::Options opts;, after the --dump-training-data early-return
path. Preserve the existing telemetry fields and category handling so the event
is emitted only when segmentation prediction actually begins.
In `@src/SentryReporter.cpp`:
- Around line 329-350: Update captureInvocationEvent so it sets only the
surface-appropriate identifier: populate tool for the MCP surface and command
for other surfaces, using sanitizedValue(name) in the selected field. Remove the
unconditional assignment that currently writes both fields, while preserving the
remaining telemetry properties and event naming.
---
Nitpick comments:
In `@src/AIModelCatalog.cpp`:
- Around line 83-92: Remove the SentryReporter::sanitizedErrorCategory call from
the captureModelTelemetry invocation in the download-failure handling block, and
pass error directly as the failureCategory argument. Leave captureModelTelemetry
responsible for sanitization and preserve the existing telemetry fields and
breadcrumb behavior.
In `@src/CLIPipeline.cpp`:
- Around line 1573-1577: Remove the duplicate second
SentryReporter::configureSession(QStringLiteral("cli")) call in the
initialization block, leaving the initial configuration before
SentryReporter::initialize() unchanged.
In `@src/SentryReporter.h`:
- Line 1: Replace the 11 positional arguments of captureFileWorkflowEvent in
SentryReporter.h with a small parameter struct or builder, using named fields
for modelCount, animationCount, and the remaining values. Update the matching
SentryReporter.cpp definition and all call sites to construct and pass this
object, then access its fields internally while preserving the existing event
behavior.
- Around line 59-68: Replace the positional argument list of
SentryReporter::captureFileWorkflowEvent with a dedicated parameter struct or
builder, including modelCount and animationCount as named fields. Update the
declaration, definition, and all call sites such as mainwindow.cpp to construct
and pass this object, preserving the existing defaults and event behavior.
In `@src/TransformOperator.cpp`:
- Around line 2104-2106: The transform-state-to-string mapping is duplicated as
nested ternaries, increasing complexity. Add a standalone helper in the
anonymous namespace using a switch over mTransformState, preserving translate,
rotate, scale, and other mappings, then replace the duplicated expressions in
src/TransformOperator.cpp at lines 2104-2106, 2153-2155, 2204-2206, and
2303-2305 with calls to that helper.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fe25ad8-c756-421c-87e6-42a07260b2b9
📒 Files selected for processing (17)
.gitignoredocs/TELEMETRY.mdsrc/AIModelCatalog.cppsrc/AIModelCatalog.hsrc/AnimationControlController.cppsrc/AnimationWidget.cppsrc/AppSettingsKeys.hsrc/CLIPipeline.cppsrc/EditModeController.cppsrc/MCPServer.cppsrc/SelectionSet.cppsrc/SentryReporter.cppsrc/SentryReporter.hsrc/SentryReporter_test.cppsrc/TransformOperator.cppsrc/main.cppsrc/mainwindow.cpp
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 `@src/mainwindow.cpp`:
- Around line 4424-4442: Add a SentryReporter::addBreadcrumb call at the start
of the import operation in importMeshs, before MeshImporterExporter::importer
runs, using the file.import operation context and existing import path
information where appropriate. Keep the existing workflow events and transaction
handling unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb72fdc6-95a3-488c-9e7c-37c3b34b8761
📒 Files selected for processing (8)
docs/TELEMETRY.mdsrc/AIModelCatalog.cppsrc/CLIPipeline.cppsrc/SentryReporter.cppsrc/SentryReporter.hsrc/SentryReporter_test.cppsrc/TransformOperator.cppsrc/mainwindow.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/TELEMETRY.md
- src/TransformOperator.cpp
- src/SentryReporter_test.cpp
- src/AIModelCatalog.cpp
- src/CLIPipeline.cpp
- src/SentryReporter.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/SentryReporter.cpp (1)
45-52: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact sensitive values inside JSON arrays too.
sanitizedObject()recognizes sensitive keys, but array values are passed through unchanged by the surrounding logic. Payloads such asfiles: [{ "path": ... }]can therefore leak sensitive data to Sentry. Add recursive array sanitization and a regression test before converting the payload to Sentry values.🤖 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/SentryReporter.cpp` around lines 45 - 52, Update sanitizedObject() to recursively sanitize JSON array elements before converting the payload to Sentry values, applying the existing sensitive-key redaction to objects nested within arrays such as files[]. Add a regression test covering sensitive fields inside an array and verify the resulting Sentry payload contains redacted values.
🤖 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.
Outside diff comments:
In `@src/SentryReporter.cpp`:
- Around line 45-52: Update sanitizedObject() to recursively sanitize JSON array
elements before converting the payload to Sentry values, applying the existing
sensitive-key redaction to objects nested within arrays such as files[]. Add a
regression test covering sensitive fields inside an array and verify the
resulting Sentry payload contains redacted values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f24161f8-c109-4218-9f3a-548841d10ea9
📒 Files selected for processing (4)
src/AppSettingsKeys.hsrc/MCPServer.cppsrc/SentryReporter.cppsrc/mainwindow.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/AppSettingsKeys.h
- src/MCPServer.cpp
|
1 participant
Summary
Privacy
telemetry/anonymousInstallationId; never derives identity from machine/user/hardware datauser.idonly toinstall:<uuid>and sanitizes paths, filenames, prompts, emails, GitHub identifiers, and tokensQTMESH_TELEMETRY_ROLEwithdeveloper,ci,tester, defaulting touserValidation
cmake --build build_smoke --target UnitTests -j2build_smoke/bin/UnitTests --gtest_filter='SentryReporterTest.*'git diff --checkSummary by CodeRabbit
Documentation
docs/TELEMETRY.mdcovering crash diagnostics, anonymous telemetry behavior, privacy controls, allowed telemetry roles, event taxonomy, and example queries.Enhancements
Tests