For all I can gather, this feature cannot be delivered in the current form due to technical constraints.
Multiple concerns and possible solutions for them were raised on the backend MR, but none meets all the criteria.
The main problem is that the database queries cannot be made performant enough to meet the requirements of around 250-300ms uncached query time.
mean_time_to_merge query for GitLab project (so a big one) for all of the merged MRs (not limited to any time range) is about 7 seconds. Adding extra filter like a time range might make the execution time worse or improve it, depending on the filters. Most projects would be performant enough, but we have to deal with largest projects too.
Aggregation and caching (redis) was proposed, but is not an option, since we have to compute the results on the fly, in order to take the various filters into account.
Another proposed solution was to split the queries (12 queries for monthly data for a year time range, done by the frontend, then aggregated). The problem with this solution is that it might still stress the database too much, and also that a malicious user fo the GraphQL API may anyway trigger a long lasting query.
Show MTTM for the last 30 days? That should be manageable.
Another proposed solution was to split the queries (12 queries for monthly data for a year time range, done by the frontend, then aggregated). The problem with this solution is that it might still stress the database too much, and also that a malicious user fo the GraphQL API may anyway trigger a long lasting query.
I still think that, multiple queries are the way to go. The FE could request them in separate requests, so we don't overload the DB.
@blabuschagne could you give us a sense of the effort required to have the frontend request MTTM data month-by-month, versus simply requesting data for the most recent month? What's the best course to take, at least for now?
Fetching the most recent month (last 30 days) will by far be the easiest solution, but fetching the data month-by-month for the given date range will definitely be manageable. It'll be significantly more complex as we'll need to make use of an additional number of utilities and increase our test coverage - but if it's for the benefit of performance then it's worth the extra effort 💪
Question for you and @ahegyi about requesting data by month: What happens if a user sets the date range to 36 months? Then there would be 36 requests from the frontend? That seems like it could create its own problem. Should we also limit the date range to 12 months max?
Then there would be 36 requests from the frontend?
That's correct!
Should we also limit the date range to 12 months max?
TL;DR;
Unless @ahegyi thinks we need a limit, I'm happy without one.
Longer version;
The number of queries shouldn't really cause any problems. They're hopefully small enough and should therefore resolve very quickly. We can handle them async on the frontend so that a user isn't restricted from interacting with other parts of the page while the batch of queries resolve.
We could introduce this feature without a limit and always add one if we find that it's needed, as it'll be a super easy change.
Making 36 separate API requests is a bit heavy. Putting the GraphQL queries into one request is also too much.
We need a limit on the date range within MR analytics (max 1 year), on the feature level. On the GraphQL side it'd be great to have an extra validation which prevents filtering data for more than one month. This should address the DB review concerns.
So we'd make 1 GraphQL API request with 12 queries in the body. Optionally we could split this into 2 or 3 GraphQL API requests on the FE side.
@ljlane Should we rather schedule this fo %13.9? We discussed that we'd like to leverage the Single Stat component for this. Once the open discussion has been resolved I'm going to create an implementation issue for the component in gitlab-ui that we can schedule for %13.8. WDYT?
The issue mentioned above has been moved into review, also the BE MR has been merged - I think it's safe to move this into workflowready for development
@ljlane Good catch! Typically backend comes up with an initial documentation draft and hands it over to frontend for adding any missing information (like screenshots). I think in this case we missed adding the docs because the API changes have been implemented a while ago while FE worked on this just recently.
I just created a MR for updating our docs here: !53956 (merged)