Split VSA metrics into 2 rows

changed milestone to %15.0

added devopsmanage featureenhancement frontend groupoptimize sectiondev workflowready for development + 1 deleted label

set weight to 1

marked this issue as related to #355304 (closed)

Thanks, @blabuschagne I will update to the latest design.

Thanks @blabuschagne, one additional point I think there may actually be a backend component to this.

Looking a little closer, the current tiles are structured based on the data we receive from 2 separate API endpoints. 2 Requests are made and they return a different mix of data:

Summary: returns issues, deploys, deployment frequency, and for project VSA it also includes commits
Time summary: returns lead time, cycle time, lead time for changes

The new designs group the data into:

DORA: lead time for changes and deployment frequency
Key metrics: commits, issues, deploys, lead time and cycle time

Based on the current api requests, to achieve the new groupings we would need to wait until both sets of API requests complete, then manually re-group / re-order the metrics on the frontend.

I think a better approach would be to re-group how we return the data from the API, this would remove the extra unnecessary sorting on the frontend, leaving the only frontend changes we would need for this, to be:

Add the header(s)
Fix the layout (2 separate rows separated by their heading instead of all in a single row)

Additionally, it might make sense to rename the endpoints to better reflect the data they expose, eg. (analytics/value_stream_analytics/dora_metrics, analytics/value_stream_analytics/key_metrics)

Any thoughts @pshutsin @m_frankiewicz ?

@ekigbo one thing to consider is that, as I understand, the summary endpoint at project level is available also on non-EE, while the time_summary is EE-only. This means that if we re-group the data on these two endpoints we also change what is visible for non-EE customers.

@ekigbo one thing to consider is that, as I understand, the summary endpoint at project level is available also on non-EE, while the time_summary is EE-only. This means that if we re-group the data on these two endpoints we also change what is visible for non-EE customers.

Thanks, that's a good point @m_frankiewicz @pshutsin.

Currently at the project level we check for the cycleAnalyticsForGroups license, then either make both API requests (summary/time_summary), or just a single request (summary).

Perhaps it would make more sense to perform the license checks within the different endpoints and only return the metric values the project/group should have access to. That way the frontend could simply make the API requests and only render what is returned. WDYT?

@ekigbo from a technical standpoint, that looks feasible (although it does require non trivial changes). But if we do that, would it make more sense at that point to have a single endpoint that returns all licensed metrics?

It also depends on which other frontend components use the current endpoints, and whether they would play well with this change or not. It looks to me (but I might be misunderstanding how the frontend is organized) that those two endpoints are always used together, and the decision whether to call only one or both is always depending on the license. If that's the case, having a single endpoint authorizing and selecting the data to expose on the backend might be a good idea.

In other words, if the current split into two endpoints is only motivated by the different licensing (I am not sure of this, but it seems to be the case) we either should keep things this way and regroup data in the frontend, or merge them into a single endpoint and decide in the backend what to expose depending on licensing.

WDYT?

Thanks @m_frankiewicz, im pretty sure VSA would be the only place using these endpoints currently, but i havent confirmed yet. I dont particularly mind if we have a single endpoint or keep the 2 current endpoints, but i think either way performing license checks on the backend for each metric makes the most sense, the original goal as far as i recall, was for the metrics tiles to just render all the data they receive, I think we shouldnt need to make decisions on the frontend about what to render.

Moving to a single endpoint might need a bit of reworking on the frontend for the value_stream_metrics component, this component is also used in ci/cd analytics, but im a bit less familliar with where the data is fetched for that so i'd need a closer look to understand the impact. Some additional work will be required to add the headings in the image above, but that should be minimal. IMO a single endpoint would only make sense if it also grouped the metrics togethor by their type (dora_metrics/key_metrics), something like this:


`GET /groups/:group_id/-/analytics/value_stream_analytics/metrics`

{
 "dora_metrics": [
  { 
  "identifier":"",
  "value":"",
  "title":"",
  "unit":"",
  "links":[]
 }
 ...
 ],
 "key_metrics": [
  { 
   "identifier":"",
   "value":"",
   "title":"",
   "unit":"",
   "links":[]
  }
  ...
  ],
}

A structure like this would make it easier to integrate the single endpoint with the current frontend structure. And should make it easier as we add more metrics.

I think 2 endpoints might still be good to give us flexibility about how we use this data, but it might also be a case of holding onto something we might not really need, so im probably fine with either approach.

Let me know if this doesnt make sense and we need to sync on this!

/cc @pshutsin

@ekigbo thanks for that! I think there are two reasonable options here:

we leave things as they are in the backend and reshuffling is taken care of in the frontend
we first refactor the backend to have one single endpoint and the license check is done in the backend (this I would weight at ~3).

a single endpoint would only make sense if it also grouped the metrics togethor by their type (dora_metrics/key_metrics)

I agree, that's what I thought as well.

@ekigbo we discussed this in our weekly call today. Do you agree with this option:

we leave things as they are in the backend and reshuffling is taken care of in the frontend

cc @danmh @m_frankiewicz

Thanks for the ping @hsnir1, Yes i agree with this as a way forward, ideally it would be great to improve this api at the moment, but I think the callout of where we can best use our capacity makes sense.

From the weekly call, this is what I gathered:

Outcome

Implement this on the frontend, the frontend will continue to fetch data from the 2 endpoints, once the data is loaded we can group it according to the designs (DORA vs key metrics).

The new data for Time to restore and Change failure rate will also be added and will need to be handled on the frontend as well once it is available in the relevant API endpoints.

Rationale

Unblocks frontend and does not require additional backend work in %15.0
Rethinking the structure of the requests will make more sense as an improvement afterwards
We should add a backend issue to investigate how to clean up the API requests and license checks

Did i miss anything @m_frankiewicz @danmh @ahegyi

As a side note, this changes the weight for frontend, i'd suggest 2 now.

Thanks @ekigbo!

mentioned in issue #355304 (closed)

removed the relation with #355304 (closed)

marked this issue as related to #355304 (closed)

marked this issue as related to #357071 (closed)

changed the description

added priority2 label

All SUS-impacting issues need to have a proper severity label set. Please add those to this issue and remove the automation:ux-missing-labels label.

@danmh can you help get a severity label applied? Thanks!

Thank you for the ping @vkarnes.

Looking again at this, perhaps this should be featureaddition and ~"non-SUS". As splitting across multiple rows is something new, rather than building on a previous change or insight. What do you think?

cc: @ekigbo @m_gill

I view this as an enhancement to the current experience and would leave the current set of labels, @danmh.

@vkarnes thank you for checking, I appreciate it.

added automation:ux-missing-labels label

added [deprecated] Accepting merge requests label

This feature issue does not have the documentation label. Please add it if appropriate, because documentation is one of the aspects of our MR acceptance checklist.