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

Fix 7955: allow datetime filtering on /spend/logs #7956

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

Conversation

rewbs
Copy link

@rewbs rewbs commented Jan 24, 2025

Allow datetime filtering on /spend/logs

This change allows you to query the spend logs with a datetime range filter. This functionality is currently unavailable (the existing start_date and end_date parameters do something altogether different).

Relevant issues

Implements #7955 7955

Type

🆕 New Feature

Changes

  • Add additional params to /spend/logs
  • Improve documentation of /spend/logs
  • Pass new params down the chain of query functions, without changing their overall structure (though I am not sure why we go from the spendlog API implementation into a generalised query function which then branches on the table type anyway? seems like an unnecessary level of indirection).

[REQUIRED] Testing - Attach a screenshot of any new tests passing locally

I have not included tests yet, happy to add them if this PR is considered desirable.

Copy link

vercel bot commented Jan 24, 2025

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

Name Status Preview Comments Updated (UTC)
litellm ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 24, 2025 0:43am

@krrishdholakia
Copy link
Contributor

Hi @rewbs this is great. Can you share a screenshot of this working for you as expected?

@@ -36,6 +36,7 @@ opentelemetry-exporter-otlp==1.25.0
sentry_sdk==2.2.1 # for sentry error handling
detect-secrets==1.5.0 # Enterprise - secret detection / masking in LLM requests
cryptography==43.0.1
python-dateutil==2.9.0.post0
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this dependency?

Copy link
Author

Choose a reason for hiding this comment

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

IMO yes, because I don't think it's worth re-implementing flexible datetime parsing. It is a mature and maintained library.

@rewbs
Copy link
Author

rewbs commented Jan 24, 2025

Hi @rewbs this is great. Can you share a screenshot of this working for you as expected?

Sure, here are a couple of curl requests. In the first you can see a start_time feature just a date, as well as an api_key filter. There are 2 matching requests for that key after the start_time. In the second request, we add an end_time parameter with a full datetime stamp that excludes the later request.

image

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