Skip to content
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

Fix: Add fields to frame if it does not already exist when grouping by multiple terms #392

Merged
merged 3 commits into from
May 24, 2024
Merged

Fix: Add fields to frame if it does not already exist when grouping by multiple terms #392

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
30adb59
Fix: Add fields to frame if it does not already exist when grouping b…
kevinwcyu May 22, 2024
afa2766
use utils.Pointer
kevinwcyu May 23, 2024
1cce630
pr comment
kevinwcyu May 23, 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
9 changes: 7 additions & 2 deletions pkg/opensearch/response_parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -999,7 +999,11 @@ func (rp *responseParser) processAggregationDocs(esAgg *simplejson.Json, aggDef
frames := data.Frames{}
var fields []*data.Field

if queryResult.Frames == nil {
if queryResult.Frames != nil && len(queryResult.Frames) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit: you can just write if len(queryResult.Frames) != 0 here - you'd only need the nil check if queryResult.Frames was a pointer.

for _, frame := range queryResult.Frames {
fields = append(fields, frame.Fields...)
}
} else {
for _, propKey := range propKeys {
fields = append(fields, data.NewField(propKey, nil, []*string{}))
}
Expand All @@ -1012,7 +1016,8 @@ func (rp *responseParser) processAggregationDocs(esAgg *simplejson.Json, aggDef
for _, e := range fields {
for _, propKey := range propKeys {
if e.Name == propKey {
e.Append(props[propKey])
value := props[propKey]
e.Append(&value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious - did something in the sdk or elsewhere change, or did this code path never get executed before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this bit of the code used to build the results the same way as the frontend flow (building a table), but was refactored to use data frames instead. that refactoring was done by following elasticsearch's pr (which had the same bug that this PR is addressing). so it probably wasn't being executed from that point onwards. but we didn't notice because the backend flow would've only been run for alerts or in explore and it would've only been an issue if someone was using multiple group by terms.

kevinwcyu marked this conversation as resolved.
Show resolved Hide resolved
}
}
if e.Name == aggDef.Field {
Expand Down
103 changes: 103 additions & 0 deletions pkg/opensearch/response_parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,109 @@ func Test_ResponseParser_test(t *testing.T) {
assert.EqualValues(t, 32, *seriesFour.Fields[1].At(1).(*float64))
})

t.Run("Multiple group by query with two metrics", func(t *testing.T) {
targets := []tsdbQuery{{
refId: "A",
body: `{
"timeField": "@timestamp",
"metrics": [{"id": "1", "type": "count"}, {"id": "4", "type": "max", "field": "DistanceMiles"}],
"bucketAggs": [{"field": "DestCityName", "id": "3", "type": "terms"}, {"field": "FlightDelayType", "id": "2", "type": "terms"}]
}`,
}}
response := `{
"responses": [
{
"aggregations": {
"3": {
"buckets": [
{
"2": {
"buckets": [
{
"4": {
"value": 5640.1
},
"key": "Weather Delay",
"doc_count": 10
},
{
"4": {
"value": 5624.2
},
"key": "Security Delay",
"doc_count": 15
}
]
},
"key": "Zurich",
"doc_count": 691
},
{
"2": {
"buckets": [
{
"4": {
"value": 8245.1
},
"key": "Weather Delay",
"doc_count": 9
},
{
"4": {
"value": 8300.4
},
"key": "Security Delay",
"doc_count": 8
}
]
},
"key": "Xi'an",
"doc_count": 526
}
]
}
}
}
]
}`
rp, err := newResponseParserForTest(targets, response, nil, client.ConfiguredFields{TimeField: "@timestamp"}, nil)
assert.Nil(t, err)
result, err := rp.parseResponse()
assert.Nil(t, err)
require.Len(t, result.Responses, 1)

queryRes := result.Responses["A"]
assert.NotNil(t, queryRes)
assert.Len(t, queryRes.Frames, 1)
frame := queryRes.Frames[0]
require.Len(t, frame.Fields, 4)

assert.Equal(t, "DestCityName", frame.Fields[0].Name)
assert.Equal(t, "FlightDelayType", frame.Fields[1].Name)
assert.Equal(t, "Count", frame.Fields[2].Name)
assert.Equal(t, "Max", frame.Fields[3].Name)

assert.Equal(t, "Zurich", *frame.Fields[0].At(0).(*string))
assert.Equal(t, "Weather Delay", *frame.Fields[1].At(0).(*string))
assert.Equal(t, float64(10), *frame.Fields[2].At(0).(*float64))
assert.Equal(t, float64(5640.1), *frame.Fields[3].At(0).(*float64))

assert.Equal(t, "Zurich", *frame.Fields[0].At(1).(*string))
assert.Equal(t, "Security Delay", *frame.Fields[1].At(1).(*string))
assert.Equal(t, float64(15), *frame.Fields[2].At(1).(*float64))
assert.Equal(t, float64(5624.2), *frame.Fields[3].At(1).(*float64))

assert.Equal(t, "Xi'an", *frame.Fields[0].At(2).(*string))
assert.Equal(t, "Weather Delay", *frame.Fields[1].At(2).(*string))
assert.Equal(t, float64(9), *frame.Fields[2].At(2).(*float64))
assert.Equal(t, float64(8245.1), *frame.Fields[3].At(2).(*float64))

assert.Equal(t, "Xi'an", *frame.Fields[0].At(3).(*string))
assert.Equal(t, "Security Delay", *frame.Fields[1].At(3).(*string))
assert.Equal(t, float64(8), *frame.Fields[2].At(3).(*float64))
assert.Equal(t, float64(8300.4), *frame.Fields[3].At(3).(*float64))
})

t.Run("With percentiles", func(t *testing.T) {
targets := []tsdbQuery{{
refId: "A",
Expand Down
Loading