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

Implement agent time recording and reporting to playground v2 #251

Merged

Conversation

satococoa
Copy link
Contributor

Summary

Implement these changes into v2(current) playground.

Testing

  • agentActivities and agentTimeUsageReports are successfully saved
  • Stripe receives meter events.

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

vercel bot commented Dec 18, 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 2:23am

@satococoa satococoa changed the title Implement agent time record and reporting to playground v2 Implement agent time recording and reporting to playground v2 Dec 18, 2024
@shige
Copy link
Member

shige commented Dec 18, 2024

@satococoa Cloud you fix the CI error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was made to avoid unnecessary initialization in testing and specific runtime environments, and to enable more flexible module mocking.
Previously, the Stripe instance was initialized immediately upon module import, causing errors during tests without an API key and making it difficult to mock.

By switching to lazy initialization — deferring the actual Stripe setup until the first time it’s accessed — we can now skip initialization during testing or easily swap in mock instances. As a result, our code becomes more testable, maintainable, and adaptable.

As an alternative, I also considered introducing vitest to leverage its advanced module mocking capabilities whith bun:test doesn't have.
However, transitioning to a different test framework would involve a large scope of changes, so I decided to postpone that option for now.

The lazy initialization approach provides a simpler, more incremental improvement to address our current needs.

@satococoa
Copy link
Contributor Author

@toyamarinyon
I've merged the retry mechanism from the main branch into this feature branch.

Could you please:

  1. Review the changes
  2. Verify that the retry recording agent properly integrates with the retry mechanism

Changes made:

  • Merge main branch and integrated retry mechanism.
  • Updated recording agent implementation

Thank you for your time.

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.

Verify that the retry recording agent properly integrates with the retry mechanism

Please tell me what and how do I verify it? Reviewing code? If should I do some operations, please let me know that.

Comment on lines +604 to +609
await recordAgentUsageAction(
currentExecution.runStartedAt,
runEndedAt,
currentExecution.durationMs,
);

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@toyamarinyon toyamarinyon self-requested a review December 19, 2024 01:58
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.

Sorry. I thought I had commented on it, but it was Approve, so I will redo it.

Copy link
Member

@shige shige left a comment

Choose a reason for hiding this comment

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

LGTM! ⏲️

@toyamarinyon
Copy link
Contributor

Why am I meant to have approved 🤔?

image

@satococoa
Copy link
Contributor Author

@toyamarinyon

Please tell me what and how do I verify it? Reviewing code? If should I do some operations, please let me know that.

Sorry for any confusion, simply I want you to re-review code. (Because conflict has occurred.)


Conflicts is happening again, I'll fix it and request re-review again.

@satococoa
Copy link
Contributor Author

@toyamarinyon

I've resolved the merge conflicts.
Could you please check that your changes are correctly preserved?

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 for resolving the conflict so many times!

@satococoa satococoa merged commit 5e1f183 into main Dec 19, 2024
9 checks passed
@satococoa satococoa deleted the implement-agent-time-record-and-reporting-to-playground-v2 branch December 19, 2024 02:34
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.

3 participants