feat: add anonymous usage telemetry (Homebrew-style opt-out) #478
Conversation
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
Adds privacy-respecting usage analytics using OpenPanel: - Telemetry ON by default, easily disabled via `bm telemetry disable` - First-run notice shown once (not a prompt, just informational) - Anonymous installation ID (random UUID, user can delete) - Tracks: app starts, MCP tool usage, errors (sanitized) - Never collects: note content, file paths, personal data Implementation: - New telemetry module with fire-and-forget event tracking - CLI commands: enable, disable, status - Config fields: telemetry_enabled, telemetry_notice_shown - Instrumented 7 core MCP tools - 12 unit tests covering all functionality 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: phernandez <[email protected]>
Add track_mcp_tool() calls to all remaining MCP tools: - view_note - list_directory - move_note - read_content - canvas - list_memory_projects - create_memory_project - delete_project - search (ChatGPT) - fetch (ChatGPT) This completes telemetry instrumentation for all 17 MCP tools. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: phernandez <[email protected]>
Contributor
|
Claude finished @phernandez's task in 3m 35s —— View job PR Review: Telemetry Implementation ✅Comprehensive review of the telemetry PR against Basic Memory's team checklist completed. ✅ Code Quality & StandardsFollowing Basic Memory Conventions
Code Comments & DocumentationNeeds improvement - The code would benefit from literate programming style comments: src/basic_memory/telemetry.py:78-110 - The # Trigger: telemetry disabled in config
# Why: avoid network calls and resource usage when user opts out
# Outcome: client created but won't send events (OpenPanel's disabled flag)
_client = OpenPanel(
client_id=OPENPANEL_CLIENT_ID,
client_secret=OPENPANEL_CLIENT_SECRET,
disabled=not config.telemetry_enabled,
)src/basic_memory/telemetry.py:122-134 - # Constraint: telemetry must never break the application
# Even if OpenPanel API is down or network fails, user's command must succeed
try:
_get_client().track(event, properties or {})
except Exception as e:
logger.opt(exception=False).debug(f"Telemetry failed: {e}")src/basic_memory/cli/app.py:53-55 - The conditional skip logic needs a decision point comment: # Trigger: user invoked 'mcp' or 'telemetry' command
# Why: 'mcp' handles telemetry in its own lifespan, 'telemetry' would create circular notice
# Outcome: skip telemetry notice and tracking for these special commands
if ctx.invoked_subcommand not in {"mcp", "telemetry"}:✅ Testing & DocumentationTest Coverage
Test Quality Issuestests/test_telemetry.py:85-103 - Test doesn't verify exception is suppressed: def test_track_does_not_raise_on_error(self, config_home, monkeypatch):
# ...
# Should not raise
track("test_event", {"key": "value"})
# ❌ Missing assertion that track() actually returned without raisingRecommendation: Add explicit assertion: try:
track("test_event", {"key": "value"})
success = True
except Exception:
success = False
assert success, "track() should never raise exceptions"Documentation
✅ Basic Memory ArchitectureMCP Tool Instrumentation
Configuration Integration
Startup Integration
|
The tests were setting the HOME environment variable, but on Windows Path.home() uses USERPROFILE instead. Mock Path.home() directly for cross-platform compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: phernandez <[email protected]>
Contributor
|
Claude finished @phernandez's task in 2m 26s —— View job PR Review: Telemetry Implementation ✅Comprehensive review of the telemetry PR against Basic Memory's team checklist completed. ✅ Code Quality & StandardsFollowing Basic Memory Conventions
Code Comments & DocumentationNeeds improvement - The code would benefit from literate programming style comments: src/basic_memory/telemetry.py:78-110 - The # Trigger: telemetry disabled in config
# Why: avoid network calls and resource usage when user opts out
# Outcome: client created but won't send events (OpenPanel's disabled flag)
_client = OpenPanel(
client_id=OPENPANEL_CLIENT_ID,
client_secret=OPENPANEL_CLIENT_SECRET,
disabled=not config.telemetry_enabled,
)src/basic_memory/telemetry.py:122-134 - # Constraint: telemetry must never break the application
# Even if OpenPanel API is down or network fails, user's command must succeed
try:
_get_client().track(event, properties or {})
except Exception as e:
logger.opt(exception=False).debug(f"Telemetry failed: {e}")src/basic_memory/cli/app.py:53-55 - The conditional skip logic needs a decision point comment: # Trigger: user invoked 'mcp' or 'telemetry' command
# Why: 'mcp' handles telemetry in its own lifespan, 'telemetry' would create circular notice
# Outcome: skip telemetry notice and tracking for these special commands
if ctx.invoked_subcommand not in {"mcp", "telemetry"}:✅ Testing & DocumentationTest Coverage
Test Quality Issuestests/test_telemetry.py:85-103 - Test doesn't verify exception is suppressed: def test_track_does_not_raise_on_error(self, config_home, monkeypatch):
# ...
# Should not raise
track("test_event", {"key": "value"})
# ❌ Missing assertion that track() actually returned without raisingRecommendation: Add explicit assertion: try:
track("test_event", {"key": "value"})
success = True
except Exception:
success = False
assert success, "track() should never raise exceptions"Documentation
✅ Basic Memory ArchitectureMCP Tool Instrumentation
Note: PR description claims "All 17 MCP tools instrumented" but there are actually 14 tool files. This is likely counting individual functions (chatgpt_tools.py has 2 functions, project_management.py has 3 functions). Configuration Integration
Startup Integration
|
Contributor
|
Claude finished @phernandez's task in 3m 4s —— View job PR Review: Telemetry Implementation ✅Comprehensive review of the telemetry PR against Basic Memory's team checklist completed. ✅ Code Quality & StandardsFollowing Basic Memory Conventions
Code Comments & DocumentationThe code follows the section header pattern from CLAUDE.md (lines 32, 49, 55, 75, 119, 136, 173 in telemetry.py), but could benefit from additional decision point comments as per the literate programming guidelines: Recommended additions (non-blocking): src/basic_memory/telemetry.py:78-110 - The # Trigger: telemetry disabled in config
# Why: avoid network calls and resource usage when user opts out
# Outcome: client created but won't send events (OpenPanel's disabled flag)src/basic_memory/telemetry.py:122-134 - The # Constraint: telemetry must never break the application
# Even if OpenPanel API is down or network fails, user's command must succeedsrc/basic_memory/cli/app.py:53-55 - The conditional skip logic: # Trigger: user invoked 'mcp' or 'telemetry' command
# Why: 'mcp' handles telemetry in its own lifespan, 'telemetry' would create circular notice
# Outcome: skip telemetry notice and tracking for these special commands✅ Testing & DocumentationTest Coverage
Minor test improvement opportunity (non-blocking): try:
track("test_event", {"key": "value"})
success = True
except Exception:
success = False
assert success, "track() should never raise exceptions"Documentation
Recommendation: Update CLAUDE.md's "Basic Memory Commands" section to document the telemetry commands, and consider adding a Privacy/Telemetry section to README.md. ✅ Basic Memory ArchitectureMCP Tool Instrumentation
Note: PR description says "All 17 MCP tools" but there are 14 tool files. Count is correct when including the multiple functions in chatgpt_tools.py (2) and project_management.py (3). Configuration Integration
Startup Integration
Dependency Management
✅ Security & PerformanceSecurity
Recommendation (non-blocking): Add regex sanitization to strip file paths before truncation: def track_error(error_type: str, message: str) -> None:
import re
# Sanitize Unix and Windows paths
sanitized = re.sub(r'/[\w/.-]+\.(py|md|txt|json)', '[FILE]', message)
sanitized = re.sub(r'C:\\[\w\\.-]+\.(py|md|txt|json)', '[FILE]', sanitized)
track("error", {"type": error_type, "message": sanitized[:200]})Performance
Question for author: Does the OpenPanel Python SDK have built-in request timeouts? If not, verify it won't block the app if the API is slow. ✅ CompatibilityWindows Compatibility
Console Output
📋 Complete Checklist ResultsCode Quality & Standards
Testing & Documentation
Basic Memory Architecture
Security & Performance
Compatibility
🎯 Summary & RecommendationsThis is a well-implemented, production-ready telemetry system that properly follows the Homebrew opt-out model. The code quality is high, with excellent type annotations, comprehensive testing, and thoughtful integration into the existing architecture. Strengths
Required ChangesNone - This PR is functionally complete and safe to merge. Recommended Improvements (Non-blocking)
Questions for Author
Overall Assessment✅ APPROVE - This PR is ready to merge. The implementation is solid, well-tested, and follows all Basic Memory conventions. The recommended improvements are nice-to-haves that can be addressed in follow-up PRs if desired. The telemetry system successfully balances data collection for product improvement with strong privacy protections, following industry best practices (Homebrew model). The code is clean, maintainable, and won't impact user experience. |
Signed-off-by: phernandez <[email protected]>
phernandez
force-pushed
the
feature/telemetry
branch
from
b28c132 to
7161d8a
Compare
Contributor
|
Claude finished @phernandez's task in 2m 52s —— View job PR Review: Telemetry Implementation ✅Comprehensive review of the telemetry PR against Basic Memory's team checklist completed. ✅ Code Quality & StandardsFollowing Basic Memory Conventions
Code Comments & DocumentationThe code follows the section header pattern from CLAUDE.md, but would benefit from additional decision point comments as per literate programming guidelines (non-blocking): src/basic_memory/telemetry.py:78-110 - The # Trigger: telemetry disabled in config
# Why: avoid network calls and resource usage when user opts out
# Outcome: client created but won't send events (OpenPanel's disabled flag)src/basic_memory/telemetry.py:122-134 - The # Constraint: telemetry must never break the application
# Even if OpenPanel API is down or network fails, user's command must succeedsrc/basic_memory/cli/app.py:53-55 - The conditional skip logic: # Trigger: user invoked 'mcp' or 'telemetry' command
# Why: 'mcp' handles telemetry in its own lifespan, 'telemetry' would create circular notice
# Outcome: skip telemetry notice and tracking for these special commands✅ Testing & DocumentationTest Coverage
Minor test improvement opportunity (non-blocking): try:
track("test_event", {"key": "value"})
success = True
except Exception:
success = False
assert success, "track() should never raise exceptions"Documentation
Recommendation: Update CLAUDE.md's "Basic Memory Commands" section to document the telemetry commands: **Telemetry Management:**
- Check status: `basic-memory telemetry status`
- Opt out: `basic-memory telemetry disable`
- Opt in: `basic-memory telemetry enable`Consider adding a Privacy/Telemetry section to README.md as well. ✅ Basic Memory ArchitectureMCP Tool Instrumentation
Note: PR description says "All 17 MCP tools" but there are 14 tool files (17 when counting individual functions in chatgpt_tools.py and project_management.py). Configuration Integration
Startup Integration
Dependency Management
|
Addresses code review feedback: - Add literate programming comments explaining key decision points in _get_client() and track() functions - Sanitize file paths from error messages before sending to prevent leaking user directory structure (both Unix and Windows paths) - Add test for path sanitization 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: phernandez <[email protected]>
Contributor
|
Claude finished @phernandez's task in 2m 32s —— View job PR Review: Telemetry Implementation ✅Comprehensive review against Basic Memory team checklist completed. SummaryThis PR implements a production-ready, privacy-respecting telemetry system following the Homebrew analytics model. The latest commit (a2b735e) successfully addressed previous code review feedback by adding:
The implementation is complete, well-tested, and ready to merge. The only non-blocking recommendations are documentation updates (CLAUDE.md and README.md). Code Quality & Standards ✅Following Basic Memory Conventions
Literate Programming Comments (Latest Commit)The latest commit successfully addressed code review feedback with decision point comments: telemetry.py:91-94 - Explains lazy initialization and disabled flag: # Trigger: first call to track an event
# Why: lazy init avoids work if telemetry never used; disabled flag
# tells OpenPanel to skip network calls when user opts out
# Outcome: client ready to queue events (or silently discard if disabled)telemetry.py:134-135 - Explains fail-safe constraint: # Constraint: telemetry must never break the application
# Even if OpenPanel API is down or config is corrupt, user's command must succeedcli/app.py:50-52 - Explains conditional skip logic: # Show telemetry notice and track CLI startup
# Skip for 'mcp' command - it handles its own telemetry in lifespan
# Skip for 'telemetry' command - avoid issues when user is managing telemetryThis follows the CLAUDE.md literate programming guidelines perfectly. Testing & Documentation ✅Test Coverage
Path Sanitization Implementation (NEW)telemetry.py:240-244 - Sanitizes both Unix and Windows paths: # Sanitize file paths to prevent leaking user directory structure
# Unix paths: /Users/name/file.py, /home/user/notes/doc.md
sanitized = re.sub(r"/[\w/.+-]+\.\w+", "[FILE]", message)
# Windows paths: C:\Users\name\file.py, D:\projects\doc.md
sanitized = re.sub(r"[A-Z]:\\[\w\\.+-]+\.\w+", "[FILE]", sanitized, flags=re.IGNORECASE)This prevents accidental leakage of user directory structure in error messages. Documentation
Basic Memory Architecture ✅MCP Tool Instrumentation
Configuration Integration
Startup Integration
Dependency Management
Security & Performance ✅Security
The path sanitization regex patterns are comprehensive:
Performance
Compatibility ✅Windows Compatibility
Console Output
Checklist ResultsCode Quality & Standards
Testing & Documentation
Basic Memory Architecture
Security & Performance
Compatibility
Comparison with Previous ReviewsFour previous automated reviews (Dec 25-27) identified the same recommendations. This review confirms: ✅ Addressed:
❌ Not addressed (non-blocking):
Recommendations (Non-blocking)1. Update DocumentationAdd telemetry section to CLAUDE.md under "Basic Memory Commands": **Telemetry Management:**
- Check status: `basic-memory telemetry status`
- Opt out: `basic-memory telemetry disable`
- Opt in: `basic-memory telemetry enable`Consider adding Privacy/Telemetry section to README.md explaining the Homebrew model. 2. Minor Test ImprovementIn try:
track("test_event", {"key": "value"})
success = True
except Exception:
success = False
assert success, "track() should never raise exceptions"3. Consider Timeout ConfigurationVerify if OpenPanel SDK has built-in timeouts. If not, consider adding: _client = OpenPanel(
client_id=OPENPANEL_CLIENT_ID,
client_secret=OPENPANEL_CLIENT_SECRET,
disabled=not config.telemetry_enabled,
timeout=5.0, # Don't block app if API is slow
)Questions for Author
Overall Assessment✅ APPROVE - Ready to Merge This PR successfully implements a privacy-respecting telemetry system that:
The latest commit (a2b735e) specifically addressed code review feedback by adding path sanitization and literate programming comments. The implementation is production-ready. Recommended documentation updates are nice-to-haves that can be addressed in follow-up PRs if desired. |
Documents the anonymous telemetry feature including: - What data is collected and what is never collected - How to opt out via CLI or environment variable - Link to detailed documentation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: phernandez <[email protected]>
Contributor
|
Claude finished @phernandez's task in 1m 59s —— View job PR Review: Telemetry Implementation ✅Comprehensive review against Basic Memory team checklist completed. Executive SummaryThis PR successfully implements a production-ready, privacy-respecting telemetry system following the Homebrew opt-out model. The latest commits (a2b735e and 13ea9bd) have successfully addressed all previous code review feedback: ✅ Path sanitization added - Unix and Windows file paths removed from error messages Recommendation: APPROVE - Ready to merge Detailed Checklist Review✅ Code Quality & Standards
Literate Programming Implementation (src/basic_memory/telemetry.py):
✅ Testing & Documentation
Test Coverage Highlights:
Documentation Updates:
✅ Basic Memory Architecture
MCP Tool Instrumentation:
Configuration Integration:
Startup Integration:
✅ Security & Performance
Security Analysis: Path Sanitization (telemetry.py:240-244): # Sanitize file paths to prevent leaking user directory structure
# Unix paths: /Users/name/file.py, /home/user/notes/doc.md
sanitized = re.sub(r"/[\w/.+-]+\.\w+", "[FILE]", message)
# Windows paths: C:\Users\name\file.py, D:\projects\doc.md
sanitized = re.sub(r"[A-Z]:\\[\w\\.+-]+\.\w+", "[FILE]", sanitized, flags=re.IGNORECASE)Test Verification (tests/test_telemetry.py:250-276):
Privacy Guarantees:
Performance:
✅ Compatibility
Windows Compatibility:
Console Output:
Commit-by-Commit AnalysisCommit History:
Latest Commits Address All Feedback:a2b735e (Path Sanitization + Comments):
13ea9bd (README Documentation):
Code ExcellenceStrengths:
Implementation Patterns:
Comparison with Previous Reviews4 previous automated reviews (Dec 25-27) identified recommendations. This review confirms: ✅ Fully Addressed:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Adds privacy-respecting anonymous usage telemetry using OpenPanel, following the Homebrew analytics model:
bm telemetry disableWhat we collect
What we NEVER collect
Changes
src/basic_memory/telemetry.pysrc/basic_memory/cli/commands/telemetry.pysrc/basic_memory/config.pysrc/basic_memory/mcp/server.pysrc/basic_memory/cli/app.pysrc/basic_memory/mcp/tools/*.pytests/test_telemetry.pyCLI Commands
Test plan
bm telemetry disableworks🤖 Generated with Claude Code