-
Notifications
You must be signed in to change notification settings - Fork 789
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
Record logs into step artifacts #1339
Changes from 8 commits
79e5763
c3cbf4b
f3debf5
c1e7ff6
b1f14c6
b2cb9c4
eb6fc3d
57cd772
419e12b
ddbc340
f313326
9b35651
2d90fed
be0e817
5ee8c3b
f8a6b58
8bf863c
5d32d53
3c37a4a
1aa1146
f3bb2fe
6606fba
aabc0fc
b83c648
40e9dd3
5c8488d
127b8a6
4742fc4
974b37e
d48f792
f57c7b6
c8274c3
3ff49cb
c3ce7d3
72b3f15
aac781f
ff86b52
a86d81d
0f0633f
727dd7e
2312281
d66e67b
2cf5f69
63829fa
9eda7ca
631f898
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,6 +72,7 @@ | |
from skyvern.webeye.browser_factory import BrowserState | ||
from skyvern.webeye.scraper.scraper import ElementTreeFormat, ScrapedPage, scrape_website | ||
from skyvern.webeye.utils.page import SkyvernFrame | ||
from skyvern.forge.skyvern_log_encoder import SkyvernLogEncoder | ||
|
||
LOG = structlog.get_logger() | ||
|
||
|
@@ -1773,6 +1774,24 @@ async def update_step( | |
step_id=step.step_id, | ||
diff=update_comparison, | ||
) | ||
|
||
try: | ||
log = skyvern_context.current().log | ||
current_step_log = [entry for entry in log if entry.get("step_id", "") == step.step_id] | ||
log_json = json.dumps(current_step_log, cls=SkyvernLogEncoder, indent=2) | ||
await app.ARTIFACT_MANAGER.create_artifact( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way we can also capture task and workflow level logs? Even if we don't render them in the UI today We historically associate logs like that w/ the first step, although @wintonzheng is currently overhauling it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah we just added new columns to the artifacts db: https://github.com/Skyvern-AI/skyvern/blob/main/skyvern/forge/sdk/db/models.py#L169-L174 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://github.com/Skyvern-AI/skyvern/pull/1345/files here's the example of supporting artifact upload for observer thought. going forward we can add support for task level and workflow run level artifacts |
||
step=step, | ||
artifact_type=ArtifactType.SKYVERN_LOG, | ||
data=log_json.encode(), | ||
) | ||
except Exception: | ||
LOG.error( | ||
"Failed to record skyvern log after action", | ||
task_id=step.task_id, | ||
step_id=step.step_id, | ||
exc_info=True, | ||
) | ||
|
||
return await app.DATABASE.update_step( | ||
task_id=step.task_id, | ||
step_id=step.step_id, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import json | ||
from typing import Any | ||
|
||
class SkyvernLogEncoder(json.JSONEncoder): | ||
"""Custom JSON encoder for Skyvern logs that handles non-serializable objects""" | ||
def default(self, obj: Any) -> Any: | ||
if hasattr(obj, 'model_dump'): | ||
return obj.model_dump() | ||
|
||
if hasattr(obj, '__dataclass_fields__'): | ||
return {k: getattr(obj, k) for k in obj.__dataclass_fields__} | ||
|
||
if hasattr(obj, '__dict__'): | ||
return { | ||
'type': obj.__class__.__name__, | ||
'attributes': {k: v for k, v in obj.__dict__.items() if not k.startswith('_')} | ||
} | ||
# Handle other non-serializable objects | ||
try: | ||
return str(obj) | ||
except Exception: | ||
return f"<non-serializable-{obj.__class__.__name__}>" |
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.
this PR is a good start.
There are a couple of problems i'm seeing right now (not because of the code here) that we need keep iterating:
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.
@wintonzheng are we certain that each log must have a step_id?
From what I see in the logs when I ran tasks there are several logs in the beginning that don't belong to any step:
Here first 7 logs would be without a step because the first step is not created yet.
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.
@wintonzheng do I undestand correctly that we want to create task and step ids in advance?
This way the logs that are currently not associated with a step or task because step/task not created yet would be associated with the first task and first step?
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.
no, step_id needs to be manually defined in
LOG.info("xxx", step_id="stp_xxxx")
nowadays. those logs you shared here don't belong to any step so it's okay many logs don't havestep_id
.yep, exactly what i mean. we need to backfill step_id and task_id in some logs.