Conversation
Summary of ChangesHello @majamassarini, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a lightweight, in-memory metrics tracking system for OGR API calls. Its primary purpose is to gather approximate usage statistics for different services and namespaces, which can then be aggregated and visualized in tools like Grafana. The implementation prioritizes simplicity and performance over strict thread-safety, acknowledging that some data loss is acceptable for trend analysis. This change provides valuable insights into API consumption patterns across various Git platforms. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a non-thread-safe metrics tracking system to count OGR API requests per namespace. A new decorator, @track_ogr_request, is added and applied to numerous methods across various service implementations (forgejo, github, gitlab, pagure) to enable this tracking. The changes also include comprehensive unit tests for the new metrics functionality, ensuring that the implementation is robust and that failures in metric collection do not impact the decorated functions. My review focuses on improving the typing in the new decorator and refining the testing patterns for better code quality and maintainability.
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 24s |
For Pagure projects, metrics are now tracked as "rpms/python-requests" instead of just "rpms" to provide better granularity per package.
1f06e60 to
54d66fb
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 32s |
betulependule
left a comment
There was a problem hiding this comment.
I like this implementation.
Regarding it not being thread-safe, I'm thinking, can't we just add locks whenever the _counts variable is being written to and read from? It doesn't seem like much of an issue, but I might be missing something. I understand that it would be nice to avoid introducing a deadlock to the code. 😅
|
Why are we not going with metaclasses here? it might be easier to just wrap everything, like we do for the exceptions, instead of this copy-paste… At the same time, the simple tracking of I'm also a bit afraid that this is something that's quite obviously service-specific, so I'm not sure how it fits the ogr in general. |
It would be nice to be able to use metaclasses, but I don't think we have a reliable/simple way to decide which methods need wrapping using metaclasses. This way is clearer/more explicit, from my point of view.
True, I missed that. Thanks! I can add the project url.
I partly agree. If the service weren't using only ogr, we'd need more logic there. But since most requests (if not all) go through ogr, I don't see why not. Counting requests where they're made seems correct to me. Also, ogr is a library, so this feature could be useful to other services. |
I think the implementation would be simple enough, but I'm concerned we might encounter deadlocks occasionally; when a task fails, a worker is killed, or in other edge cases. I'd like to avoid these situations since we only need a general trend of our request volume. |
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 22s |
This implementation isn't thread safe. I don't think we need to risk a deadlock just for tracking the trend of requests in a namespace. We can lose some of them.
Also, the aggregation of counts across different workers needs to be done in Grafana.
Should fix packit/packit-service#2912
Merge before packit/packit-service#3026