feat(agents-api): add usage tracking + usage table #1316
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CI Feedback 🧐(Feedback updated until commit 1494adb)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||
![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 e559974 in 2 minutes and 20 seconds
More details
- Looked at
689lines of code in8files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. agents-api/agents_api/clients/litellm.py:156
- Draft comment:
Use a logging library instead of print for error tracking in embedding usage. - Reason this comment was not posted:
Marked as duplicate.
2. agents-api/agents_api/queries/usage/create_usage_record.py:54
- Draft comment:
Avoid using mutable default arguments for metadata in function parameters. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
Looking at the code, while using mutable default arguments is generally considered bad practice, in this specific case: 1) The metadata parameter is only used once at line 109 with "metadata or {}" which creates a new dict if metadata is falsy 2) The metadata is immediately passed to the SQL query and not stored or modified 3) There's no risk of shared state between calls since the dict is not modified.
While the comment identifies a real Python anti-pattern, am I being too dismissive of a legitimate code quality issue that could cause problems if the code is modified in the future?
The current code usage is safe and the "metadata or {}" pattern effectively prevents any issues. The comment, while technically correct, doesn't represent a real problem in this specific implementation.
While the comment identifies a real Python anti-pattern, in this specific case the code is implemented safely and the issue is theoretical rather than practical. The comment should be removed as it doesn't represent a real problem that needs fixing.
3. agents-api/agents_api/queries/usage/create_usage_record.py:98
- Draft comment:
Consider using a logging library instead of print for fallback pricing notifications. - Reason this comment was not posted:
Marked as duplicate.
4. agents-api/agents_api/clients/litellm.py:118
- Draft comment:
Avoid shadowing the built-in name 'input'. Consider renaming this variable (e.g., to 'input_texts' or 'raw_inputs'). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. agents-api/agents_api/clients/litellm.py:96
- Draft comment:
Use a proper logging mechanism instead of print for error reporting in usage tracking. - Reason this comment was not posted:
Marked as duplicate.
6. agents-api/agents_api/clients/litellm.py:156
- Draft comment:
Use a proper logging mechanism instead of print for error reporting in embedding usage tracking. - Reason this comment was not posted:
Marked as duplicate.
7. agents-api/agents_api/common/utils/usage.py:23
- Draft comment:
Avoid using a mutable default argument for 'metadata'. Use None as the default and assign an empty dict inside the function. - Reason this comment was not posted:
Marked as duplicate.
8. agents-api/agents_api/common/utils/usage.py:86
- Draft comment:
Avoid using a mutable default argument for 'metadata'. Use None as the default and assign an empty dict inside the function. - Reason this comment was not posted:
Marked as duplicate.
9. agents-api/agents_api/queries/usage/create_usage_record.py:54
- Draft comment:
Avoid using a mutable default argument for 'metadata'. Use None as the default and assign {} inside the function to prevent unexpected behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
Using mutable default arguments in Python can lead to unexpected behavior when the same default object is shared between function calls. However, in this case, the code already handles this safely by using 'metadata or {}' when building params. The actual risk of bugs is very low here since the metadata dict isn't modified within the function.
While using mutable defaults is generally considered a Python anti-pattern, the current code already has a safe guard. The comment might be technically correct but not practically important.
Given that the code already safely handles the metadata with 'or {}' and doesn't modify the dict, this is more of a style preference than a real issue.
While technically correct, this comment addresses a theoretical issue that's already properly handled in the implementation. The suggested change wouldn't meaningfully improve the code.
Workflow ID: wflow_0EgharEWFmi2lsWz
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.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on f6226ed in 56 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. agents-api/agents_api/queries/usage/create_usage_record.py:54
- Draft comment:
Good catch on using None as the default for metadata to avoid mutable defaults. Consider updating the docstring (line 66) to clarify that metadata is Optional[dict[str, Any]]. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
Workflow ID: wflow_mfgg60upNroHLAMC
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.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 7b0fad7 in 31 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. agents-api/agents_api/queries/usage/create_usage_record.py:54
- Draft comment:
Good update: the metadata parameter in the docstring now correctly reflects that it can be None. Ensure this optional behavior is consistently handled in all callers. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. agents-api/agents_api/queries/usage/create_usage_record.py:66
- Draft comment:
Good update clarifying thatmetadatacan be None. This ensures the docstring aligns with the function signature. Consider using Optional[dict] if a more explicit type hint is preferred. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. agents-api/agents_api/queries/usage/create_usage_record.py:27
- Draft comment:
Typographical issue: In the SQL query definition, the closing part of the column list (after 'metadata') uses a colon (':') on line 27 instead of a closing parenthesis (')'). This might lead to SQL syntax issues. Please update ':' to ')' to fix the format. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_HqgpzXka6vtgYs3S
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on fb478b3 in 52 seconds
More details
- Looked at
415lines of code in6files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. agents-api/agents_api/common/utils/usage.py:21
- Draft comment:
Consider replacing the mutable default for 'metadata' (dict) with None to avoid potential side-effects. E.g., use 'metadata: dict[str, Any] | None = None' and then inside the function assign 'metadata = metadata or {}'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. agents-api/agents_api/clients/litellm.py:12
- Draft comment:
Minor formatting: The import order change looks cosmetic but ensures consistency; no action needed if that's intended. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
3. agents-api/agents_api/queries/usage/create_usage_record.py:78
- Draft comment:
Fallback pricing is computed correctly when cost_per_token fails. Logging a message via print is acceptable, but consider using a structured logging framework for production reliability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. agents-api/tests/test_usage_tracking.py:192
- Draft comment:
Tests are comprehensive and cover both scenarios with and without response.usage. No changes required. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
5. agents-api/agents_api/clients/litellm.py:118
- Draft comment:
Avoid using the variable name 'input' as it shadows the built-in. Consider renaming it (e.g., to 'payload' or 'inputs_list'). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. agents-api/agents_api/common/utils/usage.py:22
- Draft comment:
Avoid using mutable default arguments for 'metadata'. Use None as the default and then assign {} inside the function to prevent unintended side effects. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. agents-api/agents_api/common/utils/usage.py:86
- Draft comment:
Avoid using mutable default arguments for 'metadata' in track_embedding_usage. Use None as the default and assign {} within the function. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. agents-api/agents_api/queries/usage/create_usage_record.py:120
- Draft comment:
Consider using a logger instead of print for reporting missing fallback pricing. This change would improve maintainability and consistency with the rest of the codebase. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_2JaMdrgR9TPh3U2G
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 9eb2e3b in 1 minute and 10 seconds
More details
- Looked at
85lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. agents-api/agents_api/queries/usage/create_usage_record.py:120
- Draft comment:
Avoid using print statements for error logging. Consider using a logging framework to record missing fallback pricing info. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. agents-api/agents_api/queries/usage/create_usage_record.py:111
- Draft comment:
Consider logging the exception details in the cost calculation try/except block to aid in debugging. - Reason this comment was not posted:
Comment was on unchanged code.
3. agents-api/agents_api/queries/usage/create_usage_record.py:120
- Draft comment:
Avoid using print statements in production code; consider using a proper logging mechanism. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. agents-api/agents_api/queries/usage/create_usage_record.py:106
- Draft comment:
Catching a broad exception here may mask specific errors. Consider catching specific exceptions raised by cost_per_token. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_pcJ6nmGSxOluG7o5
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.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5de4fb2 in 45 seconds
More details
- Looked at
27lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. agents-api/agents_api/queries/usage/create_usage_record.py:115
- Draft comment:
Use proper logging for missing fallback pricing instead of print(). This is best practice for production code. - Reason this comment was not posted:
Comment was on unchanged code.
2. agents-api/agents_api/queries/usage/create_usage_record.py:112
- Draft comment:
Ensure exception handling in cost calculation logs the error or provides context for debugging. - Reason this comment was not posted:
Comment was on unchanged code.
3. agents-api/agents_api/queries/usage/create_usage_record.py:16
- Draft comment:
Good job renamingfallback_pricingtoFALLBACK_PRICINGto follow constant naming conventions. Ensure this naming is consistently used across the module. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
4. agents-api/agents_api/queries/usage/create_usage_record.py:121
- Draft comment:
Avoid usingprint()for fallback logging. Consider using a proper logging mechanism (e.g., using theloggingmodule) for better control in production. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_wwgEeO3Cj1npKxYE
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
User description
Closes #846
PR Type
Enhancement, Tests
Description
Added usage tracking for LLM API calls, including token usage and costs.
track_usageandtrack_embedding_usageutilities for tracking.acompletionandaembeddingfunctions.Created a new
usagetable in the database to store usage data.Added tests for usage tracking and database interactions.
Added database migrations for creating and dropping the
usagetable.Changes walkthrough 📝
5 files
Integrated usage tracking into LLM API client functionsAdded utilities for tracking token usage and costsAdded `usage` module to queries packageIntroduced `usage` module for database operationsAdded function to create usage records in the database1 files
Added tests for usage tracking and database interactions2 files
Added migration script to drop `usage` tableAdded migration script to create `usage` tableImportant
Add usage tracking for LLM API calls with a new
usagetable and integrate tracking into existing functions, including tests.track_usageandtrack_embedding_usageutilities inusage.pyfor tracking LLM API calls.acompletionandaembeddinginlitellm.py.usagetable to store usage data, including developer ID, model, token counts, costs, and metadata.000036_usage.up.sqland000036_usage.down.sqlfor creating and dropping theusagetable.test_usage_tracking.pyto verify usage tracking and database interactions, including cost calculations and fallback pricing.This description was created by
for 5de4fb2. It will automatically update as commits are pushed.