-
Notifications
You must be signed in to change notification settings - Fork 82
Alert refactor #305
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
Alert refactor #305
Conversation
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.
Hello @Mandolaro, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team,
Gemini here to provide a summary of this pull request titled "Alert refactor". The primary goal of this PR, as indicated by the title and description, is to fix and improve the existing alert functionality, making it more robust and useful. This aligns with the linked Linear issue JUD-1060, which aims to address issues with alerts.
The changes involve refactoring how alert statuses are represented, adding more contextual information to alert results, ensuring metric names are properly serialized, and making a small improvement to timestamp handling in the tracer module for better timezone accuracy.
Highlights
- Alert Status Refactor: The
AlertStatusenum (using strings like "triggered", "not_triggered") has been replaced with a simple boolean (Truefor triggered,Falsefor not triggered) insrc/judgeval/rules.py. This simplifies the representation and usage of alert statuses. - Enhanced Alert Result Data: The
AlertResultmodel insrc/judgeval/rules.pyhas been updated to include additional fields:combined_type,project_id, andtrace_span_id. These fields provide more context about the alert, linking it to the specific project and trace span where it occurred. - Metric Name Serialization: Logic has been added to the
Rulemodel'smodel_dumpmethod insrc/judgeval/rules.pyto ensure that the name of the metric associated with a condition is included in the serialized output. It attempts to get the name from__name__,name, or falls back toscore_type. - Timestamp Handling Improvement: In
src/judgeval/common/tracer.py, the method for converting a timestamp to an ISO format string has been updated fromdatetime.utcfromtimestamptodatetime.fromtimestampwithtimezone.utc. This is the recommended way to create timezone-aware datetimes from timestamps in Python 3.3+.
Changelog
Click here to see the changelog
- src/judgeval/common/tracer.py
- Imported
timezonefrom thedatetimemodule (L21). - Updated timestamp serialization to use
datetime.fromtimestampwithtimezone.utcfor better timezone handling (L597).
- Imported
- src/judgeval/rules.py
- Removed the
AlertStatusstring enum and replaced it with a boolean type alias (L16). - Added a
"name": ""field to the initialmetric_datadictionary inRule.model_dump(L146). - Added logic to populate the
metric_data['name']field using__name__,name, or a fallback (L185-L192). - Updated the example in the
AlertResultdocstring to reflect the boolean status (L217). - Changed the type of the
statusfield inAlertResultfromAlertStatustobool(L236). - Added
combined_type,project_id, andtrace_span_idfields to theAlertResultmodel (L242-L244). - Added a
model_dumpmethod toAlertResult(L256-L258). - Updated
RulesEngine.evaluate_rulesto assign the booleantriggeredvalue directly to thestatusfield (L427). - Populated the new
combined_type,project_id, andtrace_span_idfields when creatingAlertResultinstances (L437-L439).
- Removed the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request refactors the alert system, primarily by changing the AlertStatus from a string enum to a boolean and updating related logic and data structures. It also includes updates to the tracing system, particularly around handling timestamps and serializing complex objects, and adds fields to link evaluation runs and examples to trace spans. The changes seem to align with the goal of making the alert system functional and useful. Overall, the changes are well-intentioned and address some technical debt and add useful features. There are a few areas for improvement regarding consistency and code clarity.
Summary of Findings
- Inconsistent Naming: The field name
combine_typein theRuleclass is namedcombined_typein theAlertResultclass. While minor, consistency would be beneficial. - Redundant
model_dumpmethod: TheAlertResultclass defines amodel_dumpmethod that simply calls the parent's method, which is redundant. - Type Hint Precision: The
created_atfield inTraceSpanuses theOptional[Any]type hint, which could be more precise (e.g.,floatorUnion[float, str]). - Serialization Logic Complexity: The
_serialize_valuemethod inTraceSpanis complex due to recursive handling of various types and fallback logic.
Merge Readiness
The pull request implements the alert refactor and integrates it with the tracing system. The core logic changes seem correct within the scope of the modified files. The identified issues are primarily related to code maintainability and minor inconsistencies (medium severity). While the PR could technically be merged, addressing these points would improve the overall code quality and clarity. I recommend addressing the medium severity issues before merging.
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.
LGTM! One QQ, and waiting to merge until Judgment is merged and deployed
📝 Summary
Fix alert to be functional again and useful
#305
https://github.com/JudgmentLabs/async-eval-server/pull/33
https://github.com/JudgmentLabs/judgment/pull/416
🎯 Purpose
🎥 Demo of Changes
🧪 Testing
✅ Checklist
📌 Linear Issue
https://linear.app/judgment/issue/JUD-1060/fix-alerts
✏️ Additional Notes