Skip to content

TraceQL performance improvements #4114

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 24, 2024
Merged

TraceQL performance improvements #4114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a51087
performance improvements
mdisibio Sep 24, 2024
faefa70
Exit binop early withou evaluating RHS if possible
mdisibio Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions pkg/traceql/ast_execute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,20 @@ func (o *BinaryOperation) execute(span Span) (Static, error) {
return NewStaticNil(), err
}

// Look for cases where we don't even need to evalulate the RHS
if lhsB, ok := lhs.Bool(); ok {
if o.Op == OpAnd && !lhsB {
// x && y
// x is false so we don't need to evalulate y
return StaticFalse, nil
}
if o.Op == OpOr && lhsB {
// x || y
// x is true so we don't need to evalulate y
return StaticTrue, nil
}
}

rhs, err := o.RHS.execute(span)
if err != nil {
return NewStaticNil(), err
Expand All @@ -339,7 +353,7 @@ func (o *BinaryOperation) execute(span Span) (Static, error) {
lhsT := lhs.Type
rhsT := rhs.Type
if !lhsT.isMatchingOperand(rhsT) {
return NewStaticBool(false), nil
return StaticFalse, nil
}

if !o.Op.binaryTypesValid(lhsT, rhsT) {
Expand Down Expand Up @@ -585,7 +599,7 @@ func (a Attribute) execute(span Span) (Static, error) {
return static, nil
}

return NewStaticNil(), nil
return StaticNil, nil
}

func uniqueSpans(ss1 []*Spanset, ss2 []*Spanset) []Span {
Expand Down
1 change: 1 addition & 0 deletions pkg/traceql/enum_statics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ func (k Kind) String() string {
}

var (
StaticNil = NewStaticNil()
StaticTrue = NewStaticBool(true)
StaticFalse = NewStaticBool(false)
)
105 changes: 62 additions & 43 deletions tempodb/encoding/vparquet4/block_traceql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ func (s *span) AttributeFor(a traceql.Attribute) (traceql.Static, bool) {
if attrs[0].a == a {
return &attrs[0].s
}
return nil
}
if len(attrs) == 2 {
if attrs[0].a == a {
Expand All @@ -130,11 +131,12 @@ func (s *span) AttributeFor(a traceql.Attribute) (traceql.Static, bool) {
if attrs[1].a == a {
return &attrs[1].s
}
return nil
}

for _, st := range attrs {
if st.a == a {
return &st.s
for i := range attrs {
if attrs[i].a == a {
return &attrs[i].s
}
}
return nil
Expand All @@ -144,6 +146,7 @@ func (s *span) AttributeFor(a traceql.Attribute) (traceql.Static, bool) {
if attrs[0].a.Name == s {
return &attrs[0].s
}
return nil
Copy link
Contributor

@javiermolinar javiermolinar Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat

}
if len(attrs) == 2 {
if attrs[0].a.Name == s {
Expand All @@ -152,47 +155,53 @@ func (s *span) AttributeFor(a traceql.Attribute) (traceql.Static, bool) {
if attrs[1].a.Name == s {
return &attrs[1].s
}
return nil
Copy link
Contributor

@javiermolinar javiermolinar Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we could return directly the StaticNil from here

}

for _, st := range attrs {
if st.a.Name == s {
return &st.s
for i := range attrs {
if attrs[i].a.Name == s {
return &attrs[i].s
}
}
return nil
}

if a.Scope == traceql.AttributeScopeResource {
if attr := find(a, s.resourceAttrs); attr != nil {
return *attr, true
switch a.Scope {
case traceql.AttributeScopeResource:
if len(s.resourceAttrs) > 0 {
if attr := find(a, s.resourceAttrs); attr != nil {
return *attr, true
}
}
return traceql.NewStaticNil(), false
}

if a.Scope == traceql.AttributeScopeSpan {
if attr := find(a, s.spanAttrs); attr != nil {
return *attr, true
return traceql.StaticNil, false
case traceql.AttributeScopeSpan:
if len(s.spanAttrs) > 0 {
if attr := find(a, s.spanAttrs); attr != nil {
return *attr, true
}
}
return traceql.NewStaticNil(), false
}

if a.Scope == traceql.AttributeScopeEvent {
if attr := find(a, s.eventAttrs); attr != nil {
return *attr, true
return traceql.StaticNil, false
case traceql.AttributeScopeEvent:
if len(s.eventAttrs) > 0 {
if attr := find(a, s.eventAttrs); attr != nil {
return *attr, true
}
}
return traceql.NewStaticNil(), false
}
if a.Scope == traceql.AttributeScopeLink {
if attr := find(a, s.linkAttrs); attr != nil {
return *attr, true
return traceql.StaticNil, false
case traceql.AttributeScopeLink:
if len(s.linkAttrs) > 0 {
if attr := find(a, s.linkAttrs); attr != nil {
return *attr, true
}
}
return traceql.NewStaticNil(), false
}
if a.Scope == traceql.AttributeScopeInstrumentation {
if attr := find(a, s.instrumentationAttrs); attr != nil {
return *attr, true
return traceql.StaticNil, false
case traceql.AttributeScopeInstrumentation:
if len(s.instrumentationAttrs) > 0 {
if attr := find(a, s.instrumentationAttrs); attr != nil {
return *attr, true
}
}
return traceql.NewStaticNil(), false
return traceql.StaticNil, false
}

if a.Intrinsic != traceql.IntrinsicNone {
Expand Down Expand Up @@ -230,27 +239,37 @@ func (s *span) AttributeFor(a traceql.Attribute) (traceql.Static, bool) {

// name search in span, resource, link, and event to give precedence to span
// we don't need to do a name search at the trace level b/c it is intrinsics only
if attr := findName(a.Name, s.spanAttrs); attr != nil {
return *attr, true
if len(s.spanAttrs) > 0 {
Copy link
Contributor

@javiermolinar javiermolinar Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we move this logic to the findName method instead? We are already counting the length. We can add a first case:

if len(attrs) == 0 {
return nil
}

We are adding a new entry in the call stack but the code is nicer

Copy link
Contributor Author

@mdisibio mdisibio Sep 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this way it's skipping the method call entirely, so it ends up faster. I think it's worth it, even though the code is less nice.

if attr := findName(a.Name, s.spanAttrs); attr != nil {
return *attr, true
}
}

if attr := findName(a.Name, s.resourceAttrs); attr != nil {
return *attr, true
if len(s.resourceAttrs) > 0 {
if attr := findName(a.Name, s.resourceAttrs); attr != nil {
return *attr, true
}
}

if attr := findName(a.Name, s.eventAttrs); attr != nil {
return *attr, true
if len(s.eventAttrs) > 0 {
if attr := findName(a.Name, s.eventAttrs); attr != nil {
return *attr, true
}
}

if attr := findName(a.Name, s.linkAttrs); attr != nil {
return *attr, true
if len(s.linkAttrs) > 0 {
if attr := findName(a.Name, s.linkAttrs); attr != nil {
return *attr, true
}
}

if attr := findName(a.Name, s.instrumentationAttrs); attr != nil {
return *attr, true
if len(s.instrumentationAttrs) > 0 {
if attr := findName(a.Name, s.instrumentationAttrs); attr != nil {
return *attr, true
}
}

return traceql.NewStaticNil(), false
return traceql.StaticNil, false
}

func (s *span) ID() []byte {
Expand Down