shu/maksimi/record logs new #1399
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to d18b852 in 6 minutes and 15 seconds
More details
- Looked at
933lines of code in16files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. skyvern/forge/sdk/db/client.py:812
- Draft comment:
Ensure that at least one filter is applied inget_artifacts_by_entity_idto avoid full table scans and potential performance issues. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_5m0vLQP0KJqVXjLJ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| from skyvern.forge.sdk.db.id import generate_artifact_id | ||
| from skyvern.forge.sdk.models import Step | ||
| from skyvern.forge.sdk.schemas.observers import ObserverCruise, ObserverThought | ||
|
|
||
| LOG = structlog.get_logger(__name__) | ||
|
|
||
| PRIMARY_KEY = Literal[ |
There was a problem hiding this comment.
Consider using an Enum instead of Literal for PRIMARY_KEY to define a set of constant values. Literal is not intended for use as a type for function parameters.
| @@ -70,3 +74,13 @@ def serialize_datetime_to_isoformat(self, value: datetime) -> str: | |||
| observer_thought_id: str | None = None | |||
| signed_url: str | None = None | |||
| organization_id: str | None = None | |||
|
|
|||
| def __getitem__(self, key: str) -> Any: | |||
There was a problem hiding this comment.
The __getitem__ method should handle cases where the attribute does not exist to avoid runtime errors. Consider using getattr(self, key, default) to provide a default value if the attribute is not found.
| @@ -13,6 +13,7 @@ class SkyvernContext: | |||
| max_steps_override: int | None = None | |||
| tz_info: ZoneInfo | None = None | |||
| totp_codes: dict[str, str | None] = field(default_factory=dict) | |||
| log: list[dict] = field(default_factory=list) | |||
There was a problem hiding this comment.
Consider using a structured log entry class for the log attribute in SkyvernContext to ensure consistency in log entry formatting.
| @@ -54,6 +54,21 @@ def add_kv_pairs_to_msg(logger: logging.Logger, method_name: str, event_dict: Ev | |||
| return event_dict | |||
|
|
|||
|
|
|||
| def skyvern_logs_processor(logger: logging.Logger, method_name: str, event_dict: EventDict) -> EventDict: | |||
There was a problem hiding this comment.
Consider implementing a mechanism to limit the size of the log list in skyvern_logs_processor to prevent excessive memory usage.
wintonzheng
force-pushed
the
shu/maksimi/record-logs-new
branch
from
fcadd70 to
63829fa
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 63829fa in 58 seconds
More details
- Looked at
472lines of code in7files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. skyvern/forge/agent.py:53
- Draft comment:
Duplicate import ofsave_step_logsandsave_task_logs. Remove the redundant import statement. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. skyvern/forge/sdk/artifact/manager.py:205
- Draft comment:
PRIMARY_KEYtype alias is removed but still used inupdate_artifact_data. Ensure the type is correctly defined or updated. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. skyvern/forge/sdk/log_artifacts.py:27
- Draft comment:
Ensure context is always available when callingsave_step_logs,save_task_logs,save_workflow_run_logs, andsave_workflow_run_block_logssincewith_skyvern_contextdecorator is removed. - Reason this comment was not posted:
Comment did not seem useful.
4. skyvern/forge/sdk/routes/agent_protocol.py:530
- Draft comment:
Review the# type: ignorecomment onget_artifacts_by_entity_idto ensure type safety and correctness. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_2IwAvw71rovhxq9W
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Introduces a new logging system for Skyvern, including log artifact types, context management, and frontend integration for efficient log handling and display.
SkyvernLogandSkyvernLogRawartifact types.SkyvernLogEncoderandSkyvernJSONLogEncoderfor log serialization.save_step_logs,save_task_logs, andsave_workflow_run_logsinlog_artifacts.pyto store logs.forge_log.pyto includeskyvern_logs_processorfor context-based logging.agent.pyto callsave_step_logsandsave_task_logsduring task and step updates.create_log_artifactmethod inmanager.pyfor log artifact creation.client.pyto handle log artifacts retrieval by entity ID.StepArtifacts.tsxto display logs.ScrollableActionList.tsxto fetch artifacts using new endpoint structure./entity_type/entity_id/artifactsendpoint inagent_protocol.pyfor fetching artifacts by entity type and ID.This description was created by
for 63829fa. It will automatically update as commits are pushed.