-
Notifications
You must be signed in to change notification settings - Fork 0
Add comprehensive metrics system to HAL #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
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
dansimau
commented
Jul 21, 2025
dansimau
commented
Jul 21, 2025
dansimau
commented
Jul 21, 2025
dansimau
commented
Jul 21, 2025
dansimau
commented
Jul 21, 2025
dansimau
commented
Jul 21, 2025
Owner
Author
dansimau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good but can we:
- Use a popular Golang library for the CLI
- Add tests for everything here
- Added SQLite metrics storage with optimized batched writes and automatic pruning - Implemented metrics service with configurable retention (30 days default) - Added instrumentation for: * Automation triggered events * Automation evaluation events * Tick processing time measurements - Created HAL CLI with stat command showing metrics across time periods: * Last minute, hour, day, month * Counter metrics summed, timer metrics show p99 - Added database path configuration support for flexible deployment - Fixed test isolation with in-memory databases 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
## Cobra CLI Integration - Replaced basic CLI with professional Cobra library - Added proper command structure, help text, and flags - Supports aliases and examples for better UX - Added --db flag for custom database paths ## Comprehensive Test Suite - Added metrics service tests (async and sync scenarios) - Added CLI stats command tests with sample data - Added connection metrics integration tests - Tests cover batching, pruning, error handling, and edge cases - Fixed database concurrency issues in tests ## Metrics System Enhancements - Fixed service shutdown to properly drain buffered metrics - Improved async processing reliability - Better error handling and logging All major functionality now has test coverage ensuring reliability. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
## Line-by-line feedback addressed: ### 1. Remove hal binary and add to .gitignore ✅ - Removed committed hal binary - Added hal to .gitignore to prevent future commits ### 2. Simplify metrics system (remove complex batching) ✅ - Removed complex buffering and batching system - Now writes directly to SQLite, leveraging WAL mode for performance - Eliminates risk of data loss if process dies - Much simpler and more reliable implementation ### 3. Extend retention from 30 days to 3 months ✅ - Changed retention from 30 days to 90 days (3 months) - Updated tests to reflect new retention period ### 4. Make MetricType a custom type instead of string ✅ - Created custom MetricType type for type safety - Updated all usage throughout codebase - Prevents string-based errors and improves API clarity ### 5. Updated tests for simplified service ✅ - Removed tests for complex batching behavior - Updated all tests to work with direct SQLite writes - All metrics tests now pass The metrics system is now simpler, more reliable, and addresses all the specific concerns raised in the PR review. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
d4117fc to
0d106ed
Compare
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
statscommand showing metrics across multiple time periodsKey Features
hal statscommand with tabular outputTest plan
🤖 Generated with Claude Code
Note
Introduce SQLite-backed metrics (automation triggers, tick processing time) with pruning and a new
hal statsCLI to display multi-period summaries.store.Metricmodel andMetricTypeenums; migrate viastore.Open.metrics.Servicefor recording counters/timers and periodic pruning.Connectionlifecycle (Start/Stop).tick_processing_time(timer) andautomation_triggered(counter) inStateChangeEvent.halbinary withstatscommand to show metrics for last minute/hour/day/month..gitignore; add Cobra dependencies ingo.mod.Written by Cursor Bugbot for commit f48e44f. This will update automatically on new commits. Configure here.