Improve Job detail view to make it refreshed in real-time instead of reloading

@ayufan As far as I know, this will require refactoring the job header, the job details block, and the job status list into Vue components, as they all display ci status information that needs updating.

We will probably need to refactor the build log somewhat as well, since the class that manages updating and rendering the log (Build) is contains logic for polling and refreshing the page when the status changes, which realtime components will depend on or need to replicate.

I would say leave the build log as-is, we don't have to do it now and it also doesn't make sense.

I would focus on job header and job details. Job status list do in next go, as this doesn't change that often.

added feature proposal real-time labels

@ayufan can we move this one to %9.3 please?

assigned to @filipa

mentioned in merge request !10987 (closed)

Moving this to %9.3 per this https://gitlab.com/gitlab-org/gitlab-ce/issues/24339#note_28463180

changed milestone to %9.3

mentioned in issue #31718 (moved)

In my personal experience, https://gitlab.com/gitlab-org/gitlab-ce/issues/31701 seems to be a higher priority, and easier. Can we tackle that in 9.3 instead? Unless we can do both, along with cross-project pipelines? Or if we want to continue on with job details page for other momentum reasons.

/cc @bikebilly

As per today's CI meeting I believe @zj will work on the backend of this issue. In order for either parts to be blocked, can we start working on a API schema?

/cc @ayufan do you have any suggestions?

@filipa Looking at the page we'll need the following info to be updated with the Etag mechanism:

Current jobs status
retry_path
job_details
- duration (only if finished)
- finished_at
- runner_id
pipeline_jobs or related builds

We have a BuildEntity already to provide some details:

id
name
build_path
retry_path
play_path
playable
created_at
updated_at
detailed_status
- The same structure as all other statusses

So missing would be the

runner_id
finished_at (null if no datetime in the db yet)
- finished_at - started_at == duration
related builds

In pseudo JSON my proposal, also the TLDR:

{
  "id":1,
  "name":"rspec",
  "runner_id":1,
  "build_path":"/path/to/build/3",
  "retry_path":"/path/to/build/3/retry",
  "play_path":"path/to/build/3/play",
  "playable":true
  "created_at":20170508 // or whatever the type is
  "updated_at":29123281391283,
  "finished_at":324892340234,
  "duration":12312312 // in seconds, let me know if this is not what you need
  "detailed_status": {
   // usual types for statusses
  },
  "related_jobs": [
    {
      // hashes of jobs/builds
    }
  ]
}

Polling interval I would propose 10 seconds

@zj I think we need more info:

Continuing your list:

Header:

status
job id
pipeline id
pipeline path
commit sha
commit path
branch name
author information
finished_at
new issue path
retry_path

Log

Log output
size we are showing
Raw path

Sidebar

MR ID
duration
runner id
finished_At
raw path
erase path
commit tile
tags
stages
related_jobs

Log format should be kept, as in, we should receive HTML

{
  "log" : "<code>log</code>"
}

@filipa I wonder why the frontend needs all the paths all the time, it seems like a simple string concat each time and can be easily be feature spec'ed against? This increases both the response times and the payload size. Good example for is you asking for the issue path? Thats just /namespace/project/issues/new

@ayufan I was talking with @zj about this issue.

I think we should provide all data in the API response. If we have half in haml and half in vue it won't be real-time and it will much harder to maintain the state across haml and vue.

I know it can mean a lot more work for both sides but I think this is worth it.

We currently have some open bugs for this view. If we could make it all in vue we could guarantee a better code.

Do you think we can make that happen?

@zj Frontend should not mess around with the paths:

Some of them are not relative to the window.location object
It can mean some security issues
~~All paths are provided from the backend~~~
FE does not have access to the namespace or the project, meaning we would have to query the url which would only work for instances deployed exactly like gitlab.com

@zj we did not cover some info. According to UX mockups on this issue: https://gitlab.com/gitlab-org/gitlab-ce/issues/24339

There is an entry about queued

@zj @ayufan I went over the existing data we show and the data we need for the mockups and I think it's all in here: https://gitlab.com/snippets/1662207

I am not sure if this structure makes sense, but it has all the data needed. Can you please validate this?

Thank you!

assigned to @zj

assigned to @ayufan

@filipa I will tackle that.

@filipa I think this is a good start, but there is still something we can improve probably. That is a pitty that we can't comment on a snippets yet.

Some ideas and questions

"path": "/root/review-app/pipelines/131" <- is that a pipeline path? If so the key should be probably pipeline_path here
does it make sense to include coverage in details?
maybe we can rephrase environment_for_build and environment_link_for_build? I wonder if environment and environment_path would be enough?
does it make sense to move everything below flags to details section? Maybe except paths that we can also group under paths key
do we need can_update|read_build?

What do you think about it?

Also:

move runner out of details
The whole flags hash makes me cry inside, just to note it here.
Trigger has no entity yet, more keys will be added
I'll expose finished_at too. Probably some other keys
Related_jobs will include only whats needed and on the model
Artifacts keys are inconsistent it seems, why expire_at and expired_at?
coverage is just a prop on a build

do we need can_update|read_build?

@grzesiek It appears so, as frontend is not context aware and seems to be unable to load details initially

We can provide can_update|read_build through haml @zj @grzesiek like we do in other places!

@filipa In that case I'd like to pass a lot of other stuff there too. Think about static paths.

@zj sounds good to me!

assigned to @filipa

@filipa It is better to keep it in the payload if we really need it. I just wonder if this information is maybe already there (like there is no cancel_path = cannot cancel).

@grzesiek @filipa I seriously think we are going about this the wrong way, and I wonder why we can't have multiple Etag requests. 1 for related jobs, and one for the current job.

Also, all static stuff needs to be passed in the HAML. Its a waste and really shitty to pass it in the BuildEntity. Now it makes our code less modular as in all honesty I now have to dig where we're using this entity and where I need to set which flags or not. The passing of data which is not directly related to the current job should be avoided IMO.

Today I wasted about a day to come up with an elegant solution which would make the entities as useable as they are in the public facing API. The BuildEntity is quite the mess already and we're not using the Serializers design pattern/structure that long.

@zj

Also, all static stuff needs to be passed in the HAML. Its a waste and really shitty to pass it in the BuildEntity. Now it makes our code less modular as in all honesty I now have to dig where we're using this entity and where I need to set which flags or not. The passing of data which is not directly related to the current job should be avoided IMO.

I disagree. HAML should be dropped in favor of using everything with API. Having to maintain two sources of truth makes it harder to maintain it later.

Today I wasted about a day to come up with an elegant solution which would make the entities as useable as they are in the public facing API. The BuildEntity is quite the mess already and we're not using the Serializers design pattern/structure that long.

Is there a reason why we try to put "everything" in BuildEntity?

@filipa

Let's do not touch API for Log output. It makes this much harder.

@filipa

Please, no permission checks. We should get rid of can_update|can_read.

It seems that we need:

Request with Job details,
Trace request that in the future should be replaced with streaming of raw text and client side rendering (so we are not touching it now),
Data for the sidebar: Stages and jobs for the specific stage.

The first request is covered by BuildEntity with small addition. Since BuildEntity is used in other requests we should have BuildEntityDetails that would expose detailed information used for that view.

The second is not touched at all.

The third request should be added or re-used. We need to render part of pipeline data, similar to what we do with Pipeline Graph. I would try to return only relevant data, not all data.

move runner out of details

This should be part of BuildEntityDetails,

The whole flags hash makes me cry inside, just to note it here.

Hmm?

Related_jobs will include only whats needed and on the model

This is not really part of that request. It should be in separate one.

Artifacts keys are inconsistent it seems, why expire_at and expired_at?

AFAIK we need only expired_at.

@filipa Also, you did not cover the environments component.

@zj We should have separate MR for each part of the backend story. I would start with having just job details first and merge that to master before @filipa's work.

To summarize:

We will not add relevant_jobs endpoint, @filipa will use pipelines/id.json,
We will not touch trace.json at all,
We will add only builds/id.json endpoint,
This endpoint will render BuildDetailsEntity,
BuildDetailsEntity will inherit from BuildEntity,
Change PipelineEntity to be PipelineDetailsEntity,
Introduce PipelineEntity that contains minimal information about Pipeline, everything except details: artifacts, stages, manual_actions,
BuildDetailsEntity will expose additional data:

tags: ["runner", "tag"],
duration: 90.0,
erased_at: nil|or date,
coverage: nil|90.0,
pipeline: using PipelineEntity,
artifacts: null || {
  expired: false|true,
  expire_at: date to expire,
  keep_path: path,
  download_path: path,
  browse_path: path
},
merge_request: {
  id: id,
  path: path
},
runner: {
  id: id,
  path: path
},
raw_path: path,
new_issue_path: new_namespace_project_issue_path(@project.namespace, @project, issue: build_failed_issue_options) if build.failed?,

The MR entity is quite unusable for this, so Ill send the MR path instead. Parsing the IID shouldn't be too hard.

JSON right now:

WDYT @filipa

mentioned in commit c17b4b17

mentioned in merge request !11726 (merged)

mentioned in merge request !11651 (merged)

mentioned in issue #32970 (closed)

mentioned in commit 47a0276e

mentioned in merge request !11848 (merged)

mentioned in issue #17910 (closed)

closed via merge request !11651 (merged)

mentioned in commit 9ccb289a

reopened

unassigned @zj

reopened

closed via merge request !11848 (merged)

mentioned in commit 194c40df

mentioned in issue gitlab#9594

mentioned in merge request !14785

Improve Job detail view to make it refreshed in real-time instead of reloading

Resources

Description

Designs

Child items ...

Activity

Header:

Log

Sidebar

Admin message

Admin message

Improve Job detail view to make it refreshed in real-time instead of reloading

Resources

Description

Activity

Header:

Log

Sidebar