Skip to content

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

Merged
merged 5 commits into from
Jun 11, 2025
Merged

Alert refactor #305

merged 5 commits into from
Jun 11, 2025

Conversation

Mandolaro
Copy link
Collaborator

@Mandolaro Mandolaro commented Jun 7, 2025

📝 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

  • Self-review
  • Video demo of changes
  • Unit Tests and CI/CD tests are passing
  • Reviewers assigned

📌 Linear Issue

https://linear.app/judgment/issue/JUD-1060/fix-alerts

✏️ Additional Notes

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 AlertStatus enum (using strings like "triggered", "not_triggered") has been replaced with a simple boolean (True for triggered, False for not triggered) in src/judgeval/rules.py. This simplifies the representation and usage of alert statuses.
  • Enhanced Alert Result Data: The AlertResult model in src/judgeval/rules.py has been updated to include additional fields: combined_type, project_id, and trace_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 Rule model's model_dump method in src/judgeval/rules.py to 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 to score_type.
  • Timestamp Handling Improvement: In src/judgeval/common/tracer.py, the method for converting a timestamp to an ISO format string has been updated from datetime.utcfromtimestamp to datetime.fromtimestamp with timezone.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 timezone from the datetime module (L21).
    • Updated timestamp serialization to use datetime.fromtimestamp with timezone.utc for better timezone handling (L597).
  • src/judgeval/rules.py
    • Removed the AlertStatus string enum and replaced it with a boolean type alias (L16).
    • Added a "name": "" field to the initial metric_data dictionary in Rule.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 AlertResult docstring to reflect the boolean status (L217).
    • Changed the type of the status field in AlertResult from AlertStatus to bool (L236).
    • Added combined_type, project_id, and trace_span_id fields to the AlertResult model (L242-L244).
    • Added a model_dump method to AlertResult (L256-L258).
    • Updated RulesEngine.evaluate_rules to assign the boolean triggered value directly to the status field (L427).
    • Populated the new combined_type, project_id, and trace_span_id fields when creating AlertResult instances (L437-L439).
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

  1. 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.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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_type in the Rule class is named combined_type in the AlertResult class. While minor, consistency would be beneficial.
  • Redundant model_dump method: The AlertResult class defines a model_dump method that simply calls the parent's method, which is redundant.
  • Type Hint Precision: The created_at field in TraceSpan uses the Optional[Any] type hint, which could be more precise (e.g., float or Union[float, str]).
  • Serialization Logic Complexity: The _serialize_value method in TraceSpan is 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.

@JCamyre JCamyre self-requested a review June 11, 2025 17:53
@JCamyre JCamyre added the In Review Task is currently being reviewed label Jun 11, 2025
Copy link
Collaborator

@JCamyre JCamyre left a 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

@JCamyre JCamyre enabled auto-merge (squash) June 11, 2025 20:07
@JCamyre JCamyre merged commit 10597eb into staging Jun 11, 2025
4 of 5 checks passed
@JCamyre JCamyre deleted the alert_refactor branch July 11, 2025 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Review Task is currently being reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants