Add insights page #20437
Add insights page #20437
Conversation
There was a problem hiding this comment.
| return ( | ||
| <li key={index} className="text-sm text-gray-600 dark:text-gray-300"> | ||
| {session.creationTime ? displayTime(session.creationTime) : "n/a"} ( | ||
| {session.workspace?.status?.instanceId.slice(0, 7) || "No instance ID"}){isRunning ? " - running" : ""} |
There was a problem hiding this comment.
Maybe we should get rid of this or add our little colored circle icon like we have in the ws list.
components/dashboard/src/insights/download/download-sessions.ts
Outdated
Show resolved
Hide resolved
| type Args = Pick<ListWorkspaceSessionsRequest, "organizationId" | "from" | "to"> & { | ||
| organizationName: string; | ||
| signal?: AbortSignal; | ||
| onProgress?: (percentage: number) => void; |
There was a problem hiding this comment.
Pagination is another issue that I faced here - WorkspaceService.ListWorkspaceSessions does not properly report a total, so we can't know how far in the process we are. For now, we just report "Exporting page ${n}".
| .offset(offset) | ||
| .limit(limit) | ||
| .skip(offset) | ||
| .take(limit) |
There was a problem hiding this comment.
This caused some inconsistencies with the way we design our pagination in other API calls, so I aligned it with those.
| <SelectItem value="day">Last 24 hours</SelectItem>{" "} | ||
| {/* here for debugging, probably not useful */} | ||
| <SelectItem value="week">Last 7 days</SelectItem> | ||
| <SelectItem value="month">Last 30 days</SelectItem> | ||
| <SelectItem value="year">Last 365 days</SelectItem> |
There was a problem hiding this comment.
I did not see a good reason for being able to select an absolute time range (like we use today with the usage dashboard), so I just made it use some relative options.
There was a problem hiding this comment.
This is also because I don't really enjoy using date pickers
| } | ||
|
|
||
| // WorkspaceContext is the git context from which the workspace is created | ||
| message WorkspaceContext { |
There was a problem hiding this comment.
This was added so that we can fill in the values for contextURLSegment_1, contextURLSegment_2 and contextURL_cloneURL in the CSV. I imagine that it can come in handy to get it directly from the API as well.
What I want to draw attention to is that I did not add the ws ctx as a top-level property (like it is for workspaces in the DB and in the protocol), but rather hid it under metadata. I'm happy to push it up, but it made sense at the time I was trying to use it.
There was a problem hiding this comment.
While I think what you proposed makes sense in itself, and feels like a nice "completion" of our API, I don't feel the usecase we have is a compelling reason to add the WorkspaceContext shape (with all it's complexity) to the (central) Workspace shape.
My two main concerns are:
- extent: most of the fields exposed here we won't use going forward - not even in the narrow use-case we have right now
- exposing the concept of
WorkspaceContextright now, with such little benefit, but a significant "cost"
- this shows in the "where to place it" problem
- adding it now would severely reduce our solution space if we really needed it in the future (a bit speculative given the state of Classic, but still)
Could we get a way without adding this shape at all? E.g. treat contextUrl as URL, and just focus on the path elements? 🤔
There was a problem hiding this comment.
@geropl this is definitely fair. The addition of the workspace context is something I tried to really get around - it's true we most likely won't be able to use this much outside of the context of workspace sessions and insights1.
I have tried some simple parsing of context URLs, but because we don't offer the cloneURL on the Workspace responses, there is close to no way to parse URLs and split segments properly without running into the question of "is this a repo name part, a subgroup or already some extra path like /commits?".
Additionally, we could really expose some bare-bones information, potentially just on the WorkspaceSession object.
Footnotes
-
that said, the flip side here is that now folks utilizing our
ListWorkspaceSessionsresponses will be able to have a lot more data about the contexts, and will be able to answer adoption-related questions like "do users open PRs/MRs with Gitpod?" ↩
There was a problem hiding this comment.
that said, the flip side here is that now folks utilizing our ListWorkspaceSessions responses will be able to have a lot more data about the contexts, and will be able to answer adoption-related questions like "do users open PRs/MRs with Gitpod?"
That is a good argument indeed. Let me dwell a bit on it.
I have tried some simple parsing of context URLs, but because we don't offer the cloneURL
Argh, you are right. The naming of fields is really all over the place 😬
There was a problem hiding this comment.
In sync, we decided to keep all of the details, but stick them under the WorkspaceSession shape.
There was a problem hiding this comment.
I've implemented the changes as discussed in 79a97aa.
There was a problem hiding this comment.
Please ping me once it's finalized and the preview is ready (still seeing the "react flower" on the bottom, so you might still be changing things...?). 🙏
There was a problem hiding this comment.
Code and preview env looks great!
One more question, do we plan to provide a link on the dashboard to access the "/insights" page?
Update: Found this on PR description
Possibly, yeah. It's not as crucial now, though, since it's not publicly accessible (without knowing about the endpoint) and also pretty straight forward as far as interaction goes.
components/dashboard/src/insights/download/download-sessions.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Gero Posmyk-Leinemann <[email protected]>
components/public-api/typescript-common/fixtures/toWorkspaceSession_3.golden
Outdated
Show resolved
Hide resolved
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
4 participants
Description
Adds a
/insightspage, offering access to the listing fromWorkspaceService.ListWorkspaceSessionsboth as a table and as a downloadable CSV, similar to the one under/usage.Related Issue(s)
Fixes CLC-1017
How to test
/insights, along with the downloadable CSVDocumentation
Possibly, yeah. It's not as crucial now, though, since it's not publicly accessible (without knowing about the endpoint) and also pretty straight forward as far as interaction goes.