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(api) adds v4 usage to api tracking #4825

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

leohentschker
Copy link

@leohentschker leohentschker commented Dec 16, 2024

Resolves #4787 by checking usage over a set of versions (v3 and v4) instead of just v3 in the invert_user_logs function.

Note: I don't actually know that Redis interface is the same across v3 and v4! Definitely needs eyes from someone more familiar. Also, this is my first contribution to the project, would love help on any and all conventions to confirm I'm contributing in a way that is helpful.

@leohentschker leohentschker force-pushed the 4787-add-v4-usage-to-api-tracking branch 2 times, most recently from 36e0b3e to 155dd1f Compare December 16, 2024 01:16
@leohentschker leohentschker force-pushed the 4787-add-v4-usage-to-api-tracking branch from dd03451 to b429ae0 Compare December 16, 2024 01:18
@leohentschker leohentschker reopened this Dec 16, 2024
@leohentschker leohentschker force-pushed the 4787-add-v4-usage-to-api-tracking branch 2 times, most recently from d9e6887 to b166040 Compare December 16, 2024 01:46
Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

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

Thanks for the handy PR. I appreciate that you figured out where to work on the code and that you included a test. But I think you've got a bug, which I highlighted for you.

Let me know what you think, and thank you again!

cl/api/utils.py Outdated Show resolved Hide resolved
cl/api/utils.py Outdated Show resolved Hide resolved
@leohentschker
Copy link
Author

@mlissner -- I've updated this but I'm worried I'm wasting your time here with reviews! Any way that I can do some more testing on my own to make sure this is working as anticipated? I'm running API calls locally but I have yet to see them get tracked in the local redis store.

@mlissner
Copy link
Member

Yeah, I don't think it's quite right, but I'll assign it back to one of our devs who should be able to tweak and land this. (It was on her backlog anyway before you grabbed it!)

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog Dec 23 - Jan 10 (🎉)
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

API Recent Usage Page doesn't include v4 usage
2 participants