fix(quota): add reset_time_iso to quota stats API response #100
Conversation
|
I'm starting my review of the quota stats enhancement. Adding ISO timestamps is a great move for readability. Checking the implementation details now! |
![mirrobot-agent[bot]](https://avatars.githubusercontent.com/in/2140342?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Assessment
This is a solid improvement to the quota stats API. Adding human-readable timestamps significantly improves the usability of the /v1/quota-stats endpoint for developers and follows the established _iso suffix pattern in the project.
Architectural Feedback
The approach of adding a dedicated helper method _format_timestamp_iso is clean and maintainable. It aligns well with the existing _format_timestamp_local helper and centralizes the formatting logic.
Key Suggestions
- ISO Formatting: I've suggested a minor tweak to the
isoformat()call to ensure it matches theZsuffix mentioned in the docstring and avoids unnecessary microsecond precision. - API Consistency: Since the endpoint already returns
window_start_ts, addingwindow_start_isoalongsidereset_time_isowould provide a more consistent experience for API consumers.
Nitpicks and Minor Points
- I noticed that
src/rotator_library/client.pycontains a similar inline implementation of this ISO formatting. While not urgent, moving this helper to a shared utility module in the future could help reduce duplication.
Questions for the Author
None.
This review was generated by an AI assistant.
| def _format_timestamp_iso(self, ts: Optional[float]) -> Optional[str]: | ||
| """ | ||
| Format Unix timestamp as ISO 8601 UTC string for API responses. | ||
| Args: | ||
| ts: Unix timestamp or None | ||
| Returns: | ||
| Formatted string like "2025-12-07T14:30:17Z" or None | ||
| """ | ||
| if ts is None: | ||
| return None | ||
| try: | ||
| return datetime.fromtimestamp(ts, tz=timezone.utc).isoformat() | ||
| except (OSError, ValueError, OverflowError): | ||
| return None |
There was a problem hiding this comment.
The current implementation of isoformat() on a timezone-aware datetime will produce a +00:00 offset instead of the Z suffix mentioned in the docstring. Additionally, it may include microseconds if the timestamp has them. For a cleaner API response that matches the docstring, consider:
| def _format_timestamp_iso(self, ts: Optional[float]) -> Optional[str]: | |
| """ | |
| Format Unix timestamp as ISO 8601 UTC string for API responses. | |
| Args: | |
| ts: Unix timestamp or None | |
| Returns: | |
| Formatted string like "2025-12-07T14:30:17Z" or None | |
| """ | |
| if ts is None: | |
| return None | |
| try: | |
| return datetime.fromtimestamp(ts, tz=timezone.utc).isoformat() | |
| except (OSError, ValueError, OverflowError): | |
| return None | |
| try: | |
| return datetime.fromtimestamp(ts, tz=timezone.utc).isoformat(timespec="seconds").replace("+00:00", "Z") | |
| except (OSError, ValueError, OverflowError): |
| "window_start_ts": model_stats.get("window_start_ts"), | ||
| "quota_reset_ts": model_stats.get("quota_reset_ts"), | ||
| "reset_time_iso": self._format_timestamp_iso( | ||
| model_stats.get("quota_reset_ts") | ||
| ), |
There was a problem hiding this comment.
For consistency with reset_time_iso, consider adding window_start_iso as well. This provides a readable format for both key time metrics in the response.
| "window_start_ts": model_stats.get("window_start_ts"), | |
| "quota_reset_ts": model_stats.get("quota_reset_ts"), | |
| "reset_time_iso": self._format_timestamp_iso( | |
| model_stats.get("quota_reset_ts") | |
| ), | |
| "window_start_ts": model_stats.get("window_start_ts"), | |
| "window_start_iso": self._format_timestamp_iso( | |
| model_stats.get("window_start_ts") | |
| ), | |
| "quota_reset_ts": model_stats.get("quota_reset_ts"), | |
| "reset_time_iso": self._format_timestamp_iso( | |
| model_stats.get("quota_reset_ts") | |
| ), |
|
Damn, Opened and closed within 5 minutes. |
2 participants
Summary
reset_time_isofield to model stats in/v1/quota-statsAPI responseChanges
_format_timestamp_iso()method to format Unix timestamps as ISO 8601 UTC stringsreset_time_isofield to model stats dict inget_stats_for_endpoint()Context
Previously, the
/v1/quota-statsendpoint returned onlyquota_reset_ts(Unix timestamp). Now it also returnsreset_time_iso(ISO 8601 UTC string) following the established_isosuffix pattern.Important
Adds
reset_time_isoto/v1/quota-statsAPI response for ISO 8601 UTC timestamp of quota reset time.reset_time_isofield to/v1/quota-statsAPI response, providing ISO 8601 UTC timestamp for quota reset time.quota_reset_ts(Unix timestamp) was returned._format_timestamp_iso()inusage_manager.pyto convert Unix timestamps to ISO 8601 UTC strings.get_stats_for_endpoint()inusage_manager.pyto includereset_time_isoin model stats._isosuffix pattern for human-readable timestamps.This description was created by
for 96d8d01. You can customize this summary. It will automatically update as commits are pushed.