chore: some cleanup of filter suggestion tests

SigNoz · Aug 5, 2024 · c040bc9 · c040bc9
1 parent 7b49680
commit c040bc9
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 105 deletions.
diff --git a/pkg/query-service/app/clickhouseReader/reader.go b/pkg/query-service/app/clickhouseReader/reader.go
@@ -4430,11 +4430,11 @@ func (r *ClickHouseReader) GetQBFilterSuggestionsForLogs(
 		}
 	}
 
-	// Suggest example query for top suggested attribute
-	// using existing autocomplete logic for recommending attrib values
+	// Suggest example query for top suggested attribute using existing
+	// autocomplete logic for recommending attrib values
 	//
-	// TODO(Raj): create example queries from multiple top attributes using
-	// a batch version of GetLogAttributeValues
+	// Example queries for multiple top attributes using a batch version of
+	// GetLogAttributeValues is expected to come in a follow up change
 	if len(suggestions.AttributeKeys) > 0 {
 		topAttrib := suggestions.AttributeKeys[0]
 

diff --git a/pkg/query-service/tests/integration/filter_suggestions_test.go b/pkg/query-service/tests/integration/filter_suggestions_test.go
@@ -4,10 +4,7 @@ import (
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
-	"io"
 	"net/http"
-	"net/http/httptest"
-	"runtime/debug"
 	"slices"
 	"strings"
 	"testing"
@@ -23,15 +20,15 @@ import (
 	"go.signoz.io/signoz/pkg/query-service/utils"
 )
 
-// Filter suggestions should contain standard fields and static
-// examples based on them if no data has been received yet
+// If no data has been received yet, filter suggestions should contain
+// standard log fields and static example queries based on them
 func TestDefaultLogsFilterSuggestions(t *testing.T) {
 	require := require.New(t)
 	tb := NewFilterSuggestionsTestBed(t)
 
-	expectAttribKeysQuery(tb.mockClickhouse, []v3.AttributeKey{})
-	queryParams := map[string]string{}
-	suggestionsResp := tb.GetQBFilterSuggestionsForLogs(queryParams)
+	tb.mockAttribKeysQueryResponse([]v3.AttributeKey{})
+	suggestionsQueryParams := map[string]string{}
+	suggestionsResp := tb.GetQBFilterSuggestionsForLogs(suggestionsQueryParams)
 
 	require.Greater(len(suggestionsResp.AttributeKeys), 0)
 	require.True(slices.ContainsFunc(
@@ -41,6 +38,11 @@ func TestDefaultLogsFilterSuggestions(t *testing.T) {
 	))
 
 	require.Greater(len(suggestionsResp.ExampleQueries), 0)
+	require.False(slices.ContainsFunc(
+		suggestionsResp.AttributeKeys, func(a v3.AttributeKey) bool {
+			return a.Type == v3.AttributeKeyTypeTag || a.Type == v3.AttributeKeyTypeResource
+		},
+	))
 }
 
 func TestLogsFilterSuggestionsWithoutExistingFilter(t *testing.T) {
@@ -55,24 +57,19 @@ func TestLogsFilterSuggestionsWithoutExistingFilter(t *testing.T) {
 	}
 	testAttribValue := "test-container"
 
-	expectAttribKeysQuery(tb.mockClickhouse, []v3.AttributeKey{
-		testAttrib,
-	})
-	expectAttribValuesQuery(
-		tb.mockClickhouse, testAttrib, []string{testAttribValue},
-	)
-	queryParams := map[string]string{}
-	suggestionsResp := tb.GetQBFilterSuggestionsForLogs(queryParams)
+	tb.mockAttribKeysQueryResponse([]v3.AttributeKey{testAttrib})
+	tb.mockAttribValuesQueryResponse(testAttrib, []string{testAttribValue})
+	suggestionsQueryParams := map[string]string{}
+	suggestionsResp := tb.GetQBFilterSuggestionsForLogs(suggestionsQueryParams)
 
 	require.Greater(len(suggestionsResp.AttributeKeys), 0)
 	require.True(slices.ContainsFunc(
 		suggestionsResp.AttributeKeys, func(a v3.AttributeKey) bool {
-			return a.Key == "container_id" && a.Type == testAttrib.Type
+			return a.Key == testAttrib.Key && a.Type == testAttrib.Type
 		},
 	))
 
 	require.Greater(len(suggestionsResp.ExampleQueries), 0)
-
 	require.True(slices.ContainsFunc(
 		suggestionsResp.ExampleQueries, func(q v3.FilterSet) bool {
 			return slices.ContainsFunc(q.Items, func(i v3.FilterItem) bool {
@@ -82,6 +79,8 @@ func TestLogsFilterSuggestionsWithoutExistingFilter(t *testing.T) {
 	))
 }
 
+// If a filter already exists, should not suggest a used attribute
+// and suggested example queries should contain existing filter
 func TestLogsFilterSuggestionsWithExistingFilter(t *testing.T) {
 	require := require.New(t)
 	tb := NewFilterSuggestionsTestBed(t)
@@ -112,17 +111,13 @@ func TestLogsFilterSuggestionsWithExistingFilter(t *testing.T) {
 		},
 	}
 
-	// Should recommend both attributes without an existing filter.
-
-	expectAttribKeysQuery(tb.mockClickhouse, []v3.AttributeKey{
-		testAttrib, testFilterAttrib,
-	})
-	expectAttribValuesQuery(
-		tb.mockClickhouse, testAttrib, []string{testAttribValue},
-	)
+	// If suggestions request doesn't specify an existing filter,
+	// both attributes should get recommended.
+	tb.mockAttribKeysQueryResponse([]v3.AttributeKey{testAttrib, testFilterAttrib})
+	tb.mockAttribValuesQueryResponse(testAttrib, []string{testAttribValue})
 
-	queryParams := map[string]string{}
-	suggestionsResp := tb.GetQBFilterSuggestionsForLogs(queryParams)
+	suggestionsQueryParams := map[string]string{}
+	suggestionsResp := tb.GetQBFilterSuggestionsForLogs(suggestionsQueryParams)
 
 	require.Greater(len(suggestionsResp.AttributeKeys), 0)
 	require.True(slices.ContainsFunc(
@@ -138,24 +133,20 @@ func TestLogsFilterSuggestionsWithExistingFilter(t *testing.T) {
 
 	require.Greater(len(suggestionsResp.ExampleQueries), 0)
 
-	// Should not recommend attrib already in existing filter
+	// If suggestions request specifies an existing filter, only the unused
+	// attribute should get recommended and example query should contain existing filter
+	tb.mockAttribKeysQueryResponse([]v3.AttributeKey{testAttrib, testFilterAttrib})
+	tb.mockAttribValuesQueryResponse(testAttrib, []string{testAttribValue})
 
-	expectAttribKeysQuery(tb.mockClickhouse, []v3.AttributeKey{
-		testAttrib, testFilterAttrib,
-	})
-	expectAttribValuesQuery(
-		tb.mockClickhouse, testAttrib, []string{testAttribValue},
-	)
-
-	// TODO(Raj): Validate RawURLEncoding maps to what the browser does
 	testFilterJson, err := json.Marshal(testFilter)
 	require.Nil(err, "couldn't serialize existing filter to JSON")
-	queryParams = map[string]string{
+	suggestionsQueryParams = map[string]string{
 		"existingFilter": base64.RawURLEncoding.EncodeToString(testFilterJson),
 	}
-	suggestionsResp = tb.GetQBFilterSuggestionsForLogs(queryParams)
+	suggestionsResp = tb.GetQBFilterSuggestionsForLogs(suggestionsQueryParams)
 
 	require.Greater(len(suggestionsResp.AttributeKeys), 0)
+	// Attribute already used in filter should not get recommended.
 	require.False(slices.ContainsFunc(
 		suggestionsResp.AttributeKeys, func(a v3.AttributeKey) bool {
 			return a.Key == testFilter.Items[0].Key.Key && a.Type == testFilter.Items[0].Key.Type
@@ -167,19 +158,17 @@ func TestLogsFilterSuggestionsWithExistingFilter(t *testing.T) {
 		},
 	))
 
-	// All example queries should contain the existing query as a prefix
+	// All example queries should contain the existing filter as a prefix
 	require.Greater(len(suggestionsResp.ExampleQueries), 0)
 	for _, q := range suggestionsResp.ExampleQueries {
 		require.Equal(q.Items[0], testFilter.Items[0])
 	}
 }
 
-func expectAttribKeysQuery(
-	mockClickhouse mockhouse.ClickConnMockCommon,
+// Mocks response for CH queries made by reader.GetLogAttributeKeys
+func (tb *FilterSuggestionsTestBed) mockAttribKeysQueryResponse(
 	attribsToReturn []v3.AttributeKey,
 ) {
-
-	// Add expectation for querying distributed_tag_attributes table
 	cols := []mockhouse.ColumnType{}
 	cols = append(cols, mockhouse.ColumnType{Type: "String", Name: "tagKey"})
 	cols = append(cols, mockhouse.ColumnType{Type: "String", Name: "tagType"})
@@ -194,22 +183,22 @@ func expectAttribKeysQuery(
 		values = append(values, rowValues)
 	}
 
-	mockClickhouse.ExpectQuery(
+	tb.mockClickhouse.ExpectQuery(
 		"select.*from.*signoz_logs.distributed_tag_attributes.*",
 	).WithArgs(50).WillReturnRows(mockhouse.NewRows(cols, values))
 
 	// Add expectation for the create table query used to determine
 	// if an attribute is a column
 	cols = []mockhouse.ColumnType{{Type: "String", Name: "statement"}}
 	values = [][]any{{"CREATE TABLE signoz_logs.distributed_logs"}}
-	mockClickhouse.ExpectSelect(
+	tb.mockClickhouse.ExpectSelect(
 		"SHOW CREATE TABLE.*",
 	).WillReturnRows(mockhouse.NewRows(cols, values))
 
 }
 
-func expectAttribValuesQuery(
-	mockClickhouse mockhouse.ClickConnMockCommon,
+// Mocks response for CH queries made by reader.GetLogAttributeValues
+func (tb *FilterSuggestionsTestBed) mockAttribValuesQueryResponse(
 	expectedAttrib v3.AttributeKey,
 	stringValuesToReturn []string,
 ) {
@@ -223,7 +212,7 @@ func expectAttribValuesQuery(
 		values = append(values, rowValues)
 	}
 
-	mockClickhouse.ExpectQuery(
+	tb.mockClickhouse.ExpectQuery(
 		"select distinct.*stringTagValue.*from.*signoz_logs.distributed_tag_attributes.*",
 	).WithArgs(string(expectedAttrib.Key), v3.TagType(expectedAttrib.Type), 1).WillReturnRows(mockhouse.NewRows(cols, values))
 }
@@ -239,8 +228,8 @@ func (tb *FilterSuggestionsTestBed) GetQBFilterSuggestionsForLogs(
 	queryParams map[string]string,
 ) *v3.QBFilterSuggestionsResponse {
 
-	_, dsQPExists := queryParams["dataSource"]
-	require.False(tb.t, dsQPExists)
+	_, dsExistsInQP := queryParams["dataSource"]
+	require.False(tb.t, dsExistsInQP)
 	queryParams["dataSource"] = "logs"
 
 	result := tb.QSGetRequest("/api/v3/filter_suggestions", queryParams)
@@ -257,7 +246,6 @@ func (tb *FilterSuggestionsTestBed) GetQBFilterSuggestionsForLogs(
 	}
 
 	return &resp
-
 }
 
 func NewFilterSuggestionsTestBed(t *testing.T) *FilterSuggestionsTestBed {
@@ -313,29 +301,9 @@ func (tb *FilterSuggestionsTestBed) QSGetRequest(
 		tb.t.Fatalf("couldn't create authenticated test request: %v", err)
 	}
 
-	respWriter := httptest.NewRecorder()
-	tb.qsHttpHandler.ServeHTTP(respWriter, req)
-	response := respWriter.Result()
-	responseBody, err := io.ReadAll(response.Body)
+	result, err := HandleTestRequest(tb.qsHttpHandler, req, 200)
 	if err != nil {
-		tb.t.Fatalf("couldn't read response body received from QS: %v", err)
+		tb.t.Fatalf("test request failed: %v", err)
 	}
-
-	if response.StatusCode != 200 {
-		tb.t.Fatalf(
-			"unexpected response status from query service for path %s. status: %d, body: %v\n%v",
-			path, response.StatusCode, string(responseBody), string(debug.Stack()),
-		)
-	}
-
-	var result app.ApiResponse
-	err = json.Unmarshal(responseBody, &result)
-	if err != nil {
-		tb.t.Fatalf(
-			"Could not unmarshal QS response into an ApiResponse.\nResponse body: %s",
-			string(responseBody),
-		)
-	}
-
-	return &result
+	return result
 }
diff --git a/pkg/query-service/tests/integration/signoz_integrations_test.go b/pkg/query-service/tests/integration/signoz_integrations_test.go
@@ -3,10 +3,7 @@ package tests
 import (
 	"encoding/json"
 	"fmt"
-	"io"
 	"net/http"
-	"net/http/httptest"
-	"runtime/debug"
 	"slices"
 	"testing"
 	"time"
@@ -508,31 +505,11 @@ func (tb *IntegrationsTestBed) RequestQS(
 		tb.t.Fatalf("couldn't create authenticated test request: %v", err)
 	}
 
-	respWriter := httptest.NewRecorder()
-	tb.qsHttpHandler.ServeHTTP(respWriter, req)
-	response := respWriter.Result()
-	responseBody, err := io.ReadAll(response.Body)
+	result, err := HandleTestRequest(tb.qsHttpHandler, req, 200)
 	if err != nil {
-		tb.t.Fatalf("couldn't read response body received from QS: %v", err)
+		tb.t.Fatalf("test request failed: %v", err)
 	}
-
-	if response.StatusCode != 200 {
-		tb.t.Fatalf(
-			"unexpected response status from query service for path %s. status: %d, body: %v\n%v",
-			path, response.StatusCode, string(responseBody), string(debug.Stack()),
-		)
-	}
-
-	var result app.ApiResponse
-	err = json.Unmarshal(responseBody, &result)
-	if err != nil {
-		tb.t.Fatalf(
-			"Could not unmarshal QS response into an ApiResponse.\nResponse body: %s",
-			string(responseBody),
-		)
-	}
-
-	return &result
+	return result
 }
 
 func (tb *IntegrationsTestBed) mockLogQueryResponse(logsInResponse []model.SignozLog) {

diff --git a/pkg/query-service/tests/integration/test_utils.go b/pkg/query-service/tests/integration/test_utils.go
@@ -5,8 +5,10 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"io"
 	"net/http"
 	"net/http/httptest"
+	"runtime/debug"
 	"testing"
 	"time"
 
@@ -16,6 +18,7 @@ import (
 	"github.com/open-telemetry/opentelemetry-collector-contrib/pkg/stanza/entry"
 	mockhouse "github.com/srikanthccv/ClickHouse-go-mock"
 	"github.com/stretchr/testify/require"
+	"go.signoz.io/signoz/pkg/query-service/app"
 	"go.signoz.io/signoz/pkg/query-service/app/clickhouseReader"
 	"go.signoz.io/signoz/pkg/query-service/auth"
 	"go.signoz.io/signoz/pkg/query-service/constants"
@@ -198,3 +201,31 @@ func AuthenticatedRequestForTest(
 	req.Header.Add("Authorization", "Bearer "+userJwt.AccessJwt)
 	return req, nil
 }
+
+func HandleTestRequest(handler http.Handler, req *http.Request, expectedStatus int) (*app.ApiResponse, error) {
+	respWriter := httptest.NewRecorder()
+	handler.ServeHTTP(respWriter, req)
+	response := respWriter.Result()
+	responseBody, err := io.ReadAll(response.Body)
+	if err != nil {
+		return nil, fmt.Errorf("couldn't read response body received from QS: %w", err)
+	}
+
+	if response.StatusCode != expectedStatus {
+		return nil, fmt.Errorf(
+			"unexpected response status from query service for path %s. status: %d, body: %v\n%v",
+			req.URL.Path, response.StatusCode, string(responseBody), string(debug.Stack()),
+		)
+	}
+
+	var result app.ApiResponse
+	err = json.Unmarshal(responseBody, &result)
+	if err != nil {
+		return nil, fmt.Errorf(
+			"Could not unmarshal QS response into an ApiResponse.\nResponse body: %s",
+			string(responseBody),
+		)
+	}
+
+	return &result, nil
+}