-
Notifications
You must be signed in to change notification settings - Fork 14
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
NETOBSERV-1025: include back and forth traffic #343
Conversation
Codecov Report
@@ Coverage Diff @@
## main #343 +/- ##
==========================================
- Coverage 58.61% 52.71% -5.90%
==========================================
Files 151 29 -122
Lines 6635 1857 -4778
Branches 804 0 -804
==========================================
- Hits 3889 979 -2910
+ Misses 2524 783 -1741
+ Partials 222 95 -127
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Just figured out that loading UI state from URL params seem quite broken but not sure yet if it's my PR or if also broken on |
Still some issues to fix:
Edit: fixed |
@jotak: This pull request references NETOBSERV-1025 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
{showSwap && ( | ||
<Tooltip content={t('Swap source and destination filters')}> | ||
<Button id="swap-filters-button" data-test="swap-filters-button" variant="link" onClick={swapFilters}> | ||
{t('Swap filters')} | ||
</Button> | ||
</Tooltip> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These items are taking too much width so we should consider changing the way we display them to allow being column flexed
network-observability-console-plugin/web/src/components/netflow-traffic.tsx
Lines 844 to 872 in f37382e
const filtersExtraControl = () => { | |
return ( | |
<Dropdown | |
data-test="more-options-dropdown" | |
id="more-options-dropdown" | |
onSelect={() => setFiltersOverflowMenuOpen(false)} | |
toggle={ | |
<Button | |
data-test="more-options-button" | |
id="more-options-button" | |
variant="link" | |
className="overflow-button" | |
icon={<EllipsisVIcon />} | |
onClick={() => setFiltersOverflowMenuOpen(!isFilterOverflowMenuOpen)} | |
> | |
{t('More options')} | |
</Button> | |
} | |
isOpen={isFilterOverflowMenuOpen} | |
dropdownItems={[ | |
<DropdownGroup key="fullscreen-group" label={t('View')}> | |
<DropdownItem key="fullscreen" onClick={() => setFullScreen(!isFullScreen)}> | |
{isFullScreen ? t('Collapse') : t('Expand')} | |
</DropdownItem> | |
</DropdownGroup> | |
]} | |
/> | |
); | |
}; |
<ToolbarItem className="flex-start" id="back-and-forth-item"> | ||
<Tooltip content={t('Include return traffic, with swapped source and destination filters')}> | ||
<Checkbox | ||
data-test="back-and-forth" | ||
id="back-and-forth" | ||
onChange={setBackAndForth} | ||
isChecked={filters.backAndForth} | ||
label={t('Back and forth')} | ||
aria-label={t('Back and forth')} | ||
/> | ||
{getFilterControl()} | ||
</InputGroup> | ||
<FilterHints def={selectedFilter} /> | ||
</div> | ||
</Tooltip> | ||
</ToolbarItem> | ||
</Tooltip> | ||
</ToolbarItem> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe place the back and forth
check somewhere else ?
- in the query options (but would be less visible)
- at swap filters level (would be more consistent to me since it's related to existing filters)
web/src/model/topology.ts
Outdated
const getFilterDefValue = (nodeType: NodeType, fields: Partial<TopologyMetricPeer>, dir: FilterDir, t: TFunction) => { | ||
let def: FilterDefinition | undefined; | ||
let value: string | undefined; | ||
if (fields.resource && fields.namespace) { | ||
def = findFilter(t, dir === 'src' ? 'src_resource' : dir === 'dst' ? 'dst_resource' : 'resource')!; | ||
def = findFilter(t, dir === 'src' ? 'src_resource' : 'dst_resource')!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def = findFilter(t, dir === 'src' ? 'src_resource' : 'dst_resource')!; | |
def = findFilter(t, `${dir}_resource`)!; |
web/src/model/topology.ts
Outdated
def = findFilter(t, dir === 'src' ? 'src_host_name' : 'dst_host_name')!; | ||
value = `"${fields.hostName || fields.resource?.name}"`; | ||
} else if (nodeType === 'namespace' && (fields.namespace || fields.resource)) { | ||
def = findFilter(t, dir === 'src' ? 'src_namespace' : dir === 'dst' ? 'dst_namespace' : 'namespace')!; | ||
def = findFilter(t, dir === 'src' ? 'src_namespace' : 'dst_namespace')!; | ||
value = `"${fields.namespace || fields.resource?.name}"`; | ||
} else if (nodeType === 'resource' && fields.resource) { | ||
def = findFilter(t, dir === 'src' ? 'src_name' : dir === 'dst' ? 'dst_name' : 'name')!; | ||
def = findFilter(t, dir === 'src' ? 'src_name' : 'dst_name')!; | ||
value = `"${fields.resource.name}"`; | ||
} else if (nodeType === 'owner' && fields.owner) { | ||
def = findFilter(t, dir === 'src' ? 'src_owner_name' : dir === 'dst' ? 'dst_owner_name' : 'owner_name')!; | ||
def = findFilter(t, dir === 'src' ? 'src_owner_name' : 'dst_owner_name')!; | ||
value = `"${fields.owner.name}"`; | ||
} else if (fields.addr) { | ||
def = findFilter(t, dir === 'src' ? 'src_address' : dir === 'dst' ? 'dst_address' : 'address')!; | ||
def = findFilter(t, dir === 'src' ? 'src_address' : 'dst_address')!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment than above
web/src/model/filters.ts
Outdated
export type FilterId = | ||
| 'namespace' | ||
| 'src_namespace' | ||
| 'dst_namespace' | ||
| 'name' | ||
| 'src_name' | ||
| 'dst_name' | ||
| 'kind' | ||
| 'src_kind' | ||
| 'dst_kind' | ||
| 'owner_name' | ||
| 'src_owner_name' | ||
| 'dst_owner_name' | ||
| 'resource' | ||
| 'src_resource' | ||
| 'dst_resource' | ||
| 'address' | ||
| 'src_address' | ||
| 'dst_address' | ||
| 'mac' | ||
| 'src_mac' | ||
| 'dst_mac' | ||
| 'port' | ||
| 'src_port' | ||
| 'dst_port' | ||
| 'host_address' | ||
| 'src_host_address' | ||
| 'dst_host_address' | ||
| 'host_name' | ||
| 'src_host_name' | ||
| 'dst_host_name' | ||
| 'protocol' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of managing each src
/ dst
you can defined targeted filters separately and then build the final type as:
export type TargetedFilterId =
| 'namespace'
| 'name'
| 'kind'
| 'owner_name'
| 'resource'
| 'address'
| 'mac'
| 'port'
| 'host_address'
| 'host_name';
export type FilterId =
| `src_${TargetedFilterId}`
| `dst_${TargetedFilterId}`
| 'protocol'
| 'type'
| 'id';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waw I didn't know we could infer a type like that, that's cool!
srcEncoder: FiltersEncoder, | ||
dstEncoder: FiltersEncoder | ||
): FilterDefinition[] => { | ||
const withDest = (src: FilterDefinition, dstEncoder: FiltersEncoder): FilterDefinition[] => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following TargetedFilterId approach you can keep the previous peers
approach that takes TargetedFilterId + srcEncoder + dstEncoder as arguments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it's mor ecomplicated... e.g. here's what I would have:
const peers = (
base: Omit<Omit<FilterDefinition, 'id'>, 'encoder'> & { id: TargetedFilterId },
srcEncoder: FiltersEncoder,
dstEncoder: FiltersEncoder
): FilterDefinition[] => {
return [
{
...base,
id: ('src_' + base.id) as FilterId,
category: FilterCategory.Source,
encoder: srcEncoder
},
{
...base,
id: ('dst_' + base.id) as FilterId,
category: FilterCategory.Destination,
encoder: dstEncoder
}
];
};
The double Omit
is a bit ugly, I felt like the new approach that takes a fully built FilterDefinition
and mirroring it was nicer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about split and extend the FilterDefinition
interface like:
export interface FilterPeerDefinition {
targetedId: TargetedFilterId;
name: string;
component: FilterComponent;
category: FilterCategory;
getOptions: (value: string) => Promise<FilterOption[]>;
validate: (value: string) => { val?: string; err?: string };
checkCompletion?: (value: string, selected: string) => { completed: boolean; option: FilterOption };
autoCompleteAddsQuotes?: boolean;
hint?: string;
examples?: string;
placeholder?: string;
// overlap tells if the type of entity referred to with this filter may have overlapping (duplicated)
// flows when querying for returned traffic (back and forth) - they result in slightly more complicated queries.
overlap: boolean;
}
export interface FilterDefinition extends FilterPeerDefinition {
id: FilterId;
encoder: FiltersEncoder;
}
Then you don't need the Omit
part anymore. You can also use back quotes for ids to avoid the casts:
const peers = (
base: FilterPeerDefinition,
srcEncoder: FiltersEncoder,
dstEncoder: FiltersEncoder
): FilterDefinition[] => {
return [
{
...base,
id: `src_${base.targetedId}`,
category: FilterCategory.Source,
encoder: srcEncoder
},
{
...base,
id: `dst_${base.targetedId}`,
category: FilterCategory.Destination,
encoder: dstEncoder
},
];
};
I'll let you pick what you prefer 😉 no blocks either way for me
|
||
const determineOverlap = (orig: Filter[], swapped: Filter[]): { overlaps: Filter[]; cancelSwap: boolean } => { | ||
// With "back and forth", the input query is "doubled" with an analoguous query that captures the return traffic | ||
// Overlap detection consists in excluding from that added query the overlapping part, by adding the opposite of the 1st query to the second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice ASCII art 😄
I'm fine increasing the max-len
to 170
if needed
Thanks for this improvment @jotak ! It really brings a better UX. I feel it needs some small changes to makes it good to go:
|
Thanks @jpinsonneau for the feedback! |
@jpinsonneau I took the opportunity to change a little bit the design, looks quite better I think: |
That's cool ! thanks ! Do you want to address the chips refactoring in a followup ? |
Yeah I guess so, let me create a task |
Replace "Common" filters with an option to include return traffic. Add swapping feature reuse swapFilters func Split filters chips / toolbar in two files Fix filters not reloaded from url Allow faster frontend builds New make commands to build faster: - Build locally with `make build-frontend-fast` - Build image with `BUILDSCRIPT=:fast make images` The drawback is potentially not catching errors related to using dependencies (e.g. patternfly ...); that's the reason why I prefer to keep the default build as a "full compiling" build. But these new targets can save developer's time when doing frequent iterative changes. In any case, if a compile error is uncaught by the developer, it should be caught by the CI build, which is unchanged. Create filter menu in topology - Filter by source or dest - Stepping into turns on back'n forth - Split decorators in new files / new react components - Make more use of react callbacks - Avoid redrawing everything on hover Fix issues with back & forth Need to process differently symetric filters vs non-symetric ones, ie. a filter such as "srcns=A, dstns=B", when applied back and forth, becomes "srcns=A, dstns=B OR (dstns=A, srcns=B, srcns!=A, dstns!=B)" (the "!=" bits are to exclude overlaps ie. counting traffic twice). But when the query is symetric (src==dst, such as "srcns=A, dstns=A") we can't apply the same logic; in this particular example, the "swapped" query will be ignored because fully overlapping the initial query. Fix missing useEffect for filters drilldown in tpology review feedback Move "back and forth" to chip links and more ... - Move to chips links - Change design, using link+icon - Use Overflow menu - Create a new component LinksOverflow - Create a new component MaybeTooltip Fix fmt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/approve |
/approve cancel |
.. sorry .. cancelling approval (I was too eager to merge it and forgot QE approval 😇) |
@jotak: This pull request references NETOBSERV-1025 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@jotak: This pull request references NETOBSERV-1025 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
New image: It will expire after two weeks. To deploy this build, run from the operator repo, assuming the operator is running: USER=netobserv VERSION=6174fd8 make set-plugin-image |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Outdated screenshots: for up-to-date screenshot, cf comment #343 (comment)
New stuff on the UI:
New filters per source/dest in topology: