-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
(Contributor Recognition Infrastructure) Milestone 2.1: Add new frontend endpoints to call the ContributorStatsSummariesHandler #16153
Conversation
Merged with uostream
merged with develop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked at 2 codeowner files, just one comment for now
return ( | ||
this.contributionAndReviewStatsBackendApiService | ||
.fetchContributionAndReviewStatsAsync( | ||
'translation', 'submission', username)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These hardcoded strings ought to be named constants / enums, I think.
Also these are four roundtrip calls. How long does each take? I'm wondering about this strategy vs batching the GET into a single one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These hardcoded strings ought to be named constants / enums, I think.
Done
How long does each take?
Translation Submissions -> 0.0019922256469726562 s
Translation Reviews -> 0.002099275588989258 s
Question Submissions -> 0.0017049312591552734 s
Question Reviews -> 0.0022652149200439453 s
------------------------------------------------------
Total -> 0.00806164741 s
Also I have a separate endpoint fetch all stats at once in case to fetch everything from one request. It takes 0.008791923522949219 s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check, these are being measured starting from the frontend? Could you show a screenshot of where the measurement was done from? I'm a bit surprised because I'd have thought the network latency itself would be larger than this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I just checked it in the backend.
This when they are fetched from the frontend. The first record with 126 ms
corresponds to fetch everything from one request. The other four correspond for fetching stats for translation submissions, translation reviews, question submissions and question reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for codeowner files
Hi @chris7716, there is a new change in develop which needs to be in your PR. Please update your branch with the latest changes in develop. For instructions, refer to this link. Thanks! |
@seanlip PTAL. Thanks! |
Unassigning @chris7716 since a re-review was requested. @chris7716, please make sure you have addressed all review comments. Thanks! |
@ashutoshc8101 PTAL for codeowners. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for codeowners.
Unassigning @ashutoshc8101 since they have already approved the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chris7716! OK, sounds like there isn't much difference then. I think the rest of the PR looks good.
Hi @chris7716, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks! |
Overview
Essential Checklist
Proof that changes are correct
Proof of changes on desktop with slow/throttled network
Proof of changes on mobile phone
Proof of changes in Arabic language
PR Pointers