From 5042b05fae5ba2826f227e9950bd95bf03912900 Mon Sep 17 00:00:00 2001 From: asalem Date: Tue, 2 Jun 2020 05:54:59 -0700 Subject: [PATCH 01/11] fix(findSubgraph): updated findSubgraph to return subgraphs that only have 1 reference to a node no matter how deeply nested Still need to update the filterUnusedVars function to load variables that might be called within variables Also need to determine whether a child node in a graph can have multiple parents --- ui/src/shared/utils/filterUnusedVars.ts | 2 + ui/src/variables/actions/thunks.ts | 3 +- ui/src/variables/utils/hydrateVars.test.ts | 222 +++++++++++++++++++-- ui/src/variables/utils/hydrateVars.ts | 122 ++++++++--- 4 files changed, 312 insertions(+), 37 deletions(-) diff --git a/ui/src/shared/utils/filterUnusedVars.ts b/ui/src/shared/utils/filterUnusedVars.ts index 9172e950a67..bbc00d4f6a9 100644 --- a/ui/src/shared/utils/filterUnusedVars.ts +++ b/ui/src/shared/utils/filterUnusedVars.ts @@ -20,6 +20,8 @@ export const filterUnusedVars = (variables: Variable[], views: View[]) => { (acc, vp) => [...acc, ...vp.queries.map(query => query.text)], [] as Array ) + // TODO: make sure to parse out variables for other used variables + // console.log('queryTexts: ', queryTexts) const varsInUse = variables.filter(variable => queryTexts.some(text => isInQuery(text, variable)) diff --git a/ui/src/variables/actions/thunks.ts b/ui/src/variables/actions/thunks.ts index 2b038f79c40..8db9fbde01e 100644 --- a/ui/src/variables/actions/thunks.ts +++ b/ui/src/variables/actions/thunks.ts @@ -433,7 +433,6 @@ export const selectValue = (variableID: string, selected: string) => async ( await dispatch(selectValueInState(contextID, variableID, selected)) // only hydrate the changedVariable - dispatch(hydrateVariables(true)) - // dispatch(hydrateChangedVariable(variableID)) + dispatch(hydrateChangedVariable(variableID)) dispatch(updateQueryVars({[variable.name]: selected})) } diff --git a/ui/src/variables/utils/hydrateVars.test.ts b/ui/src/variables/utils/hydrateVars.test.ts index 952232ec6a5..289409eca7c 100644 --- a/ui/src/variables/utils/hydrateVars.test.ts +++ b/ui/src/variables/utils/hydrateVars.test.ts @@ -325,19 +325,31 @@ describe('hydrate vars', () => { }) }) -xdescribe('findSubgraph', () => { - test('should return the update variable with all associated parents', async () => { - const variableGraph = await createVariableGraph(defaultVariables) - const actual = await findSubgraph(variableGraph, [defaultVariable]) - // returns the single subgraph result - expect(actual.length).toEqual(1) - const [subgraph] = actual - // expect the subgraph to return the passed in variable - expect(subgraph.variable).toEqual(defaultVariable) - // expect the parent to be returned with the returning variable - expect(subgraph.parents[0].variable).toEqual(associatedVariable) - }) +describe('findSubgraph', () => { + const getParentNodes = (node, acc: Set = new Set()): string[] => { + for (const parent of node.parents) { + if (!acc.has(parent)) { + acc.add(parent.variable.id) + getParentNodes(parent, acc) + } + } + return [...acc] + } + test('should return the variable with no parents when no association exists', async () => { + /* + This example deals with the following situation where a node with no relationship is passed in: + By passing in the variable `a`, we expect the child to be returned with a reference to the parent. + However, since no children or parents exist, the following should be: + + [ + { + variable: a, + parent: [] + children: [], + }, + ] + */ const a = createVariable('a', 'f(x: v.b)') const variableGraph = await createVariableGraph([...defaultVariables, a]) const actual = await findSubgraph(variableGraph, [a]) @@ -349,6 +361,24 @@ xdescribe('findSubgraph', () => { expect(subgraph.parents).toEqual([]) }) test('should return the update default (timeRange) variable with associated parents', async () => { + /* + This example deals with the following situation where a parent with a child has been passed in: + + associatedVariable + | + | + timeRangeStart + + By passing in the timeRangeStart, we expect the child to be returned with a reference to the parent: + + [ + { + variable: timeRangeStart, + parent: [associatedVariable] + }, + ] + The reason for this is because we want the youngest child node (end of the LL tail) to load first, followed by its parents. + */ const variableGraph = await createVariableGraph(defaultVariables) const actual = await findSubgraph(variableGraph, [timeRangeStartVariable]) expect(actual.length).toEqual(1) @@ -358,4 +388,172 @@ xdescribe('findSubgraph', () => { // expect the parent to be returned with the returning variable expect(subgraph.parents[0].variable).toEqual(associatedVariable) }) + test('should filter out inputs that have already been loaded based on a previous associated variable', async () => { + /* + This example deals with the following situation where a parent with a child has been passed in, and a unrelated variable is passed in. + Practically speaking, this looks like: + + associatedVariable a + | + | + defaultVariable + + By passing in the defaultVariable, we expect the child to be returned with a reference to the parent. + Since the variable `a` does not have any parent/child relationship, we expect it to simply be returned: + + [ + { + variable: defaultVariable, + parent: [associatedVariable] + }, + { + variable: a, + children: [], + parent: [], + } + ] + The reason for this is because we want the youngest child node (end of the LL tail) to load first, followed by its parents. + Since defaultVariable is the youngest child node, and since `a` doesn't have a child node, + we expect those values to be returned with any references to their parents + */ + const a = createVariable('a', 'f()') + const variableGraph = await createVariableGraph([...defaultVariables, a]) + const actual = await findSubgraph(variableGraph, [ + defaultVariable, + associatedVariable, + a, + ]) + // returns the two subgraph results + expect(actual.length).toEqual(2) + const resultIDs = actual.map(v => v.variable.id) + // expect the two variables with no children to be output + expect(resultIDs).toEqual([defaultVariable.id, a.id]) + expect(actual[0].children).toEqual([]) + expect(actual[1].children).toEqual([]) + // expect the subgraph to return the passed in variable + const parents = getParentNodes(actual[0]) + expect(parents.length).toEqual(1) + const [parent] = parents + // expect the defaultVariable to have the associatedVariable as a parent + expect(parent).toEqual(associatedVariable.id) + }) + test('should return the child node (defaultVariable) with associated parents of the input (associatedVariable) when the child node is passed in', async () => { + /* + This example deals with the following situation where a parent with a child has been passed in. + Practically speaking, this looks like: + + associatedVariable + | + | + defaultVariable + + By passing in the defaultVariable, we expect the child to be returned with a reference to the parent: + + [ + { + variable: defaultVariable, + parent: [associatedVariable] + }, + ] + The reason for this is because we want the youngest child node (end of the LL tail) to load first, followed by its parents. + */ + const variableGraph = await createVariableGraph(defaultVariables) + const actual = await findSubgraph(variableGraph, [defaultVariable]) + // returns the subgraph result + expect(actual.length).toEqual(1) + const resultIDs = actual.map(v => v.variable.id) + // expect the one variables with no children to be output + expect(resultIDs).toEqual([defaultVariable.id]) + expect(actual[0].children).toEqual([]) + // expect the subgraph to return the passed in variable + const parents = getParentNodes(actual[0]) + expect(parents.length).toEqual(1) + const [parent] = parents + // expect the defaultVariable to have the associatedVariable as a parent + expect(parent).toEqual(associatedVariable.id) + }) + test('should return the child node (defaultVariable) with associated parents of the input (associatedVariable) when the parent node is passed in', async () => { + /* + This example deals with the following situation where a parent with a child has been passed in. + Practically speaking, this looks like: + + associatedVariable + | + | + defaultVariable + + By passing in the associatedVariable, we expect the child to be returned with a reference to the parent: + + [ + { + variable: defaultVariable, + parent: [associatedVariable] + }, + ] + The reason for this is because we want the youngest child node (end of the LL tail) to load first, followed by its parents. + */ + const variableGraph = await createVariableGraph(defaultVariables) + const actual = await findSubgraph(variableGraph, [associatedVariable]) + // returns the subgraph result + expect(actual.length).toEqual(1) + const resultIDs = actual.map(v => v.variable.id) + // expect the one variables with no children to be output + expect(resultIDs).toEqual([defaultVariable.id]) + expect(actual[0].children).toEqual([]) + // expect the subgraph to return the passed in variable + const parents = getParentNodes(actual[0]) + expect(parents.length).toEqual(1) + const [parent] = parents + // expect the defaultVariable to have the associatedVariable as a parent + expect(parent).toEqual(associatedVariable.id) + }) + test('should only return the child nodes (defaultVariable, timeRangeStart) with the like parents filtered out of the second variable (timeRangeStart) when the multiple common parents are passed in', async () => { + /* + This example deals with the following situation where a parent has two children. + In this case, both the defaultVariable and timeRangeStart are children to associatedVariable. + Practically speaking, this looks like: + + associatedVariable + / \ + defaultVariable timeRangeStart + + By passing in the associatedVariable and the timeRangeStart, the output should be: + + [ + { + variable: defaultVariable, + parent: [associatedVariable] + }, + { + variable: timeRangeStart, + parent: [], + }, + ] + The reason for this is because we deduplicate parent nodes that have already been loaded. + Since the defaultVariable has already hydrated the associatedVariable, we can + rely upon that in order to resolve timeRangeStart + */ + const variableGraph = await createVariableGraph(defaultVariables) + const actual = await findSubgraph(variableGraph, [ + timeRangeStartVariable, + associatedVariable, + ]) + // returns the subgraph result + expect(actual.length).toEqual(2) + const resultIDs = actual.map(v => v.variable.id) + // expect the one variables with no children to be output + expect(resultIDs).toEqual([defaultVariable.id, timeRangeStartVariable.id]) + expect(actual[0].children).toEqual([]) + expect(actual[1].children).toEqual([]) + // expect the subgraph to return the passed in variable + const parents = getParentNodes(actual[0]) + expect(parents.length).toEqual(1) + const [parent] = parents + // expect the defaultVariable to have the associatedVariable as a parent + expect(parent).toEqual(associatedVariable.id) + // since the expected parent is the associatedVariable, we expect that + // the subgraph to filter out the parent before it is added onto the subgraph + const timeRangeParents = getParentNodes(actual[1]) + expect(timeRangeParents).toEqual([]) + }) }) diff --git a/ui/src/variables/utils/hydrateVars.ts b/ui/src/variables/utils/hydrateVars.ts index d11d1864b0a..86dce65cc28 100644 --- a/ui/src/variables/utils/hydrateVars.ts +++ b/ui/src/variables/utils/hydrateVars.ts @@ -111,6 +111,71 @@ const collectAncestors = ( return [...acc] } +/* + Collect youngest child of a node (or the tail of the LinkedList within our graph). + + A node `a` is a child of `b` if there exists a path from `b` to `a`. + + This function determines the root child in order to easily hydrate parent variables and + resolve and prevent rehydrating variables that have already been passed in. + + In the example below, we have 3 variables `a`, `b`, `c` where a is a parent to b, which is a parent to c: + + a --> b --> c + + If `a`, `b`, or `c` are passed into this function, we should expect that the output for each should be: + + `c` with a reference to its parent `b`, which should have a reference to its parent `a` +*/ +const getRootChildNode = ( + node: VariableNode, + acc: Set = new Set() +): VariableNode => { + if (node.children.length === 0) { + return node + } + for (const child of node.children) { + if (child.children.length > 0) { + getRootChildNode(child, acc) + } else { + return child + } + } +} + +/* + Filters out any parents that have already been referenced in a previous node + + A node `a` & `c` are children of `b` if there exists a path: + + b + / \ + a c + + This function safely filters out a duplicate reference to the parent `b` + When both `a` and `c` are passed as variables to be hydrated, since `b` should + already be `hydrated` by the first variable that is passed in. +*/ +export interface DeduplicatedRoot { + node: VariableNode + subsetIDs: {[key: string]: boolean} +} + +const getDeduplicatedRootChild = ( + node: VariableNode, + subsetIDs: {[key: string]: boolean | undefined} +): DeduplicatedRoot => { + for (const n of node.parents) { + if (!subsetIDs[n.variable.id]) { + subsetIDs[n.variable.id] = true + getDeduplicatedRootChild(n, subsetIDs) + } else { + node.parents = [] + } + } + return {node, subsetIDs} +} + /* Given a variable graph, return the minimal subgraph containing only the nodes needed to hydrate the values for variables in the passed `variables` argument. @@ -128,33 +193,44 @@ export const findSubgraph = ( const subgraph: Set = new Set() // use an ID array to reduce the chance of reference errors const varIDs = variables.map(v => v.id) - // TODO: uncomment this when variable hydration is resolved - // create an array of IDs to reference later - // const graphIDs = [] + // create an ID reference object to identify relevant root variables to hydrate + let subgraphIDs = {} for (const node of graph) { - const shouldKeep = - varIDs.includes(node.variable.id) || - collectAncestors(node).some(ancestor => - varIDs.includes(ancestor.variable.id) - ) - - if (shouldKeep) { - subgraph.add(node) - // graphIDs.push(node.variable.id) + const shouldKeep = varIDs.includes(node.variable.id) + + // get youngest child of the node in order to find the lowest common denominator amongst the nodes + const rootChild = getRootChildNode(node) + // by checking whether the subgraphIDs[rootChild.variable.id] !== true, we are ensuring that the root + // node has not yet been added to the subgraph. Therefore, if a rootChild has already been added to the + // subgraph, we prevent that rootChild from being added & hydrated again + if (shouldKeep && subgraphIDs[rootChild.variable.id] !== true) { + if (rootChild.parents.length > 0) { + /* + Once a node exists within the subgraph (whether nested as a parent or as the rootNode) + we want to remove any further to that node within the subgraph. + This can be particularly challenging when a parent node has multiple child nodes that have been passed in. + For example, if variables `a` and `b` are both children to `c`, + and `a` & `b` are both variables that should be hydrated, we would need to reset a parent + reference for one of the variables so that `c` is not hydrated twice. + This can be achieved by storing a reference to all the existing nodeIDs within the subgraph to the `graphIDs` + and checking for any collisions before adding the node to the subgraph. + If a parent collision is detected, that node's parent is simply set to [] since we know that + the parent was already hydrated based on a previous input + */ + const {node: filteredNodes, subsetIDs} = getDeduplicatedRootChild( + rootChild, + subgraphIDs + ) + subgraph.add(filteredNodes) + subgraphIDs[rootChild.variable.id] = true + subgraphIDs = {...subgraphIDs, ...subsetIDs} + } else { + subgraph.add(rootChild) + subgraphIDs[rootChild.variable.id] = true + } } } - // const removeDupAncestors = (n: VariableNode) => { - // const {id} = n.variable - // return !graphIDs.includes(id) - // } - - for (const node of subgraph) { - // node.parents = node.parents.filter(removeDupAncestors) - // node.children = node.children.filter(removeDupAncestors) - node.parents = node.parents.filter(node => subgraph.has(node)) - node.children = node.children.filter(node => subgraph.has(node)) - } return [...subgraph] } From 26be172cb7bec09ee0d8d06c582e3fec5d9cd142 Mon Sep 17 00:00:00 2001 From: asalem Date: Tue, 2 Jun 2020 09:04:12 -0700 Subject: [PATCH 02/11] fix(hydrateVars.test): fixed the breaking tests. Need to figure out whether some of the tests are still necessary. Need to find out whether a child can have multiple parents Need to find out whether a parent with multiple children should hydrate all the children Need to work on the filterUnusedVars --- ui/src/variables/utils/hydrateVars.test.ts | 59 ++++++++++++---------- ui/src/variables/utils/hydrateVars.ts | 31 ++++++++---- 2 files changed, 52 insertions(+), 38 deletions(-) diff --git a/ui/src/variables/utils/hydrateVars.test.ts b/ui/src/variables/utils/hydrateVars.test.ts index 289409eca7c..080e4baf779 100644 --- a/ui/src/variables/utils/hydrateVars.test.ts +++ b/ui/src/variables/utils/hydrateVars.test.ts @@ -119,18 +119,19 @@ describe('hydrate vars', () => { // f [fontcolor = "green"] // g [fontcolor = "green"] // } - expect( - actual.filter(v => v.id === 'a')[0].arguments.values.results - ).toBeFalsy() + // TODO(ariel): figured out if these tests are necessary + // expect( + // actual.filter(v => v.id === 'a')[0].arguments.values.results + // ).toBeFalsy() expect( actual.filter(v => v.id === 'b')[0].arguments.values.results ).toBeFalsy() - expect( - actual.filter(v => v.id === 'c')[0].arguments.values.results - ).toBeFalsy() - expect( - actual.filter(v => v.id === 'd')[0].arguments.values.results - ).toBeFalsy() + // expect( + // actual.filter(v => v.id === 'c')[0].arguments.values.results + // ).toBeFalsy() + // expect( + // actual.filter(v => v.id === 'd')[0].arguments.values.results + // ).toBeFalsy() expect( actual.filter(v => v.id === 'e')[0].arguments.values.results @@ -142,10 +143,10 @@ describe('hydrate vars', () => { ).toEqual(['gVal']) expect(actual.filter(v => v.id === 'g')[0].selected).toEqual(['gVal']) - expect( - actual.filter(v => v.id === 'f')[0].arguments.values.results - ).toEqual(['fVal']) - expect(actual.filter(v => v.id === 'f')[0].selected).toEqual(['fVal']) + // expect( + // actual.filter(v => v.id === 'f')[0].arguments.values.results + // ).toEqual(['fVal']) + // expect(actual.filter(v => v.id === 'f')[0].selected).toEqual(['fVal']) }) test('should invalidate all ancestors of a node when it fails', async () => { @@ -192,12 +193,13 @@ describe('hydrate vars', () => { // c [fontcolor = "green"] // } // - expect( - actual.filter(v => v.id === 'a')[0].arguments.values.results - ).toEqual([]) - expect( - actual.filter(v => v.id === 'b')[0].arguments.values.results - ).toEqual([]) + // TODO(ariel): determine if these are still relevant + // expect( + // actual.filter(v => v.id === 'a')[0].arguments.values.results + // ).toEqual([]) + // expect( + // actual.filter(v => v.id === 'b')[0].arguments.values.results + // ).toEqual([]) expect( actual.filter(v => v.id === 'c')[0].arguments.values.results @@ -244,10 +246,11 @@ describe('hydrate vars', () => { // Basic test for now, we would need an icky mock to assert that the // appropriate substitution is actually taking place - expect( - actual.filter(v => v.id === 'a')[0].arguments.values.results - ).toEqual(['aVal']) - expect(actual.filter(v => v.id === 'a')[0].selected).toEqual(['aVal']) + // TODO(ariel): determine if these are still necessary checks + // expect( + // actual.filter(v => v.id === 'a')[0].arguments.values.results + // ).toEqual(['aVal']) + // expect(actual.filter(v => v.id === 'a')[0].selected).toEqual(['aVal']) expect(actual.filter(v => v.id === 'b')[0].arguments.values).toEqual({ k: 'v', }) @@ -289,11 +292,11 @@ describe('hydrate vars', () => { selections: {}, fetcher, }).promise - - expect( - actual.filter(v => v.id === 'a')[0].arguments.values.results - ).toEqual(['aVal']) - expect(actual.filter(v => v.id === 'a')[0].selected).toEqual(['aVal']) + // TODO(ariel): determine if these are still necessary checks + // expect( + // actual.filter(v => v.id === 'a')[0].arguments.values.results + // ).toEqual(['aVal']) + // expect(actual.filter(v => v.id === 'a')[0].selected).toEqual(['aVal']) expect(actual.filter(v => v.id === 'b')[0].arguments.values).toEqual([ 'v1', diff --git a/ui/src/variables/utils/hydrateVars.ts b/ui/src/variables/utils/hydrateVars.ts index 86dce65cc28..0a502fddeab 100644 --- a/ui/src/variables/utils/hydrateVars.ts +++ b/ui/src/variables/utils/hydrateVars.ts @@ -129,17 +129,20 @@ const collectAncestors = ( */ const getRootChildNode = ( node: VariableNode, - acc: Set = new Set() + acc: Set = new Set(), + cache: {[key: string]: boolean | undefined} = {} ): VariableNode => { if (node.children.length === 0) { return node } for (const child of node.children) { - if (child.children.length > 0) { - getRootChildNode(child, acc) - } else { - return child + // by checking the cache for existing variables, we ensure that the graph stops + // when an existing node has already been encountered, invalidating a cycle if one exists + if (child.children.length > 0 && !cache[child.variable.id]) { + cache[child.variable.id] = true + return getRootChildNode(child, acc, cache) } + return child } } @@ -223,7 +226,11 @@ export const findSubgraph = ( ) subgraph.add(filteredNodes) subgraphIDs[rootChild.variable.id] = true - subgraphIDs = {...subgraphIDs, ...subsetIDs} + subgraphIDs = { + [rootChild.variable.id]: true, + ...subgraphIDs, + ...subsetIDs, + } } else { subgraph.add(rootChild) subgraphIDs[rootChild.variable.id] = true @@ -366,7 +373,7 @@ const findLeaves = (graph: VariableNode[]): VariableNode[] => */ const findCyclicPath = (node: VariableNode): VariableNode[] => { try { - findCyclicPathHelper(node, []) + findCyclicPathHelper(node, [], {}) } catch (cyclicPath) { return cyclicPath } @@ -376,14 +383,18 @@ const findCyclicPath = (node: VariableNode): VariableNode[] => { const findCyclicPathHelper = ( node: VariableNode, - seen: VariableNode[] + seen: VariableNode[], + cache: {[key: string]: undefined | boolean} = {} ): void => { - if (seen.includes(node)) { + if (cache[node.variable.id]) { throw seen } for (const child of node.children) { - findCyclicPathHelper(child, [...seen, node]) + findCyclicPathHelper(child, [...seen, node], { + ...cache, + [node.variable.id]: true, + }) } } From 4bebd6a478db73b6f3eb04574fdc290bdba555a6 Mon Sep 17 00:00:00 2001 From: asalem Date: Tue, 2 Jun 2020 09:15:31 -0700 Subject: [PATCH 03/11] feat(hydratevars): added feature flag to revert any potential issue with implementation --- flags.yml | 6 +++++ kit/feature/list.go | 16 ++++++++++++ ui/src/variables/actions/thunks.ts | 7 +++++- ui/src/variables/utils/hydrateVars.ts | 35 +++++++++++++++++++++++++-- 4 files changed, 61 insertions(+), 3 deletions(-) diff --git a/flags.yml b/flags.yml index 33ee0e9cad3..62dabe2a36d 100644 --- a/flags.yml +++ b/flags.yml @@ -103,3 +103,9 @@ contact: Alirie Gray lifetime: temporary +- name: New Hydrate Vars Functionality + description: Enables a minimalistic variable hydration + key: hydratevars + default: false + contact: Ariel Salem / Monitoring Team + lifetime: temporary diff --git a/kit/feature/list.go b/kit/feature/list.go index d5b367e299b..5cd1b4455ff 100644 --- a/kit/feature/list.go +++ b/kit/feature/list.go @@ -184,6 +184,20 @@ func NewLabelPackage() BoolFlag { return newLabels } +var hydratevars = MakeBoolFlag( + "New Hydrate Vars Functionality", + "hydratevars", + "Ariel Salem / Monitoring Team", + false, + Temporary, + false, +) + +// NewHydrateVarsFunctionality - Enables a minimalistic variable hydration +func NewHydrateVarsFunctionality() BoolFlag { + return hydratevars +} + var all = []Flag{ appMetrics, backendExample, @@ -198,6 +212,7 @@ var all = []Flag{ pushDownGroupAggregateFirst, pushDownGroupAggregateLast, newLabels, + hydratevars, } var byKey = map[string]Flag{ @@ -214,4 +229,5 @@ var byKey = map[string]Flag{ "pushDownGroupAggregateFirst": pushDownGroupAggregateFirst, "pushDownGroupAggregateLast": pushDownGroupAggregateLast, "newLabels": newLabels, + "hydratevars": hydratevars, } diff --git a/ui/src/variables/actions/thunks.ts b/ui/src/variables/actions/thunks.ts index 8db9fbde01e..790f6c82815 100644 --- a/ui/src/variables/actions/thunks.ts +++ b/ui/src/variables/actions/thunks.ts @@ -33,6 +33,7 @@ import {findDependentVariables} from 'src/variables/utils/exportVariables' import {getOrg} from 'src/organizations/selectors' import {getLabels, getStatus} from 'src/resources/selectors' import {currentContext} from 'src/shared/selectors/currentContext' +import {isFlagEnabled} from 'src/shared/utils/featureFlag' // Constants import * as copy from 'src/shared/copy/notifications' @@ -433,6 +434,10 @@ export const selectValue = (variableID: string, selected: string) => async ( await dispatch(selectValueInState(contextID, variableID, selected)) // only hydrate the changedVariable - dispatch(hydrateChangedVariable(variableID)) + if (isFlagEnabled('hydratevars')) { + dispatch(hydrateChangedVariable(variableID)) + } else { + dispatch(hydrateVariables(true)) + } dispatch(updateQueryVars({[variable.name]: selected})) } diff --git a/ui/src/variables/utils/hydrateVars.ts b/ui/src/variables/utils/hydrateVars.ts index 0a502fddeab..687d902612b 100644 --- a/ui/src/variables/utils/hydrateVars.ts +++ b/ui/src/variables/utils/hydrateVars.ts @@ -2,6 +2,7 @@ import {valueFetcher, ValueFetcher} from 'src/variables/utils/ValueFetcher' import Deferred from 'src/utils/Deferred' import {asAssignment} from 'src/variables/selectors' +import {isFlagEnabled} from 'src/shared/utils/featureFlag' // Constants import {OPTION_NAME, BOUNDARY_GROUP} from 'src/variables/constants/index' @@ -189,7 +190,7 @@ const getDeduplicatedRootChild = ( - The node for one of the passed variables depends on this node */ -export const findSubgraph = ( +export const findSubgraphFeature = ( graph: VariableNode[], variables: Variable[] ): VariableNode[] => { @@ -240,6 +241,31 @@ export const findSubgraph = ( return [...subgraph] } +export const findSubgraph = ( + graph: VariableNode[], + variables: Variable[] +): VariableNode[] => { + const subgraph: Set = new Set() + // use an ID array to reduce the chance of reference errors + const varIDs = variables.map(v => v.id) + for (const node of graph) { + const shouldKeep = + varIDs.includes(node.variable.id) || + collectAncestors(node).some(ancestor => + varIDs.includes(ancestor.variable.id) + ) + + if (shouldKeep) { + subgraph.add(node) + } + } + + for (const node of subgraph) { + node.parents = node.parents.filter(node => subgraph.has(node)) + node.children = node.children.filter(node => subgraph.has(node)) + } + return [...subgraph] +} /* Get the `VariableValues` for a variable that cannot be successfully hydrated. @@ -486,7 +512,12 @@ export const hydrateVars = ( allVariables: Variable[], options: HydrateVarsOptions ): EventedCancelBox => { - const graph = findSubgraph( + let findSubgraphFunction = findSubgraph + if (isFlagEnabled('hydratevars')) { + findSubgraphFunction = findSubgraphFeature + } + + const graph = findSubgraphFunction( createVariableGraph(allVariables), variables ).filter(n => n.variable.arguments.type !== 'system') From 686d19693f1b5e485d885350a17c45ec3ddbda98 Mon Sep 17 00:00:00 2001 From: asalem Date: Tue, 2 Jun 2020 11:14:00 -0700 Subject: [PATCH 04/11] feat(wip): this is a WIP, need to account for parent's children and am unsure where to go. Need to take a break from variables --- flags.yml | 2 +- kit/feature/list.go | 2 +- ui/src/variables/utils/hydrateVars.test.ts | 108 ++++++++++----------- ui/src/variables/utils/hydrateVars.ts | 2 +- 4 files changed, 57 insertions(+), 57 deletions(-) diff --git a/flags.yml b/flags.yml index 62dabe2a36d..25735073a09 100644 --- a/flags.yml +++ b/flags.yml @@ -106,6 +106,6 @@ - name: New Hydrate Vars Functionality description: Enables a minimalistic variable hydration key: hydratevars - default: false + default: true contact: Ariel Salem / Monitoring Team lifetime: temporary diff --git a/kit/feature/list.go b/kit/feature/list.go index 5cd1b4455ff..021b085f83a 100644 --- a/kit/feature/list.go +++ b/kit/feature/list.go @@ -188,7 +188,7 @@ var hydratevars = MakeBoolFlag( "New Hydrate Vars Functionality", "hydratevars", "Ariel Salem / Monitoring Team", - false, + true, Temporary, false, ) diff --git a/ui/src/variables/utils/hydrateVars.test.ts b/ui/src/variables/utils/hydrateVars.test.ts index 080e4baf779..420ffc3eba0 100644 --- a/ui/src/variables/utils/hydrateVars.test.ts +++ b/ui/src/variables/utils/hydrateVars.test.ts @@ -3,7 +3,7 @@ import {ValueFetcher} from 'src/variables/utils/ValueFetcher' import { hydrateVars, createVariableGraph, - findSubgraph, + findSubgraphFeature, } from 'src/variables/utils/hydrateVars' // Mocks @@ -119,19 +119,10 @@ describe('hydrate vars', () => { // f [fontcolor = "green"] // g [fontcolor = "green"] // } - // TODO(ariel): figured out if these tests are necessary - // expect( - // actual.filter(v => v.id === 'a')[0].arguments.values.results - // ).toBeFalsy() + expect(actual.length).toEqual(3) expect( actual.filter(v => v.id === 'b')[0].arguments.values.results ).toBeFalsy() - // expect( - // actual.filter(v => v.id === 'c')[0].arguments.values.results - // ).toBeFalsy() - // expect( - // actual.filter(v => v.id === 'd')[0].arguments.values.results - // ).toBeFalsy() expect( actual.filter(v => v.id === 'e')[0].arguments.values.results @@ -142,11 +133,6 @@ describe('hydrate vars', () => { actual.filter(v => v.id === 'g')[0].arguments.values.results ).toEqual(['gVal']) expect(actual.filter(v => v.id === 'g')[0].selected).toEqual(['gVal']) - - // expect( - // actual.filter(v => v.id === 'f')[0].arguments.values.results - // ).toEqual(['fVal']) - // expect(actual.filter(v => v.id === 'f')[0].selected).toEqual(['fVal']) }) test('should invalidate all ancestors of a node when it fails', async () => { @@ -192,19 +178,8 @@ describe('hydrate vars', () => { // b [fontcolor = "red"] // c [fontcolor = "green"] // } - // - // TODO(ariel): determine if these are still relevant - // expect( - // actual.filter(v => v.id === 'a')[0].arguments.values.results - // ).toEqual([]) - // expect( - // actual.filter(v => v.id === 'b')[0].arguments.values.results - // ).toEqual([]) - - expect( - actual.filter(v => v.id === 'c')[0].arguments.values.results - ).toEqual(['cVal']) - expect(actual.filter(v => v.id === 'c')[0].selected).toEqual(['cVal']) + expect(actual.length).toEqual(1) + expect(actual).toEqual([c]) }) test('works with map template variables', async () => { @@ -246,14 +221,8 @@ describe('hydrate vars', () => { // Basic test for now, we would need an icky mock to assert that the // appropriate substitution is actually taking place - // TODO(ariel): determine if these are still necessary checks - // expect( - // actual.filter(v => v.id === 'a')[0].arguments.values.results - // ).toEqual(['aVal']) - // expect(actual.filter(v => v.id === 'a')[0].selected).toEqual(['aVal']) - expect(actual.filter(v => v.id === 'b')[0].arguments.values).toEqual({ - k: 'v', - }) + expect(actual.length).toEqual(1) + expect(actual).toEqual([b]) }) // This ensures that the update of a dependant variable updates the @@ -292,16 +261,9 @@ describe('hydrate vars', () => { selections: {}, fetcher, }).promise - // TODO(ariel): determine if these are still necessary checks - // expect( - // actual.filter(v => v.id === 'a')[0].arguments.values.results - // ).toEqual(['aVal']) - // expect(actual.filter(v => v.id === 'a')[0].selected).toEqual(['aVal']) - - expect(actual.filter(v => v.id === 'b')[0].arguments.values).toEqual([ - 'v1', - 'v2', - ]) + + expect(actual.length).toEqual(1) + expect(actual).toEqual([b]) }) test('should be cancellable', done => { @@ -326,9 +288,43 @@ describe('hydrate vars', () => { cancel() }) + + test('should return the child node with associated parents', async () => { + /* + This example deals with the following situation where a parent with a child has been passed in. + Practically speaking, this looks like: + + associatedVariable + | + | + defaultVariable + + By passing in the defaultVariable, we expect the child to be returned with a reference to the parent: + + [ + { + variable: defaultVariable, + parent: [associatedVariable] + }, + ] + The reason for this is because we want the youngest child node (end of the LL tail) to load first, followed by its parents. + */ + + const fetcher = new FakeFetcher() + + const actual = await hydrateVars([associatedVariable], defaultVariables, { + url: '', + orgID: '', + selections: {}, + fetcher, + }).promise + + expect(actual.length).toEqual(1) + expect(actual).toEqual([defaultVariable]) + }) }) -describe('findSubgraph', () => { +describe('findSubgraphFeature', () => { const getParentNodes = (node, acc: Set = new Set()): string[] => { for (const parent of node.parents) { if (!acc.has(parent)) { @@ -355,7 +351,7 @@ describe('findSubgraph', () => { */ const a = createVariable('a', 'f(x: v.b)') const variableGraph = await createVariableGraph([...defaultVariables, a]) - const actual = await findSubgraph(variableGraph, [a]) + const actual = await findSubgraphFeature(variableGraph, [a]) expect(actual.length).toEqual(1) const [subgraph] = actual // expect the subgraph to return the passed in variable @@ -383,7 +379,9 @@ describe('findSubgraph', () => { The reason for this is because we want the youngest child node (end of the LL tail) to load first, followed by its parents. */ const variableGraph = await createVariableGraph(defaultVariables) - const actual = await findSubgraph(variableGraph, [timeRangeStartVariable]) + const actual = await findSubgraphFeature(variableGraph, [ + timeRangeStartVariable, + ]) expect(actual.length).toEqual(1) const [subgraph] = actual // expect the subgraph to return the passed in variable @@ -421,7 +419,7 @@ describe('findSubgraph', () => { */ const a = createVariable('a', 'f()') const variableGraph = await createVariableGraph([...defaultVariables, a]) - const actual = await findSubgraph(variableGraph, [ + const actual = await findSubgraphFeature(variableGraph, [ defaultVariable, associatedVariable, a, @@ -461,7 +459,7 @@ describe('findSubgraph', () => { The reason for this is because we want the youngest child node (end of the LL tail) to load first, followed by its parents. */ const variableGraph = await createVariableGraph(defaultVariables) - const actual = await findSubgraph(variableGraph, [defaultVariable]) + const actual = await findSubgraphFeature(variableGraph, [defaultVariable]) // returns the subgraph result expect(actual.length).toEqual(1) const resultIDs = actual.map(v => v.variable.id) @@ -496,7 +494,9 @@ describe('findSubgraph', () => { The reason for this is because we want the youngest child node (end of the LL tail) to load first, followed by its parents. */ const variableGraph = await createVariableGraph(defaultVariables) - const actual = await findSubgraph(variableGraph, [associatedVariable]) + const actual = await findSubgraphFeature(variableGraph, [ + associatedVariable, + ]) // returns the subgraph result expect(actual.length).toEqual(1) const resultIDs = actual.map(v => v.variable.id) @@ -537,7 +537,7 @@ describe('findSubgraph', () => { rely upon that in order to resolve timeRangeStart */ const variableGraph = await createVariableGraph(defaultVariables) - const actual = await findSubgraph(variableGraph, [ + const actual = await findSubgraphFeature(variableGraph, [ timeRangeStartVariable, associatedVariable, ]) diff --git a/ui/src/variables/utils/hydrateVars.ts b/ui/src/variables/utils/hydrateVars.ts index 687d902612b..04e7a3e13ad 100644 --- a/ui/src/variables/utils/hydrateVars.ts +++ b/ui/src/variables/utils/hydrateVars.ts @@ -513,6 +513,7 @@ export const hydrateVars = ( options: HydrateVarsOptions ): EventedCancelBox => { let findSubgraphFunction = findSubgraph + // if (true) { if (isFlagEnabled('hydratevars')) { findSubgraphFunction = findSubgraphFeature } @@ -529,7 +530,6 @@ export const hydrateVars = ( if (isCancelled) { return } - try { // TODO: terminate the concept of node.values at the fetcher and just use variables node.values = await hydrateVarsHelper(node, options, on) From 53499bd282227539dd4c6a96f2e10585791d1a6d Mon Sep 17 00:00:00 2001 From: asalem Date: Tue, 2 Jun 2020 16:02:39 -0700 Subject: [PATCH 05/11] feat(findSubgraph): completed findSubgraph logic to account for unique constraints around parent/child relationships Added/updated tests to ensure feature stability Need to resolve filterUnusedVariables issues --- ui/src/variables/mocks/index.ts | 56 ++++---- ui/src/variables/utils/hydrateVars.test.ts | 155 +++++++++++++-------- ui/src/variables/utils/hydrateVars.ts | 102 +++++++------- 3 files changed, 180 insertions(+), 133 deletions(-) diff --git a/ui/src/variables/mocks/index.ts b/ui/src/variables/mocks/index.ts index 234366dec64..d4ffdc51a02 100644 --- a/ui/src/variables/mocks/index.ts +++ b/ui/src/variables/mocks/index.ts @@ -639,7 +639,7 @@ export const defaultVariable: Variable = { id: '05b73f4bffe8e000', orgID: '05b73f49a1d1b000', name: 'static', - description: '', + description: 'defaultVariable', selected: ['defbuck'], arguments: {type: 'constant', values: ['beans', 'defbuck']}, createdAt: '2020-05-19T05:54:20.927477-07:00', @@ -657,7 +657,7 @@ export const associatedVariable: Variable = { id: '05b740974228e000', orgID: '05b740945a91b000', name: 'dependent', - description: '', + description: 'associatedVariable', selected: [], arguments: { type: 'query', @@ -699,32 +699,36 @@ export const timeRangeStartVariable: Variable = { selected: [], } +export const timeRangeStopVariable: Variable = { + orgID: '', + id: 'timeRangeStop', + name: 'timeRangeStop', + arguments: { + type: 'system', + values: ['now()'], + }, + status: RemoteDataState.Done, + labels: [], + selected: [], +} + +export const windowPeriodVariable: Variable = { + orgID: '', + id: 'windowPeriod', + name: 'windowPeriod', + arguments: { + type: 'system', + values: [10000], + }, + status: RemoteDataState.Done, + labels: [], + selected: [], +} + export const defaultVariables: Variable[] = [ defaultVariable, associatedVariable, timeRangeStartVariable, - { - orgID: '', - id: 'timeRangeStop', - name: 'timeRangeStop', - arguments: { - type: 'system', - values: ['now()'], - }, - status: RemoteDataState.Done, - labels: [], - selected: [], - }, - { - orgID: '', - id: 'windowPeriod', - name: 'windowPeriod', - arguments: { - type: 'system', - values: [10000], - }, - status: RemoteDataState.Done, - labels: [], - selected: [], - }, + timeRangeStopVariable, + windowPeriodVariable, ] diff --git a/ui/src/variables/utils/hydrateVars.test.ts b/ui/src/variables/utils/hydrateVars.test.ts index 420ffc3eba0..7c84c3c2367 100644 --- a/ui/src/variables/utils/hydrateVars.test.ts +++ b/ui/src/variables/utils/hydrateVars.test.ts @@ -119,16 +119,15 @@ describe('hydrate vars', () => { // f [fontcolor = "green"] // g [fontcolor = "green"] // } - expect(actual.length).toEqual(3) - expect( - actual.filter(v => v.id === 'b')[0].arguments.values.results - ).toBeFalsy() - + /* + We expect the final outcome to be the two associated variables as the parents, + Since those are the bottom-most children of the graph cycle + */ + expect(actual.length).toEqual(2) expect( actual.filter(v => v.id === 'e')[0].arguments.values.results ).toEqual(['eVal']) expect(actual.filter(v => v.id === 'e')[0].selected).toEqual(['eVal']) - expect( actual.filter(v => v.id === 'g')[0].arguments.values.results ).toEqual(['gVal']) @@ -179,7 +178,9 @@ describe('hydrate vars', () => { // c [fontcolor = "green"] // } expect(actual.length).toEqual(1) - expect(actual).toEqual([c]) + const [cResult] = actual + expect(cResult.arguments.values.results).toEqual(['cVal']) + expect(cResult.selected).toEqual(['cVal']) }) test('works with map template variables', async () => { @@ -222,7 +223,10 @@ describe('hydrate vars', () => { // Basic test for now, we would need an icky mock to assert that the // appropriate substitution is actually taking place expect(actual.length).toEqual(1) - expect(actual).toEqual([b]) + const [bResult] = actual + expect(bResult.arguments.values).toEqual({ + k: 'v', + }) }) // This ensures that the update of a dependant variable updates the @@ -263,7 +267,8 @@ describe('hydrate vars', () => { }).promise expect(actual.length).toEqual(1) - expect(actual).toEqual([b]) + const [bResult] = actual + expect(bResult.arguments.values).toEqual(['v1', 'v2']) }) test('should be cancellable', done => { @@ -325,11 +330,11 @@ describe('hydrate vars', () => { }) describe('findSubgraphFeature', () => { - const getParentNodes = (node, acc: Set = new Set()): string[] => { + const getParentNodeIDs = (node, acc: Set = new Set()): string[] => { for (const parent of node.parents) { if (!acc.has(parent)) { acc.add(parent.variable.id) - getParentNodes(parent, acc) + getParentNodeIDs(parent, acc) } } return [...acc] @@ -407,6 +412,14 @@ describe('findSubgraphFeature', () => { variable: defaultVariable, parent: [associatedVariable] }, + + variable: timeRangeStart, + parent: [] + }, + { + variable: timeRangeStop, + parent: [] + }, { variable: a, children: [], @@ -424,19 +437,25 @@ describe('findSubgraphFeature', () => { associatedVariable, a, ]) - // returns the two subgraph results - expect(actual.length).toEqual(2) - const resultIDs = actual.map(v => v.variable.id) - // expect the two variables with no children to be output - expect(resultIDs).toEqual([defaultVariable.id, a.id]) - expect(actual[0].children).toEqual([]) - expect(actual[1].children).toEqual([]) + // returns the four subgraph results + expect(actual.length).toEqual(4) + const [defaultVarInResult] = actual.filter( + vn => vn.variable.id === defaultVariable.id + ) + expect(defaultVarInResult.variable).toEqual(defaultVariable) // expect the subgraph to return the passed in variable - const parents = getParentNodes(actual[0]) - expect(parents.length).toEqual(1) - const [parent] = parents + const parentIDs = getParentNodeIDs(defaultVarInResult) + expect(parentIDs.length).toEqual(1) + const [parentID] = parentIDs // expect the defaultVariable to have the associatedVariable as a parent - expect(parent).toEqual(associatedVariable.id) + expect(parentID).toEqual(associatedVariable.id) + const [timeRangeResult] = actual.filter( + vn => vn.variable.id === timeRangeStartVariable.id + ) + expect(timeRangeResult).toEqual(timeRangeResult) + // expect the subgraph to return the passed in variable + const rents = getParentNodeIDs(timeRangeResult) + expect(rents).toEqual([]) }) test('should return the child node (defaultVariable) with associated parents of the input (associatedVariable) when the child node is passed in', async () => { /* @@ -467,29 +486,38 @@ describe('findSubgraphFeature', () => { expect(resultIDs).toEqual([defaultVariable.id]) expect(actual[0].children).toEqual([]) // expect the subgraph to return the passed in variable - const parents = getParentNodes(actual[0]) - expect(parents.length).toEqual(1) - const [parent] = parents + const parentIDs = getParentNodeIDs(actual[0]) + expect(parentIDs.length).toEqual(1) + const [parentID] = parentIDs // expect the defaultVariable to have the associatedVariable as a parent - expect(parent).toEqual(associatedVariable.id) + expect(parentID).toEqual(associatedVariable.id) }) - test('should return the child node (defaultVariable) with associated parents of the input (associatedVariable) when the parent node is passed in', async () => { + test('should return all the child nodes if the input variable has children', async () => { /* - This example deals with the following situation where a parent with a child has been passed in. + This example deals with the following situation where a parent with multiple children has been passed in. Practically speaking, this looks like: - associatedVariable - | - | - defaultVariable + associatedVariable + / | \ + / | \ + timeRangeStart defaultVariable timeRangeStop - By passing in the associatedVariable, we expect the child to be returned with a reference to the parent: + By passing in the associatedVariable, we expect the first to be returned with a reference to the parent, + while the subsequent children should not include a reference to the parent: [ { variable: defaultVariable, parent: [associatedVariable] }, + { + variable: timeRangeStart, + parent: [] + }, + { + variable: timeRangeStop, + parent: [] + }, ] The reason for this is because we want the youngest child node (end of the LL tail) to load first, followed by its parents. */ @@ -498,27 +526,36 @@ describe('findSubgraphFeature', () => { associatedVariable, ]) // returns the subgraph result - expect(actual.length).toEqual(1) - const resultIDs = actual.map(v => v.variable.id) + expect(actual.length).toEqual(3) // expect the one variables with no children to be output - expect(resultIDs).toEqual([defaultVariable.id]) - expect(actual[0].children).toEqual([]) + const [defaultVarInResult] = actual.filter( + vn => vn.variable.id === defaultVariable.id + ) + expect(defaultVarInResult.variable).toEqual(defaultVariable) // expect the subgraph to return the passed in variable - const parents = getParentNodes(actual[0]) + const parents = getParentNodeIDs(defaultVarInResult) expect(parents.length).toEqual(1) const [parent] = parents // expect the defaultVariable to have the associatedVariable as a parent expect(parent).toEqual(associatedVariable.id) + const [timeRangeResult] = actual.filter( + vn => vn.variable.id === timeRangeStartVariable.id + ) + expect(timeRangeResult).toEqual(timeRangeResult) + // expect the subgraph to return the passed in variable + const rents = getParentNodeIDs(timeRangeResult) + expect(rents).toEqual([]) }) - test('should only return the child nodes (defaultVariable, timeRangeStart) with the like parents filtered out of the second variable (timeRangeStart) when the multiple common parents are passed in', async () => { + test('should only return the child nodes (defaultVariable, timeRangeStart, timeRangeStop) with the like parents filtered out of the second variable (timeRangeStart) when the multiple common parents are passed in', async () => { /* This example deals with the following situation where a parent has two children. In this case, both the defaultVariable and timeRangeStart are children to associatedVariable. Practically speaking, this looks like: - associatedVariable - / \ - defaultVariable timeRangeStart + associatedVariable + / | \ + / | \ + timeRangeStart defaultVariable timeRangeStop By passing in the associatedVariable and the timeRangeStart, the output should be: @@ -531,6 +568,10 @@ describe('findSubgraphFeature', () => { variable: timeRangeStart, parent: [], }, + { + variable: timeRangeStop, + parent: [], + }, ] The reason for this is because we deduplicate parent nodes that have already been loaded. Since the defaultVariable has already hydrated the associatedVariable, we can @@ -542,21 +583,23 @@ describe('findSubgraphFeature', () => { associatedVariable, ]) // returns the subgraph result - expect(actual.length).toEqual(2) - const resultIDs = actual.map(v => v.variable.id) - // expect the one variables with no children to be output - expect(resultIDs).toEqual([defaultVariable.id, timeRangeStartVariable.id]) - expect(actual[0].children).toEqual([]) - expect(actual[1].children).toEqual([]) + expect(actual.length).toEqual(3) + const [defaultVarInResult] = actual.filter( + vn => vn.variable.id === defaultVariable.id + ) + expect(defaultVarInResult.variable).toEqual(defaultVariable) // expect the subgraph to return the passed in variable - const parents = getParentNodes(actual[0]) - expect(parents.length).toEqual(1) - const [parent] = parents + const parentIDs = getParentNodeIDs(defaultVarInResult) + expect(parentIDs.length).toEqual(1) + const [parentID] = parentIDs // expect the defaultVariable to have the associatedVariable as a parent - expect(parent).toEqual(associatedVariable.id) - // since the expected parent is the associatedVariable, we expect that - // the subgraph to filter out the parent before it is added onto the subgraph - const timeRangeParents = getParentNodes(actual[1]) - expect(timeRangeParents).toEqual([]) + expect(parentID).toEqual(associatedVariable.id) + const [timeRangeResult] = actual.filter( + vn => vn.variable.id === timeRangeStartVariable.id + ) + expect(timeRangeResult).toEqual(timeRangeResult) + // expect the subgraph to return the passed in variable + const rents = getParentNodeIDs(timeRangeResult) + expect(rents).toEqual([]) }) }) diff --git a/ui/src/variables/utils/hydrateVars.ts b/ui/src/variables/utils/hydrateVars.ts index 04e7a3e13ad..0c6d1fc8e51 100644 --- a/ui/src/variables/utils/hydrateVars.ts +++ b/ui/src/variables/utils/hydrateVars.ts @@ -113,14 +113,10 @@ const collectAncestors = ( } /* - Collect youngest child of a node (or the tail of the LinkedList within our graph). + Using the current node as the root, create a PostOrder DFS for the given node + Where we check each cascading parent's children in order to hydrate the children before the parents - A node `a` is a child of `b` if there exists a path from `b` to `a`. - - This function determines the root child in order to easily hydrate parent variables and - resolve and prevent rehydrating variables that have already been passed in. - - In the example below, we have 3 variables `a`, `b`, `c` where a is a parent to b, which is a parent to c: + In the example below, we have 3 variables `a`, `b`, `c` where `a` is a parent to `b`, which a parent to `c`: a --> b --> c @@ -128,23 +124,31 @@ const collectAncestors = ( `c` with a reference to its parent `b`, which should have a reference to its parent `a` */ -const getRootChildNode = ( + +const postOrderDFS = ( node: VariableNode, acc: Set = new Set(), cache: {[key: string]: boolean | undefined} = {} -): VariableNode => { - if (node.children.length === 0) { - return node - } +): VariableNode[] => { + // Handle the edge case that the function is + // called without providing the root node + if (!node) return [...acc] + for (const child of node.children) { // by checking the cache for existing variables, we ensure that the graph stops // when an existing node has already been encountered, invalidating a cycle if one exists - if (child.children.length > 0 && !cache[child.variable.id]) { + if (!cache[child.variable.id]) { cache[child.variable.id] = true - return getRootChildNode(child, acc, cache) + // Recurse down the n-ary tree until we reach a leaf (node with no children) + // where this loop will not execute + postOrderDFS(child, acc, cache) } - return child } + + // Add the current node only after visiting all of its children, left to right + acc.add(node) + + return [...acc] } /* @@ -201,41 +205,37 @@ export const findSubgraphFeature = ( let subgraphIDs = {} for (const node of graph) { const shouldKeep = varIDs.includes(node.variable.id) - - // get youngest child of the node in order to find the lowest common denominator amongst the nodes - const rootChild = getRootChildNode(node) - // by checking whether the subgraphIDs[rootChild.variable.id] !== true, we are ensuring that the root - // node has not yet been added to the subgraph. Therefore, if a rootChild has already been added to the - // subgraph, we prevent that rootChild from being added & hydrated again - if (shouldKeep && subgraphIDs[rootChild.variable.id] !== true) { - if (rootChild.parents.length > 0) { - /* - Once a node exists within the subgraph (whether nested as a parent or as the rootNode) - we want to remove any further to that node within the subgraph. - This can be particularly challenging when a parent node has multiple child nodes that have been passed in. - For example, if variables `a` and `b` are both children to `c`, - and `a` & `b` are both variables that should be hydrated, we would need to reset a parent - reference for one of the variables so that `c` is not hydrated twice. - This can be achieved by storing a reference to all the existing nodeIDs within the subgraph to the `graphIDs` - and checking for any collisions before adding the node to the subgraph. - If a parent collision is detected, that node's parent is simply set to [] since we know that - the parent was already hydrated based on a previous input - */ - const {node: filteredNodes, subsetIDs} = getDeduplicatedRootChild( - rootChild, - subgraphIDs - ) - subgraph.add(filteredNodes) - subgraphIDs[rootChild.variable.id] = true - subgraphIDs = { - [rootChild.variable.id]: true, - ...subgraphIDs, - ...subsetIDs, + if (shouldKeep) { + const postOrderNodeList = postOrderDFS(node) + postOrderNodeList.forEach(variableNode => { + const {id} = variableNode.variable + // Checking whether the subgraphIDs[id] !== true, we are ensuring that the + // node has been added to the subgraph. This prevents excessive variable rehydration + if (subgraphIDs[id] !== true) { + /* + Once a node exists within the subgraph (whether nested as a parent or as the input node) + we want to remove any further reference to that node within the subgraph. + This can be particularly challenging when a parent node has multiple child nodes that have been passed in. + For example, if variables `a` and `b` are both children to `c`, + and `a` & `b` are both variables that should be hydrated, we would need to reset a parent + reference for one of the variables so that `c` is not hydrated twice. + This can be achieved by storing a reference to all the existing nodeIDs within the subgraph to the `graphIDs` + and checking for any collisions before adding the node to the subgraph. + If a parent collision is detected, that node's parent is simply set to [] since we know that + the parent was already hydrated based on a previous input + */ + const {node: filteredNodes, subsetIDs} = getDeduplicatedRootChild( + variableNode, + subgraphIDs + ) + subgraph.add(filteredNodes) + subgraphIDs = { + [id]: true, + ...subgraphIDs, + ...subsetIDs, + } } - } else { - subgraph.add(rootChild) - subgraphIDs[rootChild.variable.id] = true - } + }) } } @@ -513,8 +513,8 @@ export const hydrateVars = ( options: HydrateVarsOptions ): EventedCancelBox => { let findSubgraphFunction = findSubgraph - // if (true) { - if (isFlagEnabled('hydratevars')) { + if (true) { + // if (isFlagEnabled('hydratevars')) { findSubgraphFunction = findSubgraphFeature } From 8ebb352283ac07e723d55d1e281d4fd208938e39 Mon Sep 17 00:00:00 2001 From: asalem Date: Tue, 2 Jun 2020 16:25:34 -0700 Subject: [PATCH 06/11] feat(filterUnusedVars): turns out this works great with the new subgraph implementation so I removed the comment also add the featureflag back to the feature --- ui/src/shared/utils/filterUnusedVars.ts | 2 -- ui/src/variables/utils/hydrateVars.ts | 6 +++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/ui/src/shared/utils/filterUnusedVars.ts b/ui/src/shared/utils/filterUnusedVars.ts index bbc00d4f6a9..9172e950a67 100644 --- a/ui/src/shared/utils/filterUnusedVars.ts +++ b/ui/src/shared/utils/filterUnusedVars.ts @@ -20,8 +20,6 @@ export const filterUnusedVars = (variables: Variable[], views: View[]) => { (acc, vp) => [...acc, ...vp.queries.map(query => query.text)], [] as Array ) - // TODO: make sure to parse out variables for other used variables - // console.log('queryTexts: ', queryTexts) const varsInUse = variables.filter(variable => queryTexts.some(text => isInQuery(text, variable)) diff --git a/ui/src/variables/utils/hydrateVars.ts b/ui/src/variables/utils/hydrateVars.ts index 0c6d1fc8e51..289937155b6 100644 --- a/ui/src/variables/utils/hydrateVars.ts +++ b/ui/src/variables/utils/hydrateVars.ts @@ -74,7 +74,7 @@ export const createVariableGraph = ( return Object.values(nodesByID) } -export const isInQuery = (query: string, v: Variable) => { +export const isInQuery = (query: string, v: Variable): boolean => { const regexp = new RegExp( `${BOUNDARY_GROUP}${OPTION_NAME}.${v.name}${BOUNDARY_GROUP}` ) @@ -194,6 +194,7 @@ const getDeduplicatedRootChild = ( - The node for one of the passed variables depends on this node */ +// TODO(ariel): rename this back to findSubgraph & update tests once feature flag is removed export const findSubgraphFeature = ( graph: VariableNode[], variables: Variable[] @@ -513,8 +514,7 @@ export const hydrateVars = ( options: HydrateVarsOptions ): EventedCancelBox => { let findSubgraphFunction = findSubgraph - if (true) { - // if (isFlagEnabled('hydratevars')) { + if (isFlagEnabled('hydratevars')) { findSubgraphFunction = findSubgraphFeature } From e25d1b4616c28cd12ba6dc55fe7af877c83fb8c1 Mon Sep 17 00:00:00 2001 From: asalem Date: Tue, 2 Jun 2020 16:36:36 -0700 Subject: [PATCH 07/11] chore(changelog): updated the changelog and updated the featureflag default --- CHANGELOG.md | 2 +- flags.yml | 2 +- kit/feature/list.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29ba9491da7..8d9bf5cb7c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ 1. [18331](https://github.com/influxdata/influxdb/pull/18331): Support organization name in addition to ID in DBRP operations 1. [18335](https://github.com/influxdata/influxdb/pull/18335): Disable failing when providing an unexpected error to influx CLI 1. [18345](https://github.com/influxdata/influxdb/pull/18345): Have influx delete cmd respect the config + <<<<<<< HEAD 1. [18385](https://github.com/influxdata/influxdb/pull/18385): Store initialization for pkger enforced on reads ### UI Improvements @@ -20,7 +21,6 @@ 1. [18319](https://github.com/influxdata/influxdb/pull/18319): Display bucket ID in bucket list and enable 1 click copying 1. [18361](https://github.com/influxdata/influxdb/pull/18361): Tokens list is now consistent with the other resource lists - ## v2.0.0-beta.11 [2020-05-26] ### Features diff --git a/flags.yml b/flags.yml index 25735073a09..62dabe2a36d 100644 --- a/flags.yml +++ b/flags.yml @@ -106,6 +106,6 @@ - name: New Hydrate Vars Functionality description: Enables a minimalistic variable hydration key: hydratevars - default: true + default: false contact: Ariel Salem / Monitoring Team lifetime: temporary diff --git a/kit/feature/list.go b/kit/feature/list.go index 021b085f83a..5cd1b4455ff 100644 --- a/kit/feature/list.go +++ b/kit/feature/list.go @@ -188,7 +188,7 @@ var hydratevars = MakeBoolFlag( "New Hydrate Vars Functionality", "hydratevars", "Ariel Salem / Monitoring Team", - true, + false, Temporary, false, ) From 1335474c495b05617da74e598512c42afe3ad524 Mon Sep 17 00:00:00 2001 From: asalem Date: Wed, 3 Jun 2020 05:47:33 -0700 Subject: [PATCH 08/11] feat(hydrateVars.test): conditionally check hydrateVars result length based on featureFlag This is a temporary workaround until the featureflag can be removed so that the tests pass --- ui/src/variables/utils/hydrateVars.test.ts | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/ui/src/variables/utils/hydrateVars.test.ts b/ui/src/variables/utils/hydrateVars.test.ts index 7c84c3c2367..8a1b366c35d 100644 --- a/ui/src/variables/utils/hydrateVars.test.ts +++ b/ui/src/variables/utils/hydrateVars.test.ts @@ -5,6 +5,7 @@ import { createVariableGraph, findSubgraphFeature, } from 'src/variables/utils/hydrateVars' +import {isFlagEnabled} from 'src/shared/utils/featureFlag' // Mocks import { @@ -123,7 +124,9 @@ describe('hydrate vars', () => { We expect the final outcome to be the two associated variables as the parents, Since those are the bottom-most children of the graph cycle */ - expect(actual.length).toEqual(2) + if (isFlagEnabled('hydratevars')) { + expect(actual.length).toEqual(2) + } expect( actual.filter(v => v.id === 'e')[0].arguments.values.results ).toEqual(['eVal']) @@ -177,7 +180,9 @@ describe('hydrate vars', () => { // b [fontcolor = "red"] // c [fontcolor = "green"] // } - expect(actual.length).toEqual(1) + if (isFlagEnabled('hydratevars')) { + expect(actual.length).toEqual(1) + } const [cResult] = actual expect(cResult.arguments.values.results).toEqual(['cVal']) expect(cResult.selected).toEqual(['cVal']) @@ -222,7 +227,9 @@ describe('hydrate vars', () => { // Basic test for now, we would need an icky mock to assert that the // appropriate substitution is actually taking place - expect(actual.length).toEqual(1) + if (isFlagEnabled('hydratevars')) { + expect(actual.length).toEqual(1) + } const [bResult] = actual expect(bResult.arguments.values).toEqual({ k: 'v', @@ -266,7 +273,9 @@ describe('hydrate vars', () => { fetcher, }).promise - expect(actual.length).toEqual(1) + if (isFlagEnabled('hydratevars')) { + expect(actual.length).toEqual(1) + } const [bResult] = actual expect(bResult.arguments.values).toEqual(['v1', 'v2']) }) From ff5a8ee3b77909608b8875c7438b6adde038315f Mon Sep 17 00:00:00 2001 From: asalem Date: Wed, 3 Jun 2020 05:56:49 -0700 Subject: [PATCH 09/11] fix(tests): conditionally render the tests based on the featureFlag --- ui/src/variables/utils/hydrateVars.test.ts | 76 ++++++++++++++++++++-- 1 file changed, 69 insertions(+), 7 deletions(-) diff --git a/ui/src/variables/utils/hydrateVars.test.ts b/ui/src/variables/utils/hydrateVars.test.ts index 8a1b366c35d..fde821d8773 100644 --- a/ui/src/variables/utils/hydrateVars.test.ts +++ b/ui/src/variables/utils/hydrateVars.test.ts @@ -126,15 +126,44 @@ describe('hydrate vars', () => { */ if (isFlagEnabled('hydratevars')) { expect(actual.length).toEqual(2) + expect( + actual.filter(v => v.id === 'e')[0].arguments.values.results + ).toEqual(['eVal']) + expect(actual.filter(v => v.id === 'e')[0].selected).toEqual(['eVal']) + expect( + actual.filter(v => v.id === 'g')[0].arguments.values.results + ).toEqual(['gVal']) + expect(actual.filter(v => v.id === 'g')[0].selected).toEqual(['gVal']) } + // TODO(ariel): remove the if condition above when feature is good + // Also remove the following tests: + expect( + actual.filter(v => v.id === 'a')[0].arguments.values.results + ).toBeFalsy() + expect( + actual.filter(v => v.id === 'b')[0].arguments.values.results + ).toBeFalsy() + expect( + actual.filter(v => v.id === 'c')[0].arguments.values.results + ).toBeFalsy() + expect( + actual.filter(v => v.id === 'd')[0].arguments.values.results + ).toBeFalsy() + expect( actual.filter(v => v.id === 'e')[0].arguments.values.results ).toEqual(['eVal']) expect(actual.filter(v => v.id === 'e')[0].selected).toEqual(['eVal']) + expect( actual.filter(v => v.id === 'g')[0].arguments.values.results ).toEqual(['gVal']) expect(actual.filter(v => v.id === 'g')[0].selected).toEqual(['gVal']) + + expect( + actual.filter(v => v.id === 'f')[0].arguments.values.results + ).toEqual(['fVal']) + expect(actual.filter(v => v.id === 'f')[0].selected).toEqual(['fVal']) }) test('should invalidate all ancestors of a node when it fails', async () => { @@ -182,10 +211,23 @@ describe('hydrate vars', () => { // } if (isFlagEnabled('hydratevars')) { expect(actual.length).toEqual(1) + const [cResult] = actual + expect(cResult.arguments.values.results).toEqual(['cVal']) + expect(cResult.selected).toEqual(['cVal']) } - const [cResult] = actual - expect(cResult.arguments.values.results).toEqual(['cVal']) - expect(cResult.selected).toEqual(['cVal']) + // TODO(ariel): remove the if condition above when feature is good + // Also remove the following tests: + expect( + actual.filter(v => v.id === 'a')[0].arguments.values.results + ).toEqual([]) + expect( + actual.filter(v => v.id === 'b')[0].arguments.values.results + ).toEqual([]) + + expect( + actual.filter(v => v.id === 'c')[0].arguments.values.results + ).toEqual(['cVal']) + expect(actual.filter(v => v.id === 'c')[0].selected).toEqual(['cVal']) }) test('works with map template variables', async () => { @@ -229,9 +271,18 @@ describe('hydrate vars', () => { // appropriate substitution is actually taking place if (isFlagEnabled('hydratevars')) { expect(actual.length).toEqual(1) + const [bResult] = actual + expect(bResult.arguments.values).toEqual({ + k: 'v', + }) } - const [bResult] = actual - expect(bResult.arguments.values).toEqual({ + // TODO(ariel): remove the if condition above when feature is good + // Also remove the following tests: + expect( + actual.filter(v => v.id === 'a')[0].arguments.values.results + ).toEqual(['aVal']) + expect(actual.filter(v => v.id === 'a')[0].selected).toEqual(['aVal']) + expect(actual.filter(v => v.id === 'b')[0].arguments.values).toEqual({ k: 'v', }) }) @@ -275,9 +326,20 @@ describe('hydrate vars', () => { if (isFlagEnabled('hydratevars')) { expect(actual.length).toEqual(1) + const [bResult] = actual + expect(bResult.arguments.values).toEqual(['v1', 'v2']) } - const [bResult] = actual - expect(bResult.arguments.values).toEqual(['v1', 'v2']) + // TODO(ariel): remove the if condition above when feature is good + // Also remove the following tests: + expect( + actual.filter(v => v.id === 'a')[0].arguments.values.results + ).toEqual(['aVal']) + expect(actual.filter(v => v.id === 'a')[0].selected).toEqual(['aVal']) + + expect(actual.filter(v => v.id === 'b')[0].arguments.values).toEqual([ + 'v1', + 'v2', + ]) }) test('should be cancellable', done => { From 57319d6d14849c9177ae8c33145c0cee094fc4ae Mon Sep 17 00:00:00 2001 From: asalem Date: Wed, 3 Jun 2020 06:06:27 -0700 Subject: [PATCH 10/11] fix(test): conditionally rendering last hydrateVars test --- ui/src/variables/utils/hydrateVars.test.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/ui/src/variables/utils/hydrateVars.test.ts b/ui/src/variables/utils/hydrateVars.test.ts index fde821d8773..b736d31adbc 100644 --- a/ui/src/variables/utils/hydrateVars.test.ts +++ b/ui/src/variables/utils/hydrateVars.test.ts @@ -394,9 +394,13 @@ describe('hydrate vars', () => { selections: {}, fetcher, }).promise - - expect(actual.length).toEqual(1) - expect(actual).toEqual([defaultVariable]) + if (isFlagEnabled('hydratevars')) { + expect(actual.length).toEqual(1) + expect(actual).toEqual([defaultVariable]) + } + // TODO(ariel): remove the if condition above when feature is good + // Also remove the following tests: + expect(actual.length).toEqual(2) }) }) From 332bb51fbcf8a3567c6a92d8e1969f419348c3a1 Mon Sep 17 00:00:00 2001 From: asalem Date: Wed, 3 Jun 2020 13:16:41 -0700 Subject: [PATCH 11/11] chore(changelog): updated CHANGELOG PR --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8d9bf5cb7c4..dde2327f793 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,13 +13,13 @@ 1. [18331](https://github.com/influxdata/influxdb/pull/18331): Support organization name in addition to ID in DBRP operations 1. [18335](https://github.com/influxdata/influxdb/pull/18335): Disable failing when providing an unexpected error to influx CLI 1. [18345](https://github.com/influxdata/influxdb/pull/18345): Have influx delete cmd respect the config - <<<<<<< HEAD 1. [18385](https://github.com/influxdata/influxdb/pull/18385): Store initialization for pkger enforced on reads ### UI Improvements 1. [18319](https://github.com/influxdata/influxdb/pull/18319): Display bucket ID in bucket list and enable 1 click copying 1. [18361](https://github.com/influxdata/influxdb/pull/18361): Tokens list is now consistent with the other resource lists +1. [18346](https://github.com/influxdata/influxdb/pull/18346): Reduce the number of variables being hydrated when toggling variables ## v2.0.0-beta.11 [2020-05-26]