Skip to content

Fix a few bugs in vParquet2 structural operators #2660

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
mdisibio merged 4 commits into from
Jul 14, 2023
Merged

Fix a few bugs in vParquet2 structural operators #2660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d45629e
Fix vparquet2 structural operators to skip spans with broken nested s…
mdisibio Jul 14, 2023
75552a5
Fix bug in sibling where it falsely identified same span as its sibling
mdisibio Jul 14, 2023
4efe032
changelog
mdisibio Jul 14, 2023
94ab674
lint
mdisibio Jul 14, 2023
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* [CHANGE] Make vParquet2 the default block format [#2526](https://github.com/grafana/tempo/pull/2526) (@stoewer)
* [CHANGE] Disable tempo-query by default in Jsonnet libs. [#2462](https://github.com/grafana/tempo/pull/2462) (@electron0zero)
* [FEATURE] New experimental API to derive on-demand RED metrics grouped by any attribute, and new metrics generator processor [#2368](https://github.com/grafana/tempo/pull/2368) [#2418](https://github.com/grafana/tempo/pull/2418) [#2424](https://github.com/grafana/tempo/pull/2424) [#2442](https://github.com/grafana/tempo/pull/2442) [#2480](https://github.com/grafana/tempo/pull/2480) [#2481](https://github.com/grafana/tempo/pull/2481) [#2501](https://github.com/grafana/tempo/pull/2501) [#2579](https://github.com/grafana/tempo/pull/2579) [#2582](https://github.com/grafana/tempo/pull/2582) (@mdisibio @zalegrala)
* [FEATURE] New TraceQL structural operators descendant (>>), child (>), and sibling (~) [#2625](https://github.com/grafana/tempo/pull/2625) (@mdisibio)
* [FEATURE] New TraceQL structural operators descendant (>>), child (>), and sibling (~) [#2625](https://github.com/grafana/tempo/pull/2625) [#2660](https://github.com/grafana/tempo/pull/2660) (@mdisibio)
* [ENHANCEMENT] Add capability to flush all remaining traces to backend when ingester is stopped [#2538](https://github.com/grafana/tempo/pull/2538)
* [ENHANCEMENT] Fill parent ID column and nested set columns [#2487](https://github.com/grafana/tempo/pull/2487) (@stoewer)
* [ENHANCEMENT] Add metrics generator config option to allow customizable ring port [#2399](https://github.com/grafana/tempo/pull/2399) (@mdisibio)
Expand Down
40 changes: 30 additions & 10 deletions pkg/util/math/math.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,38 @@ func Min(a, b int) int {
return b
}

// Max64 returns the maximum of two int64s
func Max64(a, b int64) int64 {
if a > b {
return a
// Max64 returns the maximum uint64s
func Max64(values ...uint64) uint64 {
if len(values) == 0 {
return 0
}
return b
if len(values) == 1 {
return values[0]
}

x := values[0]
for i := 1; i < len(values); i++ {
if values[i] > x {
x = values[i]
}
}
return x
}

// Min64 returns the minimum of two int64s
func Min64(a, b int64) int64 {
if a < b {
return a
// Min64 returns the minimum of uint64s
func Min64(values ...uint64) uint64 {
if len(values) == 0 {
return 0
}
return b
if len(values) == 1 {
return values[0]
}

x := values[0]
for i := 1; i < len(values); i++ {
if values[i] < x {
x = values[i]
}
}
return x
}
24 changes: 22 additions & 2 deletions tempodb/encoding/vparquet2/block_traceql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,21 +55,41 @@ func (s *span) DurationNanos() uint64 {

func (s *span) DescendantOf(x traceql.Span) bool {
if ss, ok := x.(*span); ok {
return s.nestedSetLeft > ss.nestedSetLeft && s.nestedSetLeft < ss.nestedSetRight
if s.nestedSetLeft == 0 ||
s.nestedSetRight == 0 ||
ss.nestedSetLeft == 0 ||
ss.nestedSetRight == 0 {
// Spans with missing data, never a match.
return false
}
return s.nestedSetLeft > ss.nestedSetLeft && s.nestedSetRight < ss.nestedSetRight
Copy link
Copy Markdown
Contributor Author

@mdisibio mdisibio Jul 14, 2023

Choose a reason for hiding this comment

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

The typo was the third var in this statement s.nestedSetLeft -> s.nestedSetRight

}

return false
}

func (s *span) SiblingOf(x traceql.Span) bool {
if ss, ok := x.(*span); ok {
return ss.nestedSetParent == s.nestedSetParent
if s.nestedSetParent == 0 ||
ss.nestedSetParent == 0 {
return false
}
// Same parent but not ourself
// Checking pointers here means we don't have to load
// an additional column of nestedSetLeft but assumes the span
// object. This is true because all TraceQL executions are
// currently single-pass.
return ss.nestedSetParent == s.nestedSetParent && s != ss
}
return false
}

func (s *span) ChildOf(x traceql.Span) bool {
if ss, ok := x.(*span); ok {
if s.nestedSetParent == 0 ||
ss.nestedSetLeft == 0 {
return false
}
return ss.nestedSetLeft == s.nestedSetParent
}
return false
Expand Down
80 changes: 67 additions & 13 deletions tempodb/tempodb_search_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,26 +179,30 @@ func advancedTraceQLRunner(t *testing.T, wantTr *tempopb.Trace, wantMeta *tempop
// that always match multiple spans. The only way to keep these tests from being brittle
// is to ensure that all spans are selected. It's ok if a span still shows up in multiple
// filters (because of traceDuration > 0 for example) because the && operator ensures final uniqueness.
{Query: fmt.Sprintf("{ %s && %s } && { %s && %s } && { %s && %s } | avg(duration) = %dns",
{Query: fmt.Sprintf("{ %s && %s } && { %s && %s } && { %s && %s } && { %s && %s } | avg(duration) = %dns",
rando(trueConditionsBySpan[0]), rando(trueConditionsBySpan[0]),
rando(trueConditionsBySpan[1]), rando(trueConditionsBySpan[1]),
rando(trueConditionsBySpan[2]), rando(trueConditionsBySpan[2]),
(durationBySpan[0]+durationBySpan[1]+durationBySpan[2])/3)},
{Query: fmt.Sprintf("{ %s && %s } && { %s && %s } && { %s && %s } | min(duration) = %dns",
rando(trueConditionsBySpan[3]), rando(trueConditionsBySpan[3]),
(durationBySpan[0]+durationBySpan[1]+durationBySpan[2]+durationBySpan[3])/4)},
{Query: fmt.Sprintf("{ %s && %s } && { %s && %s } && { %s && %s } && { %s && %s } | min(duration) = %dns",
rando(trueConditionsBySpan[0]), rando(trueConditionsBySpan[0]),
rando(trueConditionsBySpan[1]), rando(trueConditionsBySpan[1]),
rando(trueConditionsBySpan[2]), rando(trueConditionsBySpan[2]),
math.Min64(math.Min64(int64(durationBySpan[0]), int64(durationBySpan[1])), int64(durationBySpan[2])))},
{Query: fmt.Sprintf("{ %s && %s } && { %s && %s } && { %s && %s } | max(duration) = %dns",
rando(trueConditionsBySpan[3]), rando(trueConditionsBySpan[3]),
math.Min64(durationBySpan[0], durationBySpan[1], durationBySpan[2], durationBySpan[3]))},
{Query: fmt.Sprintf("{ %s && %s } && { %s && %s } && { %s && %s } && { %s && %s } | max(duration) = %dns",
rando(trueConditionsBySpan[0]), rando(trueConditionsBySpan[0]),
rando(trueConditionsBySpan[1]), rando(trueConditionsBySpan[1]),
rando(trueConditionsBySpan[2]), rando(trueConditionsBySpan[2]),
math.Max64(math.Max64(int64(durationBySpan[0]), int64(durationBySpan[1])), int64(durationBySpan[2])))},
{Query: fmt.Sprintf("{ %s && %s } && { %s && %s } && { %s && %s } | sum(duration) = %dns",
rando(trueConditionsBySpan[3]), rando(trueConditionsBySpan[3]),
math.Max64(durationBySpan[0], durationBySpan[1], durationBySpan[2], durationBySpan[3]))},
{Query: fmt.Sprintf("{ %s && %s } && { %s && %s } && { %s && %s } && { %s && %s }| sum(duration) = %dns",
rando(trueConditionsBySpan[0]), rando(trueConditionsBySpan[0]),
rando(trueConditionsBySpan[1]), rando(trueConditionsBySpan[1]),
rando(trueConditionsBySpan[2]), rando(trueConditionsBySpan[2]),
durationBySpan[0]+durationBySpan[1]+durationBySpan[2])},
rando(trueConditionsBySpan[3]), rando(trueConditionsBySpan[3]),
durationBySpan[0]+durationBySpan[1]+durationBySpan[2]+durationBySpan[3])},
// groupin' (.foo is a known attribute that is the same on both spans)
{Query: "{} | by(span.foo) | count() = 2"},
{Query: "{} | by(resource.service.name) | count() = 1"},
Expand Down Expand Up @@ -365,6 +369,24 @@ func groupTraceQLRunner(t *testing.T, _ *tempopb.Trace, wantMeta *tempopb.TraceS
{Key: "count()", Value: &v1_common.AnyValue{Value: &v1_common.AnyValue_IntValue{IntValue: 1}}},
},
},
{
Spans: []*tempopb.Span{
{
SpanID: "0000000000000000",
StartTimeUnixNano: 1000000000000,
DurationNanos: 1000000000,
Name: "",
Attributes: []*v1_common.KeyValue{
{Key: "service.name", Value: &v1_common.AnyValue{Value: &v1_common.AnyValue_StringValue{StringValue: "BrokenService"}}},
},
},
},
Matched: 1,
Attributes: []*v1_common.KeyValue{
{Key: "by(resource.service.name)", Value: &v1_common.AnyValue{Value: &v1_common.AnyValue_StringValue{StringValue: "BrokenService"}}},
{Key: "count()", Value: &v1_common.AnyValue{Value: &v1_common.AnyValue_IntValue{IntValue: 1}}},
},
},
},
},
},
Expand Down Expand Up @@ -519,6 +541,13 @@ func traceQLStructural(t *testing.T, _ *tempopb.Trace, wantMeta *tempopb.TraceSe
{Query: "{ .child } >> { .parent }"},
{Query: "{ .child } > { .parent }"},
{Query: "{ .child } ~ { .parent }"},
{Query: "{ .child } ~ { .child }"},
{Query: "{ .broken} >> {}"},
{Query: "{ .broken} > {}"},
{Query: "{ .broken} ~ {}"},
{Query: "{} >> {.broken}"},
{Query: "{} > {.broken}"},
{Query: "{} ~ {.broken}"},
}

for _, tc := range searchesThatMatch {
Expand Down Expand Up @@ -732,10 +761,10 @@ func intKV(k string, v int) *v1_common.KeyValue {
}
}

func boolKV(k string, v bool) *v1_common.KeyValue {
func boolKV(k string) *v1_common.KeyValue {
return &v1_common.KeyValue{
Key: k,
Value: &v1_common.AnyValue{Value: &v1_common.AnyValue_BoolValue{BoolValue: v}},
Value: &v1_common.AnyValue{Value: &v1_common.AnyValue_BoolValue{BoolValue: true}},
}
}

Expand Down Expand Up @@ -846,7 +875,7 @@ func searchTestSuite() (
stringKV("http.url", "url/Hello/World"),
intKV("http.status_code", 500),
stringKV("foo", "Bar"),
boolKV("child", true),
boolKV("child"),
},
},
},
Expand All @@ -872,7 +901,7 @@ func searchTestSuite() (
Kind: v1.Span_SPAN_KIND_CLIENT,
Attributes: []*v1_common.KeyValue{
stringKV("foo", "Bar"),
boolKV("parent", true),
boolKV("parent"),
},
},
},
Expand All @@ -897,7 +926,32 @@ func searchTestSuite() (
Kind: v1.Span_SPAN_KIND_PRODUCER,
Status: &v1.Status{Code: v1.Status_STATUS_CODE_OK},
Attributes: []*v1_common.KeyValue{
boolKV("child2", true),
boolKV("child2"),
},
},
},
},
},
},
{
Resource: &v1_resource.Resource{
Attributes: []*v1_common.KeyValue{
stringKV("service.name", "BrokenService"),
},
},
ScopeSpans: []*v1.ScopeSpans{
{
Spans: []*v1.Span{
{
Name: "BrokenSpan",
TraceId: id,
SpanId: []byte{0, 0, 0},
ParentSpanId: []byte{0, 0, 0},
StartTimeUnixNano: uint64(1000 * time.Second),
EndTimeUnixNano: uint64(1001 * time.Second),
Status: &v1.Status{Code: v1.Status_STATUS_CODE_OK},
Attributes: []*v1_common.KeyValue{
boolKV("broken"),
},
},
},
Expand Down