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

Refactor recording & reporting agent time usage #261

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

satococoa
Copy link
Contributor

Summary

Refactor recording & reporting agent time usage.

Related Issue

https://github.com/giselles-ai/giselle/pull/251/files#r1891016983

I would like to separate the recording of the execution from the context of the execution, so it would be better to make it a callback like onFinishPerformFlowExecution and add props like onFinishPerformFlowExecutionAction in the Provider. I think, but in this Pull request, it is fine as it is.

@satococoa satococoa self-assigned this Dec 19, 2024
Copy link

vercel bot commented Dec 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
giselle ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 19, 2024 8:30am

Copy link
Contributor

@toyamarinyon toyamarinyon left a comment

Choose a reason for hiding this comment

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

Thank you! We are proposing some minor changes, please check them and apply them if necessary.

currentExecution.durationMs,
);

await onFinishPerformExecution(runEndedAt, totalFlowDurationMs);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await onFinishPerformExecution(runEndedAt, totalFlowDurationMs);
await onFinishPerformExecution?.(runEndedAt, totalFlowDurationMs);

Comment on lines +613 to +622
onFinishPerformExecution: async (
endedAt: number,
totalDurationMs: number,
) => {
await onFinishPerformExecutionAction(
flowRunStartedAt,
endedAt,
totalDurationMs,
);
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onFinishPerformExecution: async (
endedAt: number,
totalDurationMs: number,
) => {
await onFinishPerformExecutionAction(
flowRunStartedAt,
endedAt,
totalDurationMs,
);
},
onFinishPerformExecution: (endedAt, durationMs) =>
onFinishPerformExecutionAction(flowRunStartedAt, endedAt, durationMs),

Comment on lines +689 to +695
onFinishPerformExecution: async (endedAt, durationMs) => {
await onFinishPerformExecutionAction(
flowRunStartedAt,
endedAt,
durationMs,
);
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onFinishPerformExecution: async (endedAt, durationMs) => {
await onFinishPerformExecutionAction(
flowRunStartedAt,
endedAt,
durationMs,
);
},
onFinishPerformExecution: (endedAt, durationMs) =>
onFinishPerformExecutionAction(flowRunStartedAt, endedAt, durationMs),

Comment on lines +766 to +772
onFinishPerformExecution: async (endedAt, durationMs) => {
await onFinishPerformExecutionAction(
flowRunStartedAt,
endedAt,
durationMs,
);
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onFinishPerformExecution: async (endedAt, durationMs) => {
await onFinishPerformExecutionAction(
flowRunStartedAt,
endedAt,
durationMs,
);
},
onFinishPerformExecution: (endedAt, durationMs) =>
onFinishPerformExecutionAction(flowRunStartedAt, endedAt, durationMs),

Comment on lines +451 to +454
onFinishPerformExecution: (
endedAt: number,
durationMs: number,
) => Promise<void>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be Optional.

Suggested change
onFinishPerformExecution: (
endedAt: number,
durationMs: number,
) => Promise<void>;
onFinishPerformExecution?: (
endedAt: number,
durationMs: number,
) => Promise<void>;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants