NETOBSERV-230 topology view #97

jpinsonneau · 2022-03-17T16:44:48Z

This PR is part of NETOBSERV-180 Topology components spike task

It implements basic view component for topology.
Data are faked from a real query result saved in web/src/components/netflow-topology/__tests__/netflow-topology.spec.tsx and randomly updated.
You can press refresh button to see the changes.

As mentioned in #93 you need to add topology=preview in your url arguments if you are not on localhost:9000

openshift-ci · 2022-03-17T16:44:53Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from jpinsonneau after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jpinsonneau · 2022-03-17T17:40:46Z

linked option panel action #98

jotak · 2022-03-18T10:06:22Z

web/src/components/netflow-topology/componentFactories/shapesComponentFactory.ts

+  type: string
+): ComponentType<{ element: GraphElement }> | undefined => {
+  switch (type) {
+    //TODO: try different shapes by Owner Kind for example


+1 to this comment, to sync with Andrew

web/src/utils/label.ts

jotak · 2022-03-18T10:12:04Z

web/src/utils/label.ts

+
+//TODO: remove kindToAbbr after integration of console ResourceIcon in topology library
+const abbrBlacklist = ['ASS'];
+export const kindToAbbr = (kind: string) => {


isn't it something we can get from the console plugin SDK? I know the console has something similar, we should try to be consistent with them.

The function itself is in console-dynamic-plugin-sdk/src/utils/k8s/k8s-get-resource.ts but is not exported.

It's related to bridge/api-discovery-resources you can retrieve in your localStorage, containing a list of kinds with abbreviations.
If we don't have an alternative we could query these data however it seems there is not the color field on kinds we are looking for.

jpinsonneau · 2022-03-22T16:32:23Z

moved TopologyMetrics interface to api to comply with #101

jpinsonneau · 2022-03-24T10:06:09Z

Rebased with #98 so you can now test display options 👍

jpinsonneau · 2022-03-28T12:18:52Z

refactoring Visualization outside of TopologyContent
useVisualizationController can retrieve controller from anywhere inside VisualizationProvider
fixed observer to render elements according to zoom level
updated @patternfly/react-topology & added new concentric layout
added long label truncate

jotak · 2022-04-05T08:42:29Z

web/src/model/topology.ts

+};
+
+export const DEFAULT_NODE_TRUNCATE_LENGTH = 25;
+export const DEFAULT_NODE_SIZE = 75;


this is nitpicking, but still it would be good to get a common approach on that: which naming case for constants?

I tend to follow the other convention, defaultNodeSize, making no difference between variables and constants, being exported or not, being global or not. I think it's the most common in jasvascript / typescript landscape (though I wouldn't bet on it), it's in the well adopted basarat's TypeScript book or in the TypeScript language convention itself (which does not pretend to be a convention for typescript projects in general)

On the other hand, we have the google approach which makes a difference between global constants and other constants, like you're doing here. The same approach can be seen elsewhere (I don't think ts.dev has anything official with TypeScript, it's just a service company)

Again, this is nitpicking, but if we get a common approach it would be more consistent and we'd clear the current ambiguity that we have while writing code or doing reviews. I'm more used to doing the first option in typescript, but I'm fine to change

It seems like the console code tend to follow capital-case approach, while not 100% consistent

I'm ok with any. We can centralize all the exported constants to a single folder so it will be easier to maintain with a single convention ?

I prefer the domain-driven approach than everything-centralized :) Keep the code closer to where it's used, and try to have some domain boundaries, rather than a single constants file that often grows out of control, can easily turn up being messy and sometimes ends up having unused constants that remain unnoticed.. I've been there :)

I have opened a follow up task since this is not related to that PR
https://issues.redhat.com/browse/NETOBSERV-268

jotak · 2022-04-06T12:36:54Z

web/src/components/netflow-topology/componentFactories/stylesComponentFactory.tsx

+//TODO: implement context menu
+const defaultMenu = createContextMenuItems('TODO');


Do you plan to implement it via this PR, or later? Maybe remove / comment-out this code if it's planned for later?

let's remove it, we'll see about the context menus later

jotak · 2022-04-06T13:21:28Z

Again, nice work!
Here's some feedback, it's mostly suggestions to improve the design.

Text in side panel seems too big on the switch labels
The side panel width can probably be decreased a bit to leave more space to the graph?
Label truncate could probably be better, currently it's split in the middle, however most of the time pod names end with a generated part which is less relevant to show. Maybe truncate as 18|6 instead of the current 12|12 ?
Rate labels are incorrect when using reporter=both, because the data is not meant to be summed as there are duplicates. Ideally we should de-duplicate, but that's not trivial. In the meantime I can see two options:
- We could divide by two as an estimate. It would be "less incorrect" in most cases, but still not perfect.
- Or we could just hide the rate label when reporter is "both".
(enhancement request / new story) Create a full screen mode or larger viewport mode? (Eg. hide the page title/header/toolbar)

As an aside, totally unrelated, maybe we should rename our workloads to "netobserv-..."? It would render better on topology.

I'm fine if you prefer to address feedback in follow-ups. The only real issue I could see is the incorrect rate for reporter "both".

jotak · 2022-04-07T06:52:12Z

@jpinsonneau I'm setting the lgtm label, I think it's fine to merge if you prefer to address feedback in follow-ups, up to you
/lgtm

jpinsonneau · 2022-04-07T07:21:53Z

@jpinsonneau I'm setting the lgtm label, I think it's fine to merge if you prefer to address feedback in follow-ups, up to you /lgtm

sure let's do this => https://issues.redhat.com/browse/NETOBSERV-270

jpinsonneau force-pushed the topology_view_component branch from efe647e to 167e4b7 Compare March 17, 2022 17:05

jpinsonneau requested review from jotak, mariomac and OlivierCazade March 17, 2022 17:06

jpinsonneau force-pushed the topology_view_component branch from 53122b0 to e290077 Compare March 17, 2022 17:20

jpinsonneau force-pushed the topology_view_component branch from e290077 to 672c680 Compare March 17, 2022 17:44

jotak reviewed Mar 18, 2022

View reviewed changes

web/src/utils/label.ts Show resolved Hide resolved

jotak reviewed Mar 18, 2022

View reviewed changes

jpinsonneau mentioned this pull request Mar 21, 2022

Topology Component : Group icons / edges refresh patternfly/patternfly-react#7092

Closed

jpinsonneau force-pushed the topology_view_component branch from 1e187c2 to dfa66cd Compare March 24, 2022 10:05

jpinsonneau requested a review from jotak March 24, 2022 10:06

openshift-ci bot added the needs-rebase label Mar 24, 2022

jpinsonneau force-pushed the topology_view_component branch from dfa66cd to 5552ea9 Compare March 28, 2022 12:14

openshift-ci bot removed the needs-rebase label Mar 28, 2022

openshift-ci bot added the needs-rebase label Apr 4, 2022

jotak reviewed Apr 5, 2022

View reviewed changes

jpinsonneau added 3 commits April 6, 2022 13:07

topology view

84cf40f

merge imports

6144f01

refactor

107617d

jpinsonneau force-pushed the topology_view_component branch from 5552ea9 to 107617d Compare April 6, 2022 11:20

openshift-ci bot removed the needs-rebase label Apr 6, 2022

optimized refresh + ensure model consistency

25ad387

jpinsonneau requested a review from jotak April 6, 2022 11:39

jotak reviewed Apr 6, 2022

View reviewed changes

jpinsonneau added 2 commits April 6, 2022 14:50

prettier

07ff232

removed context menu todo

d254e7d

openshift-ci bot assigned jotak Apr 7, 2022

openshift-ci bot added the lgtm label Apr 7, 2022

jpinsonneau merged commit e1f7f5d into netobserv:main Apr 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-230 topology view #97

NETOBSERV-230 topology view #97

jpinsonneau commented Mar 17, 2022 •

edited

Loading

openshift-ci bot commented Mar 17, 2022

jpinsonneau commented Mar 17, 2022

jotak Mar 18, 2022

jotak Mar 18, 2022

jpinsonneau Mar 18, 2022

jpinsonneau commented Mar 22, 2022

jpinsonneau commented Mar 24, 2022

jpinsonneau commented Mar 28, 2022

jotak Apr 5, 2022

jotak Apr 5, 2022

jpinsonneau Apr 5, 2022

jotak Apr 5, 2022

jpinsonneau Apr 6, 2022

jotak Apr 6, 2022

jpinsonneau Apr 6, 2022

jotak commented Apr 6, 2022 •

edited

Loading

jotak commented Apr 7, 2022

jpinsonneau commented Apr 7, 2022

		//TODO: implement context menu
		const defaultMenu = createContextMenuItems('TODO');

NETOBSERV-230 topology view #97

NETOBSERV-230 topology view #97

Conversation

jpinsonneau commented Mar 17, 2022 • edited Loading

openshift-ci bot commented Mar 17, 2022

jpinsonneau commented Mar 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpinsonneau commented Mar 22, 2022

jpinsonneau commented Mar 24, 2022

jpinsonneau commented Mar 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jotak commented Apr 6, 2022 • edited Loading

jotak commented Apr 7, 2022

jpinsonneau commented Apr 7, 2022

jpinsonneau commented Mar 17, 2022 •

edited

Loading

jotak commented Apr 6, 2022 •

edited

Loading