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

feat(api): include v4 in recent API usage #4881

Merged
merged 8 commits into from
Jan 3, 2025
Merged

Conversation

elisa-a-v
Copy link
Contributor

Closes #4787

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.

I didn't check the tests, but the approach looks good to me. Eduardo, I'm going to assign to you because you're helping Elisa get set up, but I don't have a handle on your workload or that of @albertisfu, so if you feel busy, can you guys please discuss and see who makes more sense this sprint? And I'm happy to break a tie if you both feel busy?

@mlissner mlissner assigned ERosendo and unassigned mlissner Jan 3, 2025
cl/api/utils.py Outdated
Comment on lines 757 to 759
for d, v3_data, v4_data in zip(dates, results[::2], results[1::2]):
for user_id, count in chain(v3_data, v4_data):
update_user_counts(user_id, count, d)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use the itertools.batched function in the outer loop to process the data in chunks. This function iterates over the list, yielding tuples of items with a maximum size of n.

Here's the documentation for reference: itertools.batched

Suggested change
for d, v3_data, v4_data in zip(dates, results[::2], results[1::2]):
for user_id, count in chain(v3_data, v4_data):
update_user_counts(user_id, count, d)
for d, api_usage in zip(dates, batched(results, 2)):
for user_id, count in chain(*api_usage):
update_user_counts(user_id, count, d)

This approach enhances future maintainability. If a new API version (e.g., v5) is introduced, we simply need to adjust the batch size instead of modifying the slicing expressions for previous versions (v3, v4) and adding a new slicing expression for v5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a versions array and used len(versions) instead of a hardcoded 2 so now we'd simply have to add "v5" to the versions array. What do you think? it does add a new nested for loop but it's a fixed and small number of inner iterations (just a couple of versions) so I'm guessing it's not too bad and it's probably worth the tradeoff.

Copy link
Contributor

Choose a reason for hiding this comment

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

@elisa-a-v looks great!

@ERosendo ERosendo assigned elisa-a-v and unassigned ERosendo Jan 3, 2025
@elisa-a-v elisa-a-v requested a review from ERosendo January 3, 2025 19:01
@elisa-a-v elisa-a-v assigned ERosendo and unassigned elisa-a-v Jan 3, 2025
@ERosendo ERosendo merged commit a93dbb0 into main Jan 3, 2025
15 checks passed
@ERosendo ERosendo deleted the 4787-include-v4-usage branch January 3, 2025 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

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