Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(topology): expanded row for resource details #921

Merged
merged 7 commits into from
Mar 28, 2023

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Mar 23, 2023

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed the last commit: git commit --amend --signoff

Related to #891
Depends on https://github.com/cryostatio/cryostat/issues/1428

Description of the change:

Each resource row can have an expanded section that show detail view. For now, only ActiveRecording row is supported. Others will show an empty text.

Motivation for the change:

See #906 (comment)

Screenshots

Screenshot from 2023-03-22 21-38-37

@tthvo tthvo added the feat New feature or request label Mar 23, 2023
@tthvo tthvo requested review from andrewazores and maxcao13 March 23, 2023 01:04
@mergify mergify bot added the safe-to-test label Mar 23, 2023
@tthvo tthvo force-pushed the resource-expanded-details branch from 372f9c0 to a889a16 Compare March 23, 2023 01:07
@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-921-a889a16ea75d211fc5a3a3bd5b055887411c7cf3 sh smoketest.sh

@tthvo tthvo force-pushed the resource-expanded-details branch from a889a16 to 7e7a4d2 Compare March 23, 2023 01:39
@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-921-7e7a4d2c8ee21feb86ea7d7a50c1e13879c65b47 sh smoketest.sh

@andrewazores
Copy link
Member

Looks good so far. Can there also be a badge like this one:

image

that indicates whether there are recordings present? Maybe if this can't be determined (ie. jvmId is null which indicates we haven't been able to connect, as is now the case) show a warning icon. If we can check and there is at least one running recording then show a green OK state icon. Otherwise, either show no icon, or show some neutral grey state icon. WDYT?

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-921-2384c220e8b3b84855b84103b840838fcfda9de2 sh smoketest.sh

@tthvo tthvo force-pushed the resource-expanded-details branch from 875db69 to 5dd57f1 Compare March 23, 2023 22:27
@tthvo
Copy link
Member Author

tthvo commented Mar 23, 2023

Error and No-running-recording states won't show any icon. Only targets with running recordings will show the green icon.

With this setup, we can easily add any decorator to the node to the last 2 quadrants.

Just a notice that: we should order our css after PF css or else some rules might be overriden unexpectedly.

Screenshot from 2023-03-23 18-25-33

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-921-5dd57f1ab186da67f26cd8972270104f16dc45bf sh smoketest.sh

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-921-49cc1f7582ab27056113326c0597e08df9517b1c sh smoketest.sh

@tthvo tthvo force-pushed the resource-expanded-details branch from 49cc1f7 to 7dc6976 Compare March 23, 2023 22:48
@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-921-7dc6976b6b97d65e34cc2180e65b1d76b88e95a9 sh smoketest.sh

@tthvo
Copy link
Member Author

tthvo commented Mar 23, 2023

Looks like the useResources hook in the PR can be generalize to accept a target and reused in many other places :D

@maxcao13
Copy link
Member

image

Should this be reactive to backend emitted websocket notifications? It seems like it does not right now.

@tthvo
Copy link
Member Author

tthvo commented Mar 24, 2023

Oh seems to affect the stopped state. I will look into it.

EDIT: Should work now. Got a bit off guard on the stop-event. @andrewazores just wondering why the stop-event is emitting a recording with RUNNING state?

{
  "meta": {
    "category": "ActiveRecordingStopped",
    "type": {
      "type": "application",
      "subType": "json"
    },
    "serverTime": 1679689380
  },
  "message": {
    "target": "service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi",
    "recording": {
      "downloadUrl": "http://localhost:8181/api/v1/targets/service:jmx:rmi:%2F%2F%2Fjndi%2Frmi:%2F%2Flocalhost:0%2Fjmxrmi/recordings/some_recording",
      "reportUrl": "http://localhost:8181/api/v1/targets/service:jmx:rmi:%2F%2F%2Fjndi%2Frmi:%2F%2Flocalhost:0%2Fjmxrmi/reports/some_recording",
      "metadata": {
        "labels": {}
      },
      "archiveOnStop": false,
      "id": 8,
      "name": "some_recording",
      "state": "RUNNING",
      "startTime": 1679689375197,
      "duration": 30000,
      "continuous": false,
      "toDisk": true,
      "maxSize": 0,
      "maxAge": 0
    }
  }
}

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-921-c5f30c70166da02be1955d00fe984bb84ae4c1a5 sh smoketest.sh

@andrewazores
Copy link
Member

I don't know for sure off-hand but I would assume it's because that notification gets emitted with the recording object as the payload right before the recording actually gets stopped, or at least the payload object on the server is still holding the original state that was pulled from the target without it getting updated with the new state after the stoppage occurred. Seems like it should be a minor and easily fixable backend bug, I suspect in the RecordingTargetHelper

@tthvo
Copy link
Member Author

tthvo commented Mar 27, 2023

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-921-b2f578a55dd03ac5d1fe341c80bad0ce0769e120 sh smoketest.sh

@tthvo
Copy link
Member Author

tthvo commented Mar 27, 2023

Latest changes just append the tooltips to portalRoot and remove checks for stop-rec notification since backend now should send correct rec state :))

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-921-ca96e9c6620762a6c42eb47e41b116ced085133f sh smoketest.sh

@tthvo tthvo force-pushed the resource-expanded-details branch from ca96e9c to 6d9acb0 Compare March 28, 2023 00:46
@github-actions
Copy link

This PR/issue depends on:

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-921-6d9acb07a00bdf9455ab233885ef03431506f647 sh smoketest.sh

@andrewazores andrewazores merged commit ce551f1 into cryostatio:main Mar 28, 2023
@tthvo tthvo deleted the resource-expanded-details branch March 28, 2023 01:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request safe-to-test
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants