Address child depth issues with scripts. Add tree address validation to test.

Author: scunningham

pkg/ast/ast.go

@@ -197,7 +197,7 @@ func (b *builderT) buildLeafChild(parserNode *parser.NodeT, machineAddress *AstN
 	case parserNode.IsMatcherNode():
 		leaf, err = b.buildMatcherChild(parserNode, machineAddress, termIdx)
 	case parserNode.IsPromNode():
-		leaf, err = b.buildPromQLNode(parserNode, machineAddress, termIdx)
+		leaf, err = b.buildPromQLChild(parserNode, machineAddress, termIdx)
 	}
 	return
 }
@@ -248,9 +248,11 @@ func (b *builderT) buildMatcherChild(parserNode *parser.NodeT, machineAddress *A
 		return nil, parserNode.WrapError(ErrInvalidEventType)
 	}
 
-	// Implied that the root node has an origin event
-	b.OriginCnt++
-	parserNode.Metadata.Event.Origin = true
+	// This appears to be a legacy hack to support rules that don't specify origin but have event sources.
+	// We should consider removing this and requiring explicit origin specification in the rules.
+	if b.CurrentDepth == 0 && !parserNode.Metadata.Event.Origin {
+		parserNode.Metadata.Event.Origin = true
+	}
 
 	err = b.descendTree(func() error {
 		if matchNode, err = b.buildMatcherNodes(parserNode, machineAddress, termIdx); err != nil {
@@ -329,10 +331,6 @@ func (b *builderT) buildMachineChildren(parserNode *parser.NodeT, machineAddress
 
 		// If the child has an event/data source, then it is not a state machine. Build it via buildMatcherNodes
 
-		if parserChildNode.Metadata.Event.Origin {
-			b.OriginCnt++
-		}
-
 		if parserChildNode.Metadata.Event.Source == "" {
 			log.Error().
 				Any("address", machineAddress).

pkg/ast/ast_log.go

@@ -152,6 +152,10 @@ func (b *builderT) doBuildLogMatcherNode(parserNode *parser.NodeT, machineAddres
 		Correlations: parserNode.Metadata.Correlations,
 	}
 
+	if parserNode.Metadata.Event.Origin {
+		b.OriginCnt++
+	}
+
 	return matchNode, nil
 }
 

pkg/ast/ast_metrics.go

@@ -15,6 +15,22 @@ type AstPromQL struct {
 	Event    *AstEventT
 }
 
+func (b *builderT) buildPromQLChild(parserNode *parser.NodeT, machineAddress *AstNodeAddressT, termIdx *uint32) (*AstNodeT, error) {
+	var child *AstNodeT
+
+	err := b.descendTree(func() error {
+		node, err := b.buildPromQLNode(parserNode, machineAddress, termIdx)
+		if err != nil {
+			return err
+		}
+		child = node
+		return nil
+	})
+
+	return child, err
+
+}
+
 func (b *builderT) buildPromQLNode(parserNode *parser.NodeT, machineAddress *AstNodeAddressT, termIdx *uint32) (*AstNodeT, error) {
 
 	// Expects one child of type ParsePromQL
@@ -45,6 +61,9 @@ func (b *builderT) buildPromQLNode(parserNode *parser.NodeT, machineAddress *Ast
 			Source: parserNode.Metadata.Event.Source,
 			Origin: parserNode.Metadata.Event.Origin,
 		}
+		if parserNode.Metadata.Event.Origin {
+			b.OriginCnt++
+		}
 	}
 
 	if promNode.Interval != nil {

pkg/ast/ast_script.go

@@ -39,19 +39,25 @@ func (b *builderT) buildScriptChildren(parserNode *parser.NodeT, machineAddress
 
 	leaf, err := b.buildLeafChild(parserChildNode, machineAddress, &termIdx)
 
+	var childList []*AstNodeT
+
 	switch {
 	case err != nil:
-		return nil, err
+		// fallthrough
 	case leaf != nil:
-		return []*AstNodeT{leaf}, nil
+		childList = []*AstNodeT{leaf}
 	default:
-		node, err := b.buildTree(parserChildNode, machineAddress, &termIdx)
-		if err != nil {
-			return nil, err
-		}
-
-		return []*AstNodeT{node}, nil
+		err = b.descendTree(func() error {
+			node, err := b.buildTree(parserChildNode, machineAddress, &termIdx)
+			if err != nil {
+				return err
+			}
+			childList = []*AstNodeT{node}
+			return nil
+		})
 	}
+
+	return childList, err
 }
 
 // Validate script definitions and build the script node.

pkg/ast/ast_test.go

@@ -84,6 +84,10 @@ func TestAstSuccess(t *testing.T) {
 			rule:              testdata.TestSuccessChildScriptMultipleInputs,
 			expectedNodeTypes: []string{"machine_set", "script", "machine_seq", "log_seq", "log_set"},
 		},
+		"Success_ChildScriptPromQLInput": {
+			rule:              testdata.TestSuccessChildScriptPromQLInput,
+			expectedNodeTypes: []string{"machine_set", "script", "promql"},
+		},
 	}
 
 	for name, test := range tests {
@@ -104,6 +108,10 @@ func TestAstSuccess(t *testing.T) {
 				t.Fatalf("No nodes found in AST")
 			}
 
+			if err = validateTree(ast.Nodes[0]); err != nil {
+				t.Fatalf("Error validating tree: %v", err)
+			}
+
 			var actualNodes []string
 			gatherNodeTypes(ast.Nodes[0], &actualNodes)
 
@@ -273,3 +281,45 @@ func TestFailureExamples(t *testing.T) {
 		}
 	}
 }
+
+// Validate the following invariants on the tree:
+// 1. No duplicate addresses
+// 2. Root node has no parent address
+// 3. Node ids are unique
+// 4. Depth is consistent with distance from root
+
+func validateTree(node *AstNodeT) error {
+	if node == nil {
+		return fmt.Errorf("Root node is nil")
+	}
+
+	if node.Metadata.ParentAddress != nil {
+		return fmt.Errorf("Root node has parent address: %s", node.Metadata.ParentAddress.String())
+	}
+
+	return _validateTree(node, 0, make(map[uint32]struct{})) // start at depth 0 for root
+}
+
+func _validateTree(node *AstNodeT, depth uint32, ids map[uint32]struct{}) error {
+
+	if node == nil {
+		return nil
+	}
+
+	if node.Metadata.Address.Depth != depth {
+		return fmt.Errorf("Node %s has depth %d, expected %d", node.Metadata.Address.String(), node.Metadata.Address.Depth, depth)
+	}
+
+	if _, exists := ids[node.Metadata.Address.NodeId]; exists {
+		return fmt.Errorf("Duplicate node ID %d found", node.Metadata.Address.NodeId)
+	}
+	ids[node.Metadata.Address.NodeId] = struct{}{}
+
+	for _, child := range node.Children {
+		if err := _validateTree(child, depth+1, ids); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}

pkg/parser/parse_test.go

@@ -85,6 +85,11 @@ func TestParseSuccess(t *testing.T) {
 			expectedNodeTypes:  []string{"machine_set", "script", "machine_seq", "log_seq", "log_set"},
 			expectedNegIndexes: []int{-1, -1, -1, -1, -1},
 		},
+		"Success_ChildScriptPromQLInput": {
+			rule:               testdata.TestSuccessChildScriptPromQLInput,
+			expectedNodeTypes:  []string{"machine_set", "script", "promql"},
+			expectedNegIndexes: []int{-1, -1, -1},
+		},
 	}
 
 	for name, test := range tests {

pkg/testdata/rules.go

@@ -460,6 +460,29 @@ rules:
                           - value: term4
 `
 
+var TestSuccessChildScriptPromQLInput = `
+rules:
+  - cre:
+      id: TestSuccessChildScriptPromQLInput
+    metadata:
+      id: "J7uRQTGpGMyL1iFpssnBeS"
+      hash: "rdJLgqYgkEp8jg8Qks1qiq"
+      generation: 1
+    rule:
+      set:
+        window: 30s
+        match:
+          - script:
+              code: "function process(ev) print(\"Processing...\") end"
+              input:
+                promql:
+                  event:
+                    source: cre.metrics
+                    origin: true
+                  expr: 'sum(rate(http_requests_total[5m])) by (service)'
+                  interval: 10s
+`
+
 /* Failure cases */
 var TestFailTypo = ` # Line 1 starts here
 rules: