Fetch ddg action and reducer #398
Conversation
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
…load' into fetch-ddg-action-and-reducer
Signed-off-by: Everett Ross <[email protected]>
…load' into fetch-ddg-action-and-reducer
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
…load' into fetch-ddg-action-and-reducer
Signed-off-by: Everett Ross <[email protected]>
…load' into fetch-ddg-action-and-reducer
Signed-off-by: Everett Ross <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #398 +/- ##
==========================================
+ Coverage 89.26% 89.77% +0.51%
==========================================
Files 164 170 +6
Lines 3734 3842 +108
Branches 846 869 +23
==========================================
+ Hits 3333 3449 +116
+ Misses 365 357 -8
Partials 36 36
Continue to review full report at Codecov.
|
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
| query: TDdgRequiredUrlState; | ||
| }; | ||
|
|
||
| export type TDdgAddStylePayload = TDdgRequiredUrlState & { visibilityIndices: number[]; style: number }; |
There was a problem hiding this comment.
Let's change the nomenclature around these styling related actions and state to refer to view modifiers. Also, why not use the EViewModifier enum?
E.g.
export type TDdgAddViewModifierPayload = TDdgRequiredUrlState & { visibilityIndices: number[]; viewModifier: EViewModifier };Also, I think this should take in the key instead of the visibilityIndices?
Same comment for the following payload type.
There was a problem hiding this comment.
Or maybe it should take in key or visibility indices? For find, it makes sense to use visibility indices. For mouse-over, where it's coming from the graph node, key is more natural.
export type TDdgAddViewModifierPayload = TDdgRequiredUrlState &
({ visibilityIndices: number[] } | { key: string }) & {
viewModifier: EViewModifier
};There was a problem hiding this comment.
The reason to prefer visIndices is that if we want to modify the view of edges (such as all edges on all paths that go through a specific vertex), it will be easiest to do so through knowing with pathElems have had view modifications. Even if both the to and the from of an edge are styled, that doesn't mean the edge should be styled.
There was a problem hiding this comment.
I agree that edge {from: a, to: b} should not necessarily be highlighted even if both a and b are highlighted.
OTOH, if I mouseover a node, the node should trigger an action to have a view modifier added to it. But, the node only knows the vertexKey:
type TProps = {
vertexKey: string;
service: string;
operation: string | null;
focalNodeUrl: string | null;
isFocalNode: boolean;
viewModifiers: number;
setViewModifier: (vertexKey: string, viewModifier: EViewModifier, isEnabled: boolean) => void;
};So, either the reducer or some parent component needs to translate from vertexKey to visIndices. I would argue it's fine for this to happen in the reducer.
This brings up another point, though, we currently do not have a way to identify edges. visIndices suffers from your same point: edge {from: a, to: b} should not necessarily be highlighted even if both a and b are highlighted.
| mock.expects('fetchTrace').withExactArgs(id); | ||
| jaegerApiActions.fetchTrace(id); | ||
| expect(() => mock.verify()).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
I think these can be driven by an array of params?
const tests = [
{
msg: '@JAEGER_API/SEARCH_TRACES should fetch the trace by id',
fn: 'searchTraces',
arg: query, // or null
},
// etc ...
];
tests.forEach(test => {
const { msg, fn, args } = test;
it(msg, () => {
mock.expects(fn).withExactArgs(arg);
jaegerApiActions[fn](arg);
expect(() => mock.verify()).not.toThrow();
});
});Seems like the same with the promise tests.
There was a problem hiding this comment.
that's true for most, but not @JAEGER_API/FETCH_MULTIPLE_TRACES which calls an action with one name and an API fn with another. I know that's not hard to handle (optional extra key on tests), but perhaps best to leave jaeger-api.test and api/jaeger.test clean up for its own targeted PR?
There was a problem hiding this comment.
The reason I mentioned it is because you re-wrote the tests in this PR.
| }; | ||
| } | ||
|
|
||
| export function clearStyleState(state: TDdgState, { payload }: { payload: TDdgClearStylePayload }) { |
There was a problem hiding this comment.
I think this action is too overloaded:
- remove one view modifier for a path element
- clear all view modifiers for a path element
- clear all view modifiers for all path elements
The downside of this is you pretty much need to read the implementation of the reducer to know the action has these capabilities.
Currently, we don't need the clear all for a path element or clear all for all path elements. And, one I have about the "added powers" of this action / reducer is one component is it becomes more likely for one component to clobber the view modifiers of another component.
I suggest pairing this down to the first scenario and adding other actions and reducers if we need to expand the capabilities.
There was a problem hiding this comment.
As an aside, it might be handy to use a util class for dealing with view modifers:
class ViewModifiers extends Map<number, number> {
clone() {
return new ViewModifiers(this);
}
rmModifier(modifier: EViewModifier | null, index: number) {
if (modifier == null) {
this.delete(index);
return this;
}
const value = (this.get(index) || 0) & ~modifier;
if (value === EViewModifier.None) {
this.delete(index);
} else {
this.set(index, value);
}
return this;
}
addModifier(modifier: EViewModifier, index: number) {
const value = (this.get(index) || 0) | modifier;
this.set(index, value);
return this;
}
}There was a problem hiding this comment.
Things like emphasizing all paths through a vertex will mean that many pathElems need to be added/removed at once. It may be easier to clear emphasized from all elems rather than calculate which need to be unemphasized
Clearing all view modifications for one or more pathElems could be part of a vertice's componentWillUnmount clean up.
| return state; | ||
| } | ||
| const styleStates = new Map(stateEntry.styleStates); | ||
| (visibilityIndices || Array.from(styleStates.keys())).forEach(idx => { |
There was a problem hiding this comment.
Looks like this should use forEach on visibilityIndices not styleStates.keys()?
There was a problem hiding this comment.
My first take on this was to think this is a bug. Something like the following would be more clear:
const styleStates = visibilityIndices ? new Map(stateEntry.styleStates) : new Map();There was a problem hiding this comment.
Or, are there actually four scenarios, with the fourth being
- Remove a particular view modifier from all path elements
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
Signed-off-by: Everett Ross <[email protected]>
| viewModifier: number; | ||
| }; | ||
|
|
||
| export type TDdgClearViewModifiersFromIndicesPayload = TDdgAddViewModifierPayload & { viewModifier?: void }; |
There was a problem hiding this comment.
I don't see much benefit in basing these types off of TDdgAddViewModifierPayload.
There was a problem hiding this comment.
The reason I defined the types as:
export type TDdgClearViewModifiersFromIndicesPayload = TDdgAddViewModifierPayload & { viewModifier?: void };
not export type TDdgClearViewModifiersFromIndicesPayload = TDdgModelParams & { visibilityIndices: number [] };
is because this line in the reducer const { visibilityIndices, viewModifier } = payload; would have the this type error: Property 'viewModifier' does not exist on type 'TDdgViewModifierRemovalPayload'.
I agree that the second type is a clearer, but TypeScript doesn't seem to like the implicit number | undefined when destructuring from the union type.
Playground
|
|
||
| export type TDdgAddViewModifierPayload = TDdgModelParams & { | ||
| visibilityIndices: number[]; | ||
| viewModifier: number; |
There was a problem hiding this comment.
Why not use EViewModifier, instead of number, as the type, here?
There was a problem hiding this comment.
Added a comment for this:
+ // Number instead of EViewModifier so that multiple views can be changed at once.
Ddg graph component
Add DDG Index with Loading, Error, and temporary header and done views
Signed-off-by: Everett Ross <[email protected]>
…-and-reducer Fetch ddg action and reducer Signed-off-by: vvvprabhakar <[email protected]>
Which problem is this PR solving?
Short description of the changes
actions/jaeger-apito fetch ddg data (endpoint subject to change, of course)actions/deep-dependency-graphfor adding and clearing stylesreducer/deep-dependency-graphfor processing ddg request, and updating stylesUpdated destination, previous PR