Merge branch 'main' into tvats-improve-error-msg

pkg/errors/errors.go

@@ -112,7 +112,7 @@ func (b *base) WithUrl(u string) *base {
 	}
 }
 
-// WithUrl adds additional messages to the base error and returns a new base error.
+// WithAdditional adds additional messages to the base error and returns a new base error.
 func (b *base) WithAdditional(a ...string) *base {
 	return &base{
 		t: b.t,

pkg/errors/errors_test.go

@@ -51,3 +51,88 @@ func TestUnwrapb(t *testing.T) {
 	atyp, _, _, _, _, _ = Unwrapb(oerr)
 	assert.Equal(t, TypeInternal, atyp)
 }
+
+func TestWithAdditionalf(t *testing.T) {
+	t.Run("adds additional message to base error", func(t *testing.T) {
+		typ := typ{"test-error"}
+		baseErr := New(typ, MustNewCode("test_code"), "primary message")
+
+		result := WithAdditionalf(baseErr, "additional context %d", 456)
+
+		assert.NotNil(t, result)
+		_, _, msg, _, _, additional := Unwrapb(result)
+		assert.Equal(t, "primary message", msg, "primary message should not change")
+		assert.Equal(t, []string{"additional context 456"}, additional)
+	})
+
+	t.Run("adds additional message to non-base error", func(t *testing.T) {
+		stdErr := errors.New("some error")
+
+		result := WithAdditionalf(stdErr, "extra info: %s", "details")
+
+		assert.NotNil(t, result)
+		_, _, _, _, _, additional := Unwrapb(result)
+		assert.Equal(t, []string{"extra info: details"}, additional)
+	})
+
+	t.Run("appends to existing additional messages", func(t *testing.T) {
+		typ := typ{"test-error"}
+		baseErr := New(typ, MustNewCode("test_code"), "message").
+			WithAdditional("first additional", "second additional")
+
+		result := WithAdditionalf(baseErr, "third additional %s", "msg")
+
+		_, _, _, _, _, additional := Unwrapb(result)
+		assert.Equal(t, []string{
+			"first additional",
+			"second additional",
+			"third additional msg",
+		}, additional)
+	})
+}
+
+func TestWithUrl(t *testing.T) {
+	t.Run("adds url to base error", func(t *testing.T) {
+		typ := typ{"test-error"}
+		baseErr := New(typ, MustNewCode("test_code"), "error message")
+
+		result := baseErr.WithUrl("https://docs.signoz.io/errors")
+
+		_, _, _, _, url, _ := Unwrapb(result)
+		assert.Equal(t, "https://docs.signoz.io/errors", url)
+	})
+
+	t.Run("replaces existing url", func(t *testing.T) {
+		typ := typ{"test-error"}
+		baseErr := New(typ, MustNewCode("test_code"), "error message").
+			WithUrl("https://old-url.com")
+
+		result := baseErr.WithUrl("https://new-url.com")
+
+		_, _, _, _, url, _ := Unwrapb(result)
+		assert.Equal(t, "https://new-url.com", url)
+	})
+}
+
+func TestWithAdditional(t *testing.T) {
+	t.Run("adds additional messages to base error", func(t *testing.T) {
+		typ := typ{"test-error"}
+		baseErr := New(typ, MustNewCode("test_code"), "main message")
+
+		result := baseErr.WithAdditional("hint 1", "hint 2", "hint 3")
+
+		_, _, _, _, _, additional := Unwrapb(result)
+		assert.Equal(t, []string{"hint 1", "hint 2", "hint 3"}, additional)
+	})
+
+	t.Run("replaces existing additional messages", func(t *testing.T) {
+		typ := typ{"test-error"}
+		baseErr := New(typ, MustNewCode("test_code"), "message").
+			WithAdditional("old hint")
+
+		result := baseErr.WithAdditional("new hint 1", "new hint 2")
+
+		_, _, _, _, _, additional := Unwrapb(result)
+		assert.Equal(t, []string{"new hint 1", "new hint 2"}, additional)
+	})
+}

pkg/querybuilder/where_clause_visitor.go

@@ -864,17 +864,23 @@ func (v *filterExpressionVisitor) VisitKey(ctx *grammar.KeyContext) any {
 		} else if !v.ignoreNotFoundKeys {
 			// TODO(srikanthccv): do we want to return an error here?
 			// should we infer the type and auto-magically build a key for expression?
-			v.errors = append(v.errors, fmt.Sprintf("key `%s` not found", fieldKey.Name))
+			v.errors = append(v.errors, fmt.Sprintf("key `%s` is not a valid field, consider removing it from filter query", fieldKey.Name))
 			v.mainErrorURL = "https://signoz.io/docs/userguide/search-troubleshooting/#key-fieldname-not-found"
 		}
 	}
 
 	if len(fieldKeysForName) > 1 {
+
+		keys := []string{}
+		for _, item := range fieldKeysForName {
+			keys = append(keys, fmt.Sprintf("%s.%s:%s", item.FieldContext.StringValue(), item.Name, item.FieldDataType.StringValue()))
+		}
 		warnMsg := fmt.Sprintf(
-			"Key `%s` is ambiguous, found %d different combinations of field context / data type: %v.",
+			"Key `%s` is ambiguous, found %d different combinations of field context / data type. "+
+				"Please specify one from these [ %s ] to disambiguate.",
 			fieldKey.Name,
 			len(fieldKeysForName),
-			fieldKeysForName,
+			strings.Join(keys, ", "),
 		)
 		mixedFieldContext := map[string]bool{}
 		for _, item := range fieldKeysForName {

pkg/telemetrylogs/filter_expr_logs_test.go

@@ -716,7 +716,7 @@ func TestFilterExprLogs(t *testing.T) {
 			shouldPass:            false,
 			expectedQuery:         "",
 			expectedArgs:          []any{},
-			expectedErrorContains: "key `greater` not found",
+			expectedErrorContains: "key `greater` is not a valid field, consider removing it from filter query",
 		},
 		{
 			category:              "Key-operator-value boundary",
@@ -732,7 +732,7 @@ func TestFilterExprLogs(t *testing.T) {
 			shouldPass:            false,
 			expectedQuery:         "",
 			expectedArgs:          []any{},
-			expectedErrorContains: "key `less` not found",
+			expectedErrorContains: "key `less` is not a valid field, consider removing it from filter query",
 		},
 		{
 			category:              "Key-operator-value boundary",
@@ -788,7 +788,7 @@ func TestFilterExprLogs(t *testing.T) {
 			shouldPass:            false,
 			expectedQuery:         "",
 			expectedArgs:          []any{},
-			expectedErrorContains: "key `user` not found",
+			expectedErrorContains: "key `user` is not a valid field, consider removing it from filter query",
 		},
 		{
 			category:              "Key-operator-value boundary",
@@ -1999,7 +1999,7 @@ func TestFilterExprLogs(t *testing.T) {
 			shouldPass:            false,
 			expectedQuery:         "",
 			expectedArgs:          nil,
-			expectedErrorContains: "key `response.body.data.items[].id` not found",
+			expectedErrorContains: "key `response.body.data.items[].id` is not a valid field, consider removing it from filter query",
 		},
 		{
 			category:              "Nested object paths",
@@ -2236,7 +2236,7 @@ func TestFilterExprLogs(t *testing.T) {
 			shouldPass:            false,
 			expectedQuery:         "",
 			expectedArgs:          nil,
-			expectedErrorContains: "key `user_id` not found",
+			expectedErrorContains: "key `user_id` is not a valid field, consider removing it from filter query",
 		},
 
 		// More common filter patterns
@@ -2387,7 +2387,7 @@ func TestFilterExprLogs(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(fmt.Sprintf("%s: %s", tc.category, limitString(tc.query, 50)), func(t *testing.T) {
 
-            clause, err := querybuilder.PrepareWhereClause(tc.query, opts, 0, 0)
+			clause, err := querybuilder.PrepareWhereClause(tc.query, opts, 0, 0)
 
 			if tc.shouldPass {
 				if err != nil {
@@ -2506,7 +2506,7 @@ func TestFilterExprLogsConflictNegation(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(fmt.Sprintf("%s: %s", tc.category, limitString(tc.query, 50)), func(t *testing.T) {
 
-            clause, err := querybuilder.PrepareWhereClause(tc.query, opts, 0, 0)
+			clause, err := querybuilder.PrepareWhereClause(tc.query, opts, 0, 0)
 
 			if tc.shouldPass {
 				if err != nil {

pkg/telemetrylogs/statement_builder.go

@@ -249,7 +249,7 @@ func (b *logQueryStatementBuilder) buildListQuery(
 			// get column expression for the field - use array index directly to avoid pointer to loop variable
 			colExpr, err := b.fm.ColumnExpressionFor(ctx, &query.SelectFields[index], keys)
 			if err != nil {
-				return nil, err
+				return nil, errors.WithAdditionalf(err, "Consider removing field %s from columns by clicking options and then removing the column", query.SelectFields[index].Name)
 			}
 			sb.SelectMore(colExpr)
 		}
@@ -269,7 +269,7 @@ func (b *logQueryStatementBuilder) buildListQuery(
 	for _, orderBy := range query.Order {
 		colExpr, err := b.fm.ColumnExpressionFor(ctx, &orderBy.Key.TelemetryFieldKey, keys)
 		if err != nil {
-			return nil, err
+			return nil, errors.WithAdditionalf(err, "Consider removing field %s and choosing a different column in 'Order by' drop down menu", orderBy.Key.TelemetryFieldKey.Name)
 		}
 		sb.OrderBy(fmt.Sprintf("%s %s", colExpr, orderBy.Direction.StringValue()))
 	}
@@ -592,7 +592,7 @@ func (b *logQueryStatementBuilder) addFilterCondition(
 			JsonBodyPrefix:     b.jsonBodyPrefix,
 			JsonKeyToKey:       b.jsonKeyToKey,
 			Variables:          variables,
-        }, start, end)
+		}, start, end)
 
 		if err != nil {
 			return nil, err

pkg/telemetrymeter/statement_builder.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"log/slog"
 
+	"github.com/SigNoz/signoz/pkg/errors"
 	"github.com/SigNoz/signoz/pkg/factory"
 	"github.com/SigNoz/signoz/pkg/querybuilder"
 	"github.com/SigNoz/signoz/pkg/telemetrymetrics"
@@ -122,7 +123,7 @@ func (b *meterQueryStatementBuilder) buildTemporalAggDeltaFastPath(
 	for _, g := range query.GroupBy {
 		col, err := b.fm.ColumnExpressionFor(ctx, &g.TelemetryFieldKey, keys)
 		if err != nil {
-			return "", []any{}, err
+			return "", []any{}, errors.WithAdditionalf(err, "Consider removing field %s from 'AGGREGATE ACROSS TIME SERIES by' options", g.TelemetryFieldKey.Name)
 		}
 		sb.SelectMore(col)
 	}
@@ -148,7 +149,7 @@ func (b *meterQueryStatementBuilder) buildTemporalAggDeltaFastPath(
 			FieldKeys:        keys,
 			FullTextColumn:   &telemetrytypes.TelemetryFieldKey{Name: "labels"},
 			Variables:        variables,
-        }, start, end)
+		}, start, end)
 		if err != nil {
 			return "", []any{}, err
 		}
@@ -202,7 +203,7 @@ func (b *meterQueryStatementBuilder) buildTemporalAggDelta(
 	for _, g := range query.GroupBy {
 		col, err := b.fm.ColumnExpressionFor(ctx, &g.TelemetryFieldKey, keys)
 		if err != nil {
-			return "", nil, err
+			return "", []any{}, errors.WithAdditionalf(err, "Consider removing field %s from 'AGGREGATE ACROSS TIME SERIES by' options", g.TelemetryFieldKey.Name)
 		}
 		sb.SelectMore(col)
 	}
@@ -231,7 +232,7 @@ func (b *meterQueryStatementBuilder) buildTemporalAggDelta(
 			FieldKeys:        keys,
 			FullTextColumn:   &telemetrytypes.TelemetryFieldKey{Name: "labels"},
 			Variables:        variables,
-        }, start, end)
+		}, start, end)
 		if err != nil {
 			return "", nil, err
 		}
@@ -272,7 +273,7 @@ func (b *meterQueryStatementBuilder) buildTemporalAggCumulativeOrUnspecified(
 	for _, g := range query.GroupBy {
 		col, err := b.fm.ColumnExpressionFor(ctx, &g.TelemetryFieldKey, keys)
 		if err != nil {
-			return "", nil, err
+			return "", []any{}, errors.WithAdditionalf(err, "Consider removing field %s from 'AGGREGATE ACROSS TIME SERIES by' options", g.TelemetryFieldKey.Name)
 		}
 		baseSb.SelectMore(col)
 	}
@@ -295,7 +296,7 @@ func (b *meterQueryStatementBuilder) buildTemporalAggCumulativeOrUnspecified(
 			FieldKeys:        keys,
 			FullTextColumn:   &telemetrytypes.TelemetryFieldKey{Name: "labels"},
 			Variables:        variables,
-        }, start, end)
+		}, start, end)
 		if err != nil {
 			return "", nil, err
 		}

pkg/telemetrymetrics/statement_builder.go

@@ -6,6 +6,7 @@ import (
 	"log/slog"
 	"os"
 
+	"github.com/SigNoz/signoz/pkg/errors"
 	"github.com/SigNoz/signoz/pkg/factory"
 	"github.com/SigNoz/signoz/pkg/querybuilder"
 	"github.com/SigNoz/signoz/pkg/types/metrictypes"
@@ -361,7 +362,7 @@ func (b *MetricQueryStatementBuilder) buildTimeSeriesCTE(
 	for _, g := range query.GroupBy {
 		col, err := b.fm.ColumnExpressionFor(ctx, &g.TelemetryFieldKey, keys)
 		if err != nil {
-			return "", nil, err
+			return "", nil, errors.WithAdditionalf(err, "Consider removing field %s from 'AGGREGATE ACROSS TIME SERIES by' options", g.TelemetryFieldKey.Name)
 		}
 		sb.SelectMore(col)
 	}

pkg/telemetrytraces/statement_builder.go

@@ -295,7 +295,7 @@ func (b *traceQueryStatementBuilder) buildListQuery(
 	for _, field := range selectedFields {
 		colExpr, err := b.fm.ColumnExpressionFor(ctx, &field, keys)
 		if err != nil {
-			return nil, err
+			return nil, errors.WithAdditionalf(err, "Consider removing field %s by clicking Options and then removing the field", field.Name)
 		}
 		sb.SelectMore(colExpr)
 	}
@@ -313,7 +313,7 @@ func (b *traceQueryStatementBuilder) buildListQuery(
 	for _, orderBy := range query.Order {
 		colExpr, err := b.fm.ColumnExpressionFor(ctx, &orderBy.Key.TelemetryFieldKey, keys)
 		if err != nil {
-			return nil, err
+			return nil, errors.WithAdditionalf(err, "Consider removing field %s and choosing a different field in 'Order by' drop down menu", orderBy.Key.TelemetryFieldKey.Name)
 		}
 		sb.OrderBy(fmt.Sprintf("%s %s", colExpr, orderBy.Direction.StringValue()))
 	}

pkg/telemetrytraces/trace_operator_cte_builder.go

@@ -467,7 +467,7 @@ func (b *traceOperatorCTEBuilder) buildListQuery(ctx context.Context, selectFrom
 	for _, orderBy := range b.operator.Order {
 		colExpr, err := b.stmtBuilder.fm.ColumnExpressionFor(ctx, &orderBy.Key.TelemetryFieldKey, keys)
 		if err != nil {
-			return nil, err
+			return nil, errors.WithAdditionalf(err, "Consider removing field %s and choosing a different column in 'Order by' drop down menu", orderBy.Key.TelemetryFieldKey.Name)
 		}
 		sb.OrderBy(fmt.Sprintf("%s %s", colExpr, orderBy.Direction.StringValue()))
 		orderApplied = true