Skip to content
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

Add step / task / workflow run / observer metrics as logs #1698

Merged
merged 2 commits into from
Feb 1, 2025

Conversation

wintonzheng
Copy link
Contributor

@wintonzheng wintonzheng commented Feb 1, 2025

Important

Add logging for step, task, workflow run, and observer metrics, and enhance LLM API calls with prompt_name for better tracking.

  • Metrics Logging:
    • Log step duration in update_step() in agent.py when step is completed or failed.
    • Log task duration in update_task() in agent.py when task is completed, failed, or terminated.
    • Log workflow run duration in execute_workflow() in service.py when workflow run is completed.
    • Log observer cruise duration in mark_observer_task_as_completed() in observer_service.py when completed.
    • Log LLM API handler duration in llm_api_handler_with_router_and_fallback() and llm_api_handler() in api_handler_factory.py.
  • LLM API Enhancements:
    • Add prompt_name parameter to LLM API calls in agent.py, handler.py, caching.py, and observer_service.py for better tracking.
  • Configuration Changes:
    • Increase MAX_STEPS_PER_RUN from 10 to 75 in config.py.

This description was created by Ellipsis for f072cd0. It will automatically update as commits are pushed.

<!-- ELLIPSIS_HIDDEN -->

> [!IMPORTANT]
> Add logging for step, task, workflow run, and observer metrics, and introduce `prompt_name` for LLM API calls.
>
>   - **Logging Enhancements**:
>     - Add logging for step duration in `agent.py` when step is completed or failed.
>     - Add logging for task duration in `agent.py` when task is completed, failed, or terminated.
>     - Add logging for observer cruise duration in `observer_service.py` when completed.
>     - Add logging for workflow run duration in `workflow/service.py` when completed.
>   - **LLM API Handler**:
>     - Introduce `prompt_name` parameter in `api_handler_factory.py` for LLM API calls to track prompt types.
>     - Update LLM API handler calls across multiple files to include `prompt_name`.
>   - **Miscellaneous**:
>     - Set `include_in_schema=False` for `eval_router` in `app.py` to exclude from schema documentation.
>
> <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for 5c81904d116acf693dd8a5307ab5e8bc970e7b45. It will automatically update as commits are pushed.</sup>

<!-- ELLIPSIS_HIDDEN -->
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on f072cd0 in 1 minute and 11 seconds

More details
  • Looked at 954 lines of code in 12 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/forge/agent.py:1922
  • Draft comment:
    Use datetime.now(timezone.utc) instead of datetime.now(UTC) for getting the current time in UTC. This issue is present in multiple files (e.g., agent.py, observer_service.py, workflow/service.py).
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  1. The comment suggests an alternative way to get UTC time, but both approaches are functionally equivalent.
  2. The code is already working correctly - UTC from datetime module is the standard way to get UTC timezone.
  3. Making this change would be purely stylistic and wouldn't improve functionality.
  4. The comment also mentions this issue exists in other files, but we're only supposed to focus on this file.
    The comment does point out a consistent style issue across multiple files. Maybe there's value in standardizing the approach across the codebase?
    While consistency is good, this is a minor stylistic preference between two equally valid approaches. The current code is using the standard datetime.UTC constant which is perfectly fine.
    Delete this comment. The code is using a valid approach with datetime.UTC, and changing it would be a purely stylistic change with no functional benefit.

Workflow ID: wflow_Jtg3Iuh8NHxAILXv


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to f072cd0 in 1 minute and 23 seconds

More details
  • Looked at 954 lines of code in 12 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. skyvern/forge/sdk/routes/agent_protocol.py:1047
  • Draft comment:
    The prompt_name parameter is missing in this LLM_API_HANDLER call. Consider adding it for consistency in logging and tracking.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. skyvern/forge/sdk/workflow/models/block.py:1200
  • Draft comment:
    The prompt_name parameter is missing in this LLM_API_HANDLER call. Consider adding it for consistency in logging and tracking.
  • Reason this comment was not posted:
    Marked as duplicate.
3. skyvern/forge/sdk/routes/agent_protocol.py:1001
  • Draft comment:
    The prompt_name parameter is missing in this LLM_API_HANDLER call. Consider adding it for consistency in logging and tracking.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_ZzArh0Nsv5xoX2NU


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@suchintan suchintan merged commit 204972e into main Feb 1, 2025
6 checks passed
@suchintan suchintan deleted the suchintan.add-metrics-as-logs branch February 1, 2025 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants