ERA-10519: [Events API Optimizations] Page Size change and Parallelization of calls #1202

gaboDev · 2024-12-05T22:23:26Z

What does this PR do?

It adds logic to handle parallel calls for pagination for map events fetch
It adds modification to fetch more events on feed

Relevant link(s)

Tracking tickets: ERA-10519
Hosted demo environments

luixlive · 2024-12-09T17:08:51Z

The deployment failed. An Eslint issue it seems?

luixlive

Well done Gabo. This is an interesting pattern that we can then reuse in other places in the code to optimize our requests. I have many questions and suggestions since the code is kind of hard to understand in certain places. Maybe adding comments or refactorizing the code to making more readable would help.

.env.development

src/SideBar/useReportsFeed/index.js

luixlive · 2024-12-09T18:28:17Z

src/ducks/events.js

@@ -441,17 +442,15 @@ export const fetchMapEvents = (map, parameters) => async (dispatch, getState) =>
  });

  let resultsToDate = [];
-  const onEachRequest = onePageOfResults => {
+
+  const onEachRequest = ({ results: onePageOfResults }) => {


We could follow a uniform language if we change the name of this method to onPageFetch 👍

luixlive · 2024-12-09T18:29:59Z

src/ducks/events.test.js

@@ -16,7 +16,15 @@ describe('fetchMapEvents', () => {
  });

  test('appending a bbox parameter from the map object', async () => {


Another good place to add an improvement. A long time ago we decided to stop mocking or spying requests. Instead now we intercept them and mock a server response with MSW so the client code runs intact and our test quality improves. We could add MSW handlers here and stop spying axios 😄

luixlive · 2024-12-09T18:30:35Z

src/utils/parallelPaginatedRequest.test.js

+    (_, i) => i
+    );
+
+    axios.get = jest.fn(() => {


Same than before, but here I'd say is more important since these tests are good. Let's not mock axios, let's use MSW to intercept requests 👍

src/utils/parallelPaginatedRequest.js

luixlive · 2024-12-09T19:00:24Z

src/utils/parallelPaginatedRequest.js

+      console.log('Wait for any fetch to complete.');
+    }
+
+    const fetchPromise = processPage(page)


We are mixing async / await and .then .catch patterns. Could we stay with async / await everywhere?

I got your point, but in this case I'm working with the reference of the promise not with the promise result.

Meaning that I want to set the finally callback for later when the promise is resolved/rejected in order to remove it
from the Set, and also need to add that promise to the pool, both of those steps need to be accomplished without waiting the promise to be solved (not blocking the code execution), the blocking process is only required when Promise.race enters in action.

src/utils/parallelPaginatedRequest.js

luixlive · 2024-12-09T19:06:40Z

src/utils/parallelPaginatedRequest.js

+
+  for (const page of pagesToFetch) {
+    while (activeFetches.size >= concurrencyLimit) {
+      await Promise.race([...activeFetches]);


Hmm I don't understand why we are using Promise.race here 🤔 The nature of this feature is not a race at all. We not only care about the response of the fastest request. We want to call 5 promises in parallel and everytime one responds just add another one, like a pool. It seems like everytime the race finishes, we will iterate in the while calling the Promise.race again with the 4 requests that didn't finish, causing doing the same requests several times (?) Could you explain a little the reasoning behind the race here please.

That's a typo, I thought I was fixed it, it should be an if statement

Regarding the Promise.race, it finishes when one of the promises of the set is processed, meaning it succeed or failed, so that result is taken and Promise.race if finished also, as far as I understand it only checks promises statuses so there is no request duplication

luixlive

The Promise.race is still confusing for me, I can't really link how a race is related to what we are doing here (I understand how it works though, just wonder if there is a more straightforward way to implement it). But now with the env I can see it working and it does what it is expected.

However, I get the impression that when using this new fetching system the environment gets kind of slow. But I'm not sure if I'm doing a false relationship here and maybe my computer is slow because of something else. I'd leave further testing on that to the QA team @fonzy911 @AlanCalvillo

luixlive · 2024-12-09T22:48:05Z

src/utils/parallelPaginatedRequest.js

+  return data;
+}
+
+async function parallelPaginatedRequest(apiUrl, requestConfig = {}, { itemsPerPage = 150, maxRetries = 3, concurrencyLimit = 5, onPageFetch = null } = {}) {


I'm ok having defaults I guess, but I see you added the defaults of the specific scenario you are working on (fetching map events). But this method is designed to be reused so maybe these are not the best generic defaults. I guess we shouldn't really have a default itemsPerPage since that will vary every time. The rest seems ok for me

gaboDev · 2024-12-09T23:01:37Z

The Promise.race is still confusing for me, I can't really link how a race is related to what we are doing here (I understand how it works though, just wonder if there is a more straightforward way to implement it). But now with the env I can see it working and it does what it is expected.

However, I get the impression that when using this new fetching system the environment gets kind of slow. But I'm not sure if I'm doing a false relationship here and maybe my computer is slow because of something else. I'd leave further testing on that to the QA team @fonzy911 @AlanCalvillo

We can discuss the implementation in more detail, may be the pool can be handled only by a counter or something way more simpler ? not sure haha

Also, I'm not sure if I'm getting wrongly the behavior/usage of Promise.race also it would be great to have more discussion on it

Regarding the slowness of the site, its quite a point, lets see QA results 👀

JoshuaVulcan

Nicely implemented 🙂 I did notice what feels like a UX problem; it seems like the map data may be getting cleared too early, before the newly-fetched data is retrieved. It's noticeable when zooming in towards a particular area, like this. Watch all the events disappear:

gaboDev · 2024-12-10T22:38:38Z

Nicely implemented 🙂 I did notice what feels like a UX problem; it seems like the map data may be getting cleared too early, before the newly-fetched data is retrieved. It's noticeable when zooming in towards a particular area, like this. Watch all the events disappear:

This change has been addressed, I sent the commit this morning but forgot to mentioned it here 🫡

fonzy911

Hi @gaboDev, this is looking great so far. I'm able to see the adjusted items number for both feed and map, however, just wanted to run something by you real quick @netomo.

When it comes to the map items, each batch of 150 within each page, is taking longer as they progress being fetched:

On root.dev, even though the map items count is lower (25) it is only taking this time to be fetched:

not sure if these are the numbers that we were expecting or if we should dive in a tad more.

AlanCalvillo · 2024-12-12T19:43:21Z

Hi @gaboDev, this is looking great so far. I'm able to see the adjusted items number for both feed and map, however, just wanted to run something by you real quick @netomo.

When it comes to the map items, each batch of 150 within each page, is taking longer as they progress being fetched:

On root.dev, even though the map items count is lower (25) it is only taking this time to be fetched:

not sure if these are the numbers that we were expecting or if we should dive in a tad more.

@fonzy911 this isn't an apples-to-apples comparison. root.dev is fetching way fewer events than feature-env, can you ensure you are testing similar amounts of data?

fonzy911 · 2024-12-16T20:59:37Z

Hi @gaboDev, this is looking great so far. I'm able to see the adjusted items number for both feed and map, however, just wanted to run something by you real quick @netomo.
When it comes to the map items, each batch of 150 within each page, is taking longer as they progress being fetched:

On root.dev, even though the map items count is lower (25) it is only taking this time to be fetched:

not sure if these are the numbers that we were expecting or if we should dive in a tad more.

@fonzy911 this isn't an apples-to-apples comparison. root.dev is fetching way fewer events than feature-env, can you ensure you are testing similar amounts of data?

Hi @AlanCalvillo, yes, thank you very much for that input. You are right, I have made the comparison now using my local instance, pointing to this backend: https://era-10519.dev.pamdas.org, and I see the following:

scoping this to map calls, I see that 6 calls of 25 map items take this amount of time to render:

Which is around 3.30seconds per 150 items

Now, with Gabo's fix, I see that 1 call of 150 map events, it takes around 2.24seconds to be completed

And the backend params are showing appropriately:

JoshuaVulcan · 2024-12-16T21:14:29Z

src/utils/parallelPaginatedRequest.js

@@ -0,0 +1,75 @@
+import axios from 'axios';
+
+async function fetchPage(apiUrl, requestConfig, page, pageSize, maxRetries) {


nit: we're pretty consistent throughout the repo with the const fetchPage = async (...whatever) convention. Why go async function instead?

JoshuaVulcan · 2024-12-16T21:19:20Z

src/utils/parallelPaginatedRequest.js

+  }
+
+  if (data === null) { // just logging in error, this place can be used to trigger a callback on maxRetries exhausted per page
+    console.error(`Failed to fetch page ${page} after ${maxRetries} attempts.`);


This is logging on fetch cancellations, which are intentional and happen quite frequently. As a result, the console is getting spammed in Chrome. Can we tighten that up to only log errors if they're not cancellations?

AlanCalvillo · 2024-12-16T23:09:16Z

Hi @gaboDev, this is looking great so far. I'm able to see the adjusted items number for both feed and map, however, just wanted to run something by you real quick @netomo.
When it comes to the map items, each batch of 150 within each page, is taking longer as they progress being fetched:

On root.dev, even though the map items count is lower (25) it is only taking this time to be fetched:

not sure if these are the numbers that we were expecting or if we should dive in a tad more.

@fonzy911 this isn't an apples-to-apples comparison. root.dev is fetching way fewer events than feature-env, can you ensure you are testing similar amounts of data?

Hi @AlanCalvillo, yes, thank you very much for that input. You are right, I have made the comparison now using my local instance, pointing to this backend: https://era-10519.dev.pamdas.org, and I see the following:

scoping this to map calls, I see that 6 calls of 25 map items take this amount of time to render:

Which is around 3.30seconds per 150 items

Now, with Gabo's fix, I see that 1 call of 150 map events, it takes around 2.24seconds to be completed

And the backend params are showing appropriately:

@fonzy911 Did you gather metrics around how long the client takes to load X number of events, not only on a per-call basis but on the overall sum?
E.g 15 secs to render 3000 events vs 30 secs to render the same 3000 events. I don't think comparing individual calls is enough since they might vary. So an overall time will give us a glance at how would be the real UX. (See https://allenai.atlassian.net/wiki/spaces/ER/pages/30470471682/Results for inspo)
As discussed earlier, we don't only want to validate AC in a vacuum but actually make sure that the foundational objective is being met, which for this is story is "Optimize the loading of ER events to improve the performance of the web app. ")

fonzy911 · 2024-12-17T19:51:01Z

https://allenai.atlassian.net/wiki/spaces/ER/pages/30470471682/Results

Hi @AlanCalvillo, yes, I was able to draft some numbers at a larger scale, this is what I got:

Without Parallelization, every call (for 25 items) takes in average 570ms to be completed

With Parallelization, it takes an average of 3.55 seconds for every 150 items call, making 591ms per every 25 items called.

AlanCalvillo · 2024-12-17T20:18:49Z

https://allenai.atlassian.net/wiki/spaces/ER/pages/30470471682/Results

Hi @AlanCalvillo, yes, I was able to draft some numbers at a larger scale, this is what I got:
Without Parallelization, every call (for 25 items) takes in average 570ms to be completed
With Parallelization, it takes an average of 3.55 seconds for every 150 items call, making 591ms per every 25 items called.

So if time w parallelization > w/o parallelization, to me, kinda suggests that the objective is not being met. why is it approved?

fonzy911 · 2024-12-17T20:23:34Z

https://allenai.atlassian.net/wiki/spaces/ER/pages/30470471682/Results

Hi @AlanCalvillo, yes, I was able to draft some numbers at a larger scale, this is what I got:

Without Parallelization, every call (for 25 items) takes in average 570ms to be completed
With Parallelization, it takes an average of 3.55 seconds for every 150 items call, making 591ms per every 25 items called.

So if time w parallelization > w/o parallelization, to me, kinda suggests that the objective is not being met. why is it approved?

Previously, when running on an individual basis, the Parallelization process seemed to be faster than the original feature, that's where I originally approved it, but yes, it seems that performance diminishes as calls number rises. I have removed my approval due to new findings.

JoshuaVulcan

Requesting changes as per the recent QA dialogue

gaboDev · 2025-01-09T17:28:52Z

https://allenai.atlassian.net/wiki/spaces/ER/pages/30470471682/Results

Hi @AlanCalvillo, yes, I was able to draft some numbers at a larger scale, this is what I got:

Without Parallelization, every call (for 25 items) takes in average 570ms to be completed
With Parallelization, it takes an average of 3.55 seconds for every 150 items call, making 591ms per every 25 items called.

So if time w parallelization > w/o parallelization, to me, kinda suggests that the objective is not being met. why is it approved?

Previously, when running on an individual basis, the Parallelization process seemed to be faster than the original feature, that's where I originally approved it, but yes, it seems that performance diminishes as calls number rises. I have removed my approval due to new findings.

After some meetings and discussions We agreed on changing the original QA strategy since it was not covering the whole process, it was changed to something more suitable in regards of the technical approach of this ticket

cc
@fonzy911

fonzy911

Hi @gaboDev Gabo,

Thank you very much for implementing the new feature and addressing my previous comments so thoughtfully. I really appreciate the effort you’ve put into this.

To provide additional context, I conducted some tests to evaluate the performance of the new parallelization code compared to the previous implementation. Here's a quick summary of my approach:

I loaded a local instance of the app without the parallelization code and measured the time it took to render 1000, 2000, and 3000 items.
I repeated the same process using an instance with the new parallelization code applied.
I then compared the rendering times for both setups.
Initially, the results showed that the instance with the parallelization code took slightly more time to load all the items.

After discussing this with the development team, they clarified that the primary objective of this story was to implement the parallelization feature. They emphasized that summing individual API response times (e.g., A=2s, B=3s, C=4s) is no longer an accurate measure due to concurrent execution. Instead, testing should focus on improvements in overall UX, error handling, and HTTP call pool behavior.

When I reviewed this live using a waterfall view in the network tab, I observed that the map items were indeed rendered while calls were being executed concurrently within the same period, which supports the devs' explanation.

Based on these findings, I’m approving this PR. Testing it in a higher environment will give us a clearer picture of how it behaves under more extensive conditions.

Great work, and thanks again for your effort on this!

cc: @amicavi , @JoshuaVulcan

Gabriel López added 2 commits November 28, 2024 17:24

WIP: testing initial approach

c80087d

Adding unit tests and refactors

6473af1

gaboDev added the deploy label Dec 5, 2024

gaboDev temporarily deployed to ERA-10519 December 5, 2024 22:26 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app December 5, 2024 22:26 — with GitHub Actions Inactive

Adding refactor

28f03c4

gaboDev temporarily deployed to ERA-10519 December 9, 2024 16:01 — with GitHub Actions Inactive

gaboDev requested review from JoshuaVulcan, luixlive, AlanCalvillo and fonzy911 and removed request for JoshuaVulcan December 9, 2024 16:01

gaboDev had a problem deploying to padas-app December 9, 2024 16:02 — with GitHub Actions Failure

gaboDev marked this pull request as ready for review December 9, 2024 16:02

Deleting debbug flag

db10f10

gaboDev temporarily deployed to ERA-10519 December 9, 2024 17:43 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app December 9, 2024 17:44 — with GitHub Actions Inactive

luixlive requested changes Dec 9, 2024

View reviewed changes

Adding PR refactor

6c4caf2

gaboDev temporarily deployed to ERA-10519 December 9, 2024 22:38 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app December 9, 2024 22:38 — with GitHub Actions Inactive

gaboDev requested a review from luixlive December 9, 2024 22:41

luixlive approved these changes Dec 9, 2024

View reviewed changes

JoshuaVulcan requested changes Dec 10, 2024

View reviewed changes

Fixing linter and adding validation to avoid map event clearence

7ce935b

gaboDev temporarily deployed to ERA-10519 December 10, 2024 16:46 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app December 10, 2024 16:46 — with GitHub Actions Inactive

gaboDev requested a review from JoshuaVulcan December 10, 2024 16:50

JoshuaVulcan approved these changes Dec 10, 2024

View reviewed changes

fonzy911 reviewed Dec 12, 2024

View reviewed changes

JoshuaVulcan reviewed Dec 16, 2024

View reviewed changes

JoshuaVulcan approved these changes Dec 16, 2024

View reviewed changes

fonzy911 self-requested a review December 16, 2024 21:16

fonzy911 approved these changes Dec 16, 2024

View reviewed changes

JoshuaVulcan reviewed Dec 16, 2024

View reviewed changes

fonzy911 self-requested a review December 17, 2024 20:20

JoshuaVulcan requested changes Dec 17, 2024

View reviewed changes

Removing unnecesarry logs

838efc4

gaboDev temporarily deployed to ERA-10519 December 19, 2024 00:45 — with GitHub Actions Inactive

gaboDev temporarily deployed to padas-app December 19, 2024 00:45 — with GitHub Actions Inactive

JoshuaVulcan approved these changes Jan 6, 2025

View reviewed changes

Adding timeouts to axios request for paralle query

5be7d2b

gaboDev deployed to ERA-10519 January 8, 2025 16:28 — with GitHub Actions View deployment

gaboDev temporarily deployed to padas-app January 8, 2025 16:29 — with GitHub Actions Inactive

fonzy911 approved these changes Jan 9, 2025

View reviewed changes

gaboDev merged commit 84b603a into develop Jan 9, 2025
3 checks passed

gaboDev deleted the ERA-10519 branch January 9, 2025 18:26

		@@ -16,7 +16,15 @@ describe('fetchMapEvents', () => {
		});

		test('appending a bbox parameter from the map object', async () => {

		@@ -0,0 +1,75 @@
		import axios from 'axios';

		async function fetchPage(apiUrl, requestConfig, page, pageSize, maxRetries) {

ERA-10519: [Events API Optimizations] Page Size change and Parallelization of calls #1202

ERA-10519: [Events API Optimizations] Page Size change and Parallelization of calls #1202

Uh oh!

Conversation

gaboDev commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Relevant link(s)

Uh oh!

luixlive commented Dec 9, 2024

Uh oh!

luixlive left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luixlive left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaboDev commented Dec 9, 2024

Uh oh!

JoshuaVulcan left a comment

Choose a reason for hiding this comment

Uh oh!

gaboDev commented Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fonzy911 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanCalvillo commented Dec 12, 2024

Uh oh!

fonzy911 commented Dec 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanCalvillo commented Dec 16, 2024

Uh oh!

fonzy911 commented Dec 17, 2024

Uh oh!

AlanCalvillo commented Dec 17, 2024

Uh oh!

fonzy911 commented Dec 17, 2024

Uh oh!

JoshuaVulcan left a comment

Choose a reason for hiding this comment

Uh oh!

gaboDev commented Jan 9, 2025

Uh oh!

fonzy911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gaboDev commented Dec 5, 2024 •

edited

Loading

gaboDev commented Dec 10, 2024 •

edited

Loading

fonzy911 left a comment •

edited

Loading