Skip to content

feat(ui): Split fetching of releases into two network requests #18795

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

Merged
merged 9 commits into from
May 26, 2020

Conversation

matejminar
Copy link
Member

@matejminar matejminar commented May 13, 2020

Screenshot 2020-05-25 at 12 58 36

Releases endpoint can be called with health: 1 parameter which will return releases with their health data.

This request can take significantly longer (especially for big orgs) compared to health: 0.

We decided to call health: 0 on the releases page and then fetch health: 1 lazily, showing placeholders in the meantime.

We will skip the two-phased load for sorting other than date created (default) since in those cases we already need to go to the session store anyways (which takes here the long time).

#sync-getsentry

@matejminar matejminar marked this pull request as ready for review May 25, 2020 16:22
@matejminar matejminar requested a review from a team May 25, 2020 16:22
[
'releasesWithHealth',
`/organizations/${organization.slug}/releases/`,
{query: {...query, health: 1}},
Copy link
Member

Choose a reason for hiding this comment

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

Have we considered having another endpoint that fetches only the health data without having to redo the work around getting a list of releases? Perhaps a bulk GET endpoint that takes a list of versions and returns the health data?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, and this might be something we want to try later. Generally it's a lot more work because it's conditional not just on the version but also each individual project and environment selection and we would need to pass a lot of data as GET parameters which might be way beyond the max payload size for URLs.

@@ -333,4 +346,11 @@ const ChartWrapper = styled('div')`
}
`;

const StyledPlaceholder = styled(Placeholder)`
height: 20px;
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this height if each component also sets the height to 25px?

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the height props (it was a leftover from the previous iteration)

@matejminar matejminar merged commit be31b59 into master May 26, 2020
@matejminar matejminar deleted the feat/releases-two-network-requests branch May 26, 2020 11:08
@mitsuhiko
Copy link
Contributor

@markstory one ironic thing about your proposal is that it turns out to be slower by my quick testing. Since we now fetch both requests at the same time (with and without stats data) the stats come in quicker than if you wait for the first request to finish to then load the second one. I did a quick experiment and the current — kinda hacky — version is likely to outperform the other one in most cases at the expense of transferring more data and keeping the server busier.

@markstory
Copy link
Member

@mitsuhiko I didn't consider that we'd have to make 2 requests in serial. Having to do that would make the overall flow slower. If the extra work we're putting on the server becomes a problem we can revisit the problem then 😄

@github-actions github-actions bot locked and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants