We already implemented GraphQL to render Epics Tree (see https://gitlab.com/gitlab-org/gitlab-ee/issues/10795), so API already supports all necessary keys required to render Roadmap for any group. Using GraphQL to render Roadmap will allow us to use single query file in both the places, thus having SSoT for networking logic for both Epics and Roadmap.
Current Rails API Implementation for Roadmap
Current Rails API for Roadmap supports following query params;
state: State of Epics to show, possible values; all, opened & closed.
start_date: Start date in YYYY-M-D format from which Epics with provided start date exist.
end_date: Finish date in YYYY-M-D format from which Epics with provided finish date exist.
layout [optional]: Preset value when user clicks it on UI, possible values; QUARTERS, MONTHS & WEEKS. This param is only sent when user changes the preset.
Also, the group information is part of endpoint URL itself. For eg; URL to fetch all Epics within gitlab-org group on GitLab.com is /groups/gitlab-org/-/epics.json, with above params sent as query params.
Current GraphQL API implementation for Epics
Current GraphQL query used for Epic Tree requires following variables;
fullPath: Group within which we're looking for Epics.
iid: ID of Epic for which we want to fetch child items (attached Epics & Issues).
pageSize: Number of items to fetch. Default upper limit for GraphQL backend is 100.
epicEndCursor [optional]: Cursor position for pagination in Epics list.
issueEndCursor [optional]: Cursor position for pagination in Issues list.
If we were to use GraphQL API for Roadmap at a group level, we'd have to make iid variable to be optional such that the query allows to fetch all top-level epics for provided group, and additionally support startDate, endDate, state and layout variables (all being optional when used within Epics to render tree).
Unfortunately, Matt is no longer with SFP. Please send any work related requests to Robert Theodorow at rob@sfp.net and we will be sure to handle the request in a timely manner. Thank you.
@engwan / @felipe_artur would you mind adding a weight here. The backend work is adding the following to the GraphQL API:
If we were to use GraphQL API for Roadmap at a group level, we'd have to make iid variable to be optional such that the query allows to fetch all top-level epics for provided group, and additionally support startDate, endDate, state and layout variables (all being optional when used within Epics to render tree).
@felipe_artur Heyo, looks like #5164 (closed) is blocked on this issue, any new updates here? Im trying to get an understanding of where we are so we can evaluate #5164 (closed) as part of the 12.3 milestone. Thanks.
We haven't been able to get to the frontend of this due to other priorities. The work left on the frontend is pretty minor, so will make sure we prioritize this first thing in %12.4 as there are other issues blocked by it.
Want to recap discussion we have had regarding pagination and limits on the roadmap page. Currently, using the REST API, we include all epics in the response. This way, the frontend can handle the sorting.
GraphQL has a default limit of 100, which changes things.
We need to eventually handle for this, as it is not scalable to keep sending all epics.
For the sake of this issue, I'd like to keep it as close to current state as using our REST API is.
@jprovaznik researched and it looks like it is possible to override the default limit per field, but also stated that we should try to avoid this. I agree, but I would like to take the time to properly UX and build a pagination approach. Is temporarily increasing the limit any worse off than what we're currently doing (not having a limit on the REST call)?
I agree, but I would like to take the time to properly UX and build a pagination approach. Is temporarily increasing the limit any worse off than what we're currently doing (not having a limit on the REST call)?
@donaldcook I agree, especially if there is not a limit at all, then just using a higher limit is still better
If there is not a limit at all on the REST API call, then it's a bug and we should fix it (no matter if we use GraphQL for epics). After a brief check it seems we use pagination for epics (https://gitlab.com/gitlab-org/gitlab/blob/master/ee/lib/api/epics.rb#L42) and max. value for per_page is 100. Could you please provide more details when a limit is not enforced?
@donaldcook ah, thanks for clarification - this is our "internal" API (used by our UI), not REST API. Yes you are right - pagination is not used there so using higher limits is still better than nothing . Thanks for creating #32822 (closed) - once this is implemented we can use normal limits (or add pagination to the controller, depending what is done earlier).
Until #32822 (closed) is done, do you think it is okay to increase the limit in GraphQL for the epics field like we originally suggested? Or maybe we just increase to an artificial limit (not sure if anyone has more epics than we do, so maybe limit to like 1000).
Currently, Roadmap for gitlab-org group on GitLab.com is showing around 500 epics (out of total 1500+ epics it has). As long as we don't end up with over 1000 epics with dates present, we'd be fine with this limit.
@kushalpandya@felipe_artur from my understanding review on this is approved on the frontend and in review on the backend. I'm moving it forward to ~"workflow::In review". If that is incorrect, please move it back.
Filtering by label, author, title and description View by quarters/month/year Ordering by start_date/due_date Epics shown on view
Before turning on the feature flag the first epic shown had start date from sep 2019 now it shows the first one from jun 2016. I am not sure if this behavior is expected. We should verify before turning the flag on for everyone.
I don't think changing page size is going to fix the problem, just so everyone understands what goes behind the scenes when Roadmap page gets loaded, I'm explaining everything happening step-wise (sorry for lengthy comment but I can't think of a good TL;DR to give you all a better idea );
Step - 1 - Frontend
Roadmap page is loaded with predetermined sorting & preset (Months, Weeks or Quarters) applied.
Roadmap Vue app uses viewport width to determine how many timeline columns we can fit for initial page load and such a timeline is rendered on UI.
There's no fixed upper or lower limit on number of columns, but app ensures that timeline is horizontally scrollable and column width is 180px.
Based on the timeframe we calculated (in step above), we now have startDate & dueDate for which we need to query Epics.
Here, any Epic which has either start or due date falling within timeframe start and due dates is to be fetched to show on UI.
Step - 2 - Backend
Graph query is made with variables containing timeframe start and due dates and sort order.
Backend queries all the Epics from the DB which fall within timeframe and then sorts them as per provided sort order.
Response is prepared containing epics and then sliced as per the configured page size and sent back to client.
Step - 3 - Frontend
Query response is parsed and Epics are rendered along with their timeline.
Horizontal timeline scroll listeners are initialized and we then wait for user to scroll the timeline in either direction.
Depending on which direction user has scrolled, we extend the timeline either in past or future.
When user scrolls in either of the edges, we make another graph query to fetch Epics for the extended part of timeline (with variables startDate, dueDate and sort order).
API response is received and then we insert new epics to existing timeline by sorting them so that they appear at correct location.
This sorting happens on frontend to ensure that any epic that didn't belong to older timeline but now belongs to newer timeline is shown in correct location within the list.
Due the dynamic nature of timeline that can continue to pull in new epics depending on scroll-direction, I'm not sure how this can be combined with GraphQL pagination which cuts off items from response even before they show up on UI. So no matter how we plan to handle pagination, every time user scrolls horizontally, timeframe is expanded and new epics are fetched.
Alternatives
Before we decided to go with timeframe extend on horizontal scroll behaviour of roadmap, we were exploring other ideas like exposing a date-range picker on UI where user determines the range and then we show results but I don't know why we didn't go with it (may be @pedroms would know). Personally, I'm still not a fan of endless scrolling just because of sorting and insertions happening very quickly as it has already become a performance problem. I can elaborate more about how this can be much easier to solve if we get rid of endless horizontal scrolling in favour of dumb date-range picker but I'm open to any other suggestions you all might have.
I'm still a bit confused. I think we should look into the alternatives, but until then, I thought increasing the limit to 2000 would essentially return everything that is yielded for the given timeframe, making it the same as our current JSON endpoint?
Due the dynamic nature of timeline that can continue to pull in new epics depending on scroll-direction, I'm not sure how this can be combined with GraphQL pagination which cuts off items from response even before they show up on UI.
If GraphQL pagination is set to 2000, I would expect that you always load everything withing single page?
If multiple pages would still have to be fetched, I wonder if you could make this paginated loading an atomic operation on FE side? IOW horizontal scrolling listeners would wait with loading items from the extended timeframe until loading of epics from the current timeframe is finished?
The way keyset pagination works is not only do we have to identify the specific key record that we're grabbing records before or after, but we have to know exactly how to specify in a where statement how to grab the records before or after the key record. We do that by parsing the ordering fields on the relation, and we only support two ordering fields at the moment, with one of those being the primary key.
So it's possible it needs additional smarts to handle that query (or the query's ordering fields need to get changed a little).
But, if you're querying using first: 2000 with no after or before cursor, it should be correctly pulling all the records and sorting them properly.
But, if you're querying using first: 2000 with no after or before cursor, it should be correctly pulling all the records and sorting them properly.
I don't see after or before cursor on GraphQL query and yes we added the 2000 limit. The main difference i see is that we are showing more epics including some with older start dates. @kushalpandya should we get into a call and verify if the current behavior is acceptable? That way we can toggle the flag to see the difference.
I verified this on GitLab.com for gitlab-org group, along with !19875 (merged), and while data is loaded as expected, here are some points to note;
On gitlab-org group, we have 750+ epics to show and GraphQL is enabled there. It takes over 5 seconds to load before anything can be rendered on UI.
On gitlab-com group, we have 250+ epics to show and GraphQL is not enabled. It takes ~3 seconds to load before anything can be rendered on UI.
So I'm not sure if the slow performance is due to sheer number of epics we have or something to do with GraphQL itself. I also suspect that if Rails API is performing at its best (for gitlab-com group), and is still taking 3 seconds to load, then may be we need to cap our page size at 100 epics, and that would mean adding a frontend support for pagination too, which is already an open discussion regarding its approach.
Do we know what the response time was when we were loading gitlab-org epics over the API? It seems like it would be comparable to the GraphQL time if the gitlab-com results are representative, in which case it's not a performance regression.
I re-ran the GraphQL query against a few projects and the response times did come down substantially with the number of Epics being returned:
I'd personally prefer not to ship a performance regression at this stage. I know we have a lot of epics relative to other groups, but a 20% decrease in performance is significant. Do you have any idea how we could improve the speed of the GraphQL query?
If we could identify a performance improvement in the GraphQL and schedule it for 12.8 then maybe it'd be OK. @donaldcook WDYT, particular from the user experience point of view?
I did some tests on `gitlab-com` group that has less epics. I took the querying timings.(click to expand)
5 calls to each endpoint
REST API (Flag disabled)3.54 s4.76 s4.13 s2.95 s4.12 s----------Average: 3.9 s GraphQL (Flag enabled)3.95 s3.56 s4.01 s3.58 s3.75 s----------Average: 3.77 s
Do you have any idea how we could improve the speed of the GraphQL query?
Based on the tests above, done in the same group with the same number of epics: The timings looks pretty much the same, however based on @donaldcook tests they seem to go up when the epics count are massive. Since we are using the same query to fetch epics on GraphQL and REST API i believe the problem here is not on GraphQL itself but not having pagination as @kushalpandya pointed out on #12887 (comment 269178432).
Our own dataset might be too small to gather conclusive evidence on what effect this will have on loading times. We could switch on the feature for .com and keep an eye on GraphQL response times site-wide.
Public dashboards are slow at the minute but the current 6hr average latency on the Groups::RoadmapController#show action is about 300ms.
When we can get a 24h average and a way to measure the GraphQL response time for this action (I think we can do that in Grafana also, or Kibana) we could turn this on.