From 58c8fdad7af4606ab00e4b4a1ff4fe3f788f670f Mon Sep 17 00:00:00 2001 From: Kyle Brandt Date: Tue, 26 Aug 2025 09:21:24 -0400 Subject: [PATCH] SSE/QuerySvc: (Chore) Remove expression parser code and feature toggle (#110117) --- .../src/types/featureToggles.gen.ts | 4 - pkg/expr/graph_test.go | 4 +- pkg/expr/nodes.go | 27 --- pkg/expr/reader.go | 197 ------------------ pkg/expr/reader_test.go | 162 -------------- pkg/services/featuremgmt/registry.go | 7 - pkg/services/featuremgmt/toggles_gen.csv | 1 - pkg/services/featuremgmt/toggles_gen.go | 4 - pkg/services/featuremgmt/toggles_gen.json | 3 +- 9 files changed, 3 insertions(+), 406 deletions(-) delete mode 100644 pkg/expr/reader.go delete mode 100644 pkg/expr/reader_test.go diff --git a/packages/grafana-data/src/types/featureToggles.gen.ts b/packages/grafana-data/src/types/featureToggles.gen.ts index 9ab694d9d58..3f3ccc676d7 100644 --- a/packages/grafana-data/src/types/featureToggles.gen.ts +++ b/packages/grafana-data/src/types/featureToggles.gen.ts @@ -494,10 +494,6 @@ export interface FeatureToggles { */ kubernetesAggregatorCapTokenAuth?: boolean; /** - * Enable new expression parser - */ - expressionParser?: boolean; - /** * Enable groupBy variable support in scenes dashboards */ groupByVariable?: boolean; diff --git a/pkg/expr/graph_test.go b/pkg/expr/graph_test.go index bcfee8fb306..1d1b11e63bb 100644 --- a/pkg/expr/graph_test.go +++ b/pkg/expr/graph_test.go @@ -7,7 +7,6 @@ import ( "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/services/datasources" - "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/setting" ) @@ -234,8 +233,7 @@ func TestServicebuildPipeLine(t *testing.T) { }, } s := Service{ - features: featuremgmt.WithFeatures(featuremgmt.FlagExpressionParser), - cfg: setting.NewCfg(), + cfg: setting.NewCfg(), } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/expr/nodes.go b/pkg/expr/nodes.go index 2ea0e36c40e..8e65ff9858b 100644 --- a/pkg/expr/nodes.go +++ b/pkg/expr/nodes.go @@ -9,8 +9,6 @@ import ( "time" "github.com/grafana/grafana-plugin-sdk-go/backend" - "github.com/grafana/grafana-plugin-sdk-go/data/utils/jsoniter" - data "github.com/grafana/grafana-plugin-sdk-go/experimental/apis/data/v0alpha1" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/codes" "gonum.org/v1/gonum/graph/simple" @@ -139,31 +137,6 @@ func buildCMDNode(ctx context.Context, rn *rawNode, toggles featuremgmt.FeatureT CMDType: commandType, } - if toggles.IsEnabledGlobally(featuremgmt.FlagExpressionParser) { - rn.QueryType, err = getExpressionCommandTypeString(rn.Query) - if err != nil { - return nil, err // should not happen because the command was parsed first thing - } - - // NOTE: this structure of this is weird now, because it is targeting a structure - // where this is actually run in the root loop, however we want to verify the individual - // node parsing before changing the full tree parser - reader := NewExpressionQueryReader(toggles) - iter, err := jsoniter.ParseBytes(jsoniter.ConfigDefault, rn.QueryRaw) - if err != nil { - return nil, err - } - q, err := reader.ReadQuery(ctx, data.NewDataQuery(map[string]any{ - "refId": rn.RefID, - "type": rn.QueryType, - }), iter) - if err != nil { - return nil, err - } - node.Command = q.Command - return node, err - } - switch commandType { case TypeMath: node.Command, err = UnmarshalMathCommand(rn) diff --git a/pkg/expr/reader.go b/pkg/expr/reader.go deleted file mode 100644 index ddabbf3cb04..00000000000 --- a/pkg/expr/reader.go +++ /dev/null @@ -1,197 +0,0 @@ -package expr - -import ( - "context" - "fmt" - "strings" - - "github.com/grafana/grafana-plugin-sdk-go/backend" - "github.com/grafana/grafana-plugin-sdk-go/backend/gtime" - "github.com/grafana/grafana-plugin-sdk-go/data/utils/jsoniter" - data "github.com/grafana/grafana-plugin-sdk-go/experimental/apis/data/v0alpha1" - - "github.com/grafana/grafana/pkg/expr/classic" - "github.com/grafana/grafana/pkg/expr/mathexp" - "github.com/grafana/grafana/pkg/services/featuremgmt" -) - -// Once we are comfortable with the parsing logic, this struct will -// be merged/replace the existing Query struct in grafana/pkg/expr/transform.go -type ExpressionQuery struct { - GraphID int64 `json:"id,omitempty"` - RefID string `json:"refId"` - QueryType QueryType `json:"type"` - - // The typed query parameters - Properties any `json:"properties"` - - // Hidden in debug JSON - Command Command `json:"-"` -} - -// ID is used to identify nodes in the directed graph -func (q ExpressionQuery) ID() int64 { - return q.GraphID -} - -type ExpressionQueryReader struct { - features featuremgmt.FeatureToggles -} - -func NewExpressionQueryReader(features featuremgmt.FeatureToggles) *ExpressionQueryReader { - return &ExpressionQueryReader{ - features: features, - } -} - -// nolint:gocyclo -func (h *ExpressionQueryReader) ReadQuery( - ctx context.Context, - // Properties that have been parsed off the same node - common data.DataQuery, - // An iterator with context for the full node (include common values) - iter *jsoniter.Iterator, -) (eq ExpressionQuery, err error) { - referenceVar := "" - eq.RefID = common.RefID - eq.QueryType = QueryType(common.GetString("type")) - if eq.QueryType == "" { - return eq, fmt.Errorf("missing type") - } - switch eq.QueryType { - case QueryTypeMath: - q := &MathQuery{} - err = iter.ReadVal(q) - if err == nil { - eq.Command, err = NewMathCommand(common.RefID, q.Expression) - eq.Properties = q - } - - case QueryTypeReduce: - var mapper mathexp.ReduceMapper = nil - q := &ReduceQuery{} - err = iter.ReadVal(q) - if err == nil { - referenceVar, err = getReferenceVar(q.Expression, common.RefID) - eq.Properties = q - } - if err == nil && q.Settings != nil { - switch q.Settings.Mode { - case ReduceModeStrict: - mapper = nil - case ReduceModeDrop: - mapper = mathexp.DropNonNumber{} - case ReduceModeReplace: - if q.Settings.ReplaceWithValue == nil { - err = fmt.Errorf("setting replaceWithValue must be specified when mode is '%s'", q.Settings.Mode) - } - mapper = mathexp.ReplaceNonNumberWithValue{Value: *q.Settings.ReplaceWithValue} - default: - err = fmt.Errorf("unsupported reduce mode") - } - } - if err == nil { - eq.Properties = q - eq.Command, err = NewReduceCommand(common.RefID, - q.Reducer, referenceVar, mapper) - } - - case QueryTypeResample: - q := &ResampleQuery{} - err = iter.ReadVal(q) - if err == nil && common.TimeRange == nil { - err = fmt.Errorf("missing time range in query") - } - if err == nil { - referenceVar, err = getReferenceVar(q.Expression, common.RefID) - } - if err == nil { - tr := gtime.NewTimeRange(common.TimeRange.From, common.TimeRange.To) - eq.Properties = q - eq.Command, err = NewResampleCommand(common.RefID, - q.Window, - referenceVar, - q.Downsampler, - q.Upsampler, - AbsoluteTimeRange{ - From: tr.GetFromAsTimeUTC(), - To: tr.GetToAsTimeUTC(), - }, - ) - } - - case QueryTypeClassic: - q := &ClassicQuery{} - err = iter.ReadVal(q) - if err == nil { - eq.Properties = q - eq.Command, err = classic.NewConditionCmd(common.RefID, q.Conditions) - } - - case QueryTypeSQL: - if !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) { - return eq, fmt.Errorf("sql expressions are disabled") - } - q := &SQLExpression{} - err = iter.ReadVal(q) - if err == nil { - eq.Properties = q - // TODO: Cascade limit from Grafana config in this (new Expression Parser) branch of the code - cellLimit := 0 // zero means no limit - sqlLogger := backend.NewLoggerWith("logger", SQLLoggerName).FromContext(ctx) - eq.Command, err = NewSQLCommand(ctx, sqlLogger, common.RefID, q.Format, q.Expression, int64(cellLimit), 0, 0) - } - - case QueryTypeThreshold: - q := &ThresholdQuery{} - err = iter.ReadVal(q) - if err == nil { - referenceVar, err = getReferenceVar(q.Expression, common.RefID) - } - if err == nil { - // we only support one condition for now, we might want to turn this in to "OR" expressions later - if len(q.Conditions) != 1 { - return eq, fmt.Errorf("threshold expression requires exactly one condition") - } - firstCondition := q.Conditions[0] - - threshold, err := NewThresholdCommand(common.RefID, referenceVar, firstCondition.Evaluator.Type, firstCondition.Evaluator.Params) - if err != nil { - return eq, fmt.Errorf("invalid condition: %w", err) - } - eq.Command = threshold - eq.Properties = q - - if firstCondition.UnloadEvaluator != nil { - unloading, err := NewThresholdCommand(common.RefID, referenceVar, firstCondition.UnloadEvaluator.Type, firstCondition.UnloadEvaluator.Params) - unloading.Invert = true - if err != nil { - return eq, fmt.Errorf("invalid unloadCondition: %w", err) - } - var d Fingerprints - if firstCondition.LoadedDimensions != nil { - d, err = FingerprintsFromFrame(firstCondition.LoadedDimensions) - if err != nil { - return eq, fmt.Errorf("failed to parse loaded dimensions: %w", err) - } - } - eq.Command, err = NewHysteresisCommand(common.RefID, referenceVar, *threshold, *unloading, d) - if err != nil { - return eq, err - } - } - } - - default: - err = fmt.Errorf("unknown query type (%s)", common.QueryType) - } - return eq, err -} - -func getReferenceVar(exp string, refId string) (string, error) { - exp = strings.TrimPrefix(exp, "$") - if exp == "" { - return "", fmt.Errorf("no variable specified to reference for refId %v", refId) - } - return exp, nil -} diff --git a/pkg/expr/reader_test.go b/pkg/expr/reader_test.go deleted file mode 100644 index 39b99a21ac0..00000000000 --- a/pkg/expr/reader_test.go +++ /dev/null @@ -1,162 +0,0 @@ -package expr - -import ( - "encoding/json" - "reflect" - "testing" - - "github.com/grafana/grafana-plugin-sdk-go/data/utils/jsoniter" - data "github.com/grafana/grafana-plugin-sdk-go/experimental/apis/data/v0alpha1" - "github.com/grafana/grafana/pkg/expr/mathexp" - "github.com/grafana/grafana/pkg/services/featuremgmt" - "github.com/stretchr/testify/require" -) - -func TestReaderReduceMode(t *testing.T) { - testData := []struct { - name string - bytes []byte - expectError bool - hasMapper bool - mapperType reflect.Type - }{ - { - name: "no_settings", - bytes: []byte(` - { - "refId": "B", - "datasource": { - "type": "__expr__", - "uid": "__expr__" - }, - "reducer": "last", - "expression": "A", - "window": "", - "type": "reduce" - } - `), - expectError: false, - hasMapper: false, - }, - { - name: "mode_dropnn", - bytes: []byte(` - { - "refId": "B", - "datasource": { - "type": "__expr__", - "uid": "__expr__" - }, - "reducer": "last", - "expression": "A", - "window": "", - "settings": { - "mode": "dropNN" - }, - "type": "reduce" - } - `), - expectError: false, - hasMapper: true, - mapperType: reflect.TypeOf(mathexp.DropNonNumber{}), - }, - { - name: "mode_replacenn", - bytes: []byte(` - { - "refId": "B", - "datasource": { - "type": "__expr__", - "uid": "__expr__" - }, - "reducer": "last", - "expression": "A", - "window": "", - "settings": { - "mode": "replaceNN", - "replaceWithValue": 42 - }, - "type": "reduce" - } - `), - expectError: false, - hasMapper: true, - mapperType: reflect.TypeOf(mathexp.ReplaceNonNumberWithValue{}), - }, - { - name: "mode_strict", - bytes: []byte(` - { - "refId": "B", - "datasource": { - "type": "__expr__", - "uid": "__expr__" - }, - "reducer": "last", - "expression": "A", - "window": "", - "settings": { - "mode": "" - }, - "type": "reduce" - } - `), - expectError: false, - hasMapper: false, - }, - { - name: "mode_invalid", - bytes: []byte(` - { - "refId": "B", - "datasource": { - "type": "__expr__", - "uid": "__expr__" - }, - "reducer": "last", - "expression": "A", - "window": "", - "settings": { - "mode": "invalid-mode" - }, - "type": "reduce" - } - `), - expectError: true, - }, - } - - for _, test := range testData { - t.Run("TestReduceReader:"+test.name, func(t *testing.T) { - var q data.DataQuery - - err := json.Unmarshal(test.bytes, &q) - require.NoError(t, err) - - raw, err := json.Marshal(q) - require.NoError(t, err) - - iter, err := jsoniter.ParseBytes(jsoniter.ConfigDefault, raw) - require.NoError(t, err) - - reader := NewExpressionQueryReader(featuremgmt.WithFeatures()) - - eq, err := reader.ReadQuery(t.Context(), q, iter) - - if test.expectError { - require.Error(t, err) - } else { - require.NoError(t, err) - rc, ok := eq.Command.(*ReduceCommand) - require.True(t, ok) - - if test.hasMapper { - require.NotNil(t, rc.seriesMapper) - require.Equal(t, test.mapperType, reflect.TypeOf(rc.seriesMapper)) - } else { - require.Nil(t, rc.seriesMapper) - } - } - }) - } -} diff --git a/pkg/services/featuremgmt/registry.go b/pkg/services/featuremgmt/registry.go index 7807eaee089..a9dd743f8cc 100644 --- a/pkg/services/featuremgmt/registry.go +++ b/pkg/services/featuremgmt/registry.go @@ -833,13 +833,6 @@ var ( Owner: grafanaAppPlatformSquad, RequiresRestart: true, }, - { - Name: "expressionParser", - Description: "Enable new expression parser", - Stage: FeatureStageExperimental, - Owner: grafanaAppPlatformSquad, - RequiresRestart: true, - }, { Name: "groupByVariable", Description: "Enable groupBy variable support in scenes dashboards", diff --git a/pkg/services/featuremgmt/toggles_gen.csv b/pkg/services/featuremgmt/toggles_gen.csv index a59feb73671..db6b999667a 100644 --- a/pkg/services/featuremgmt/toggles_gen.csv +++ b/pkg/services/featuremgmt/toggles_gen.csv @@ -109,7 +109,6 @@ newPDFRendering,GA,@grafana/grafana-operator-experience-squad,false,false,false tlsMemcached,GA,@grafana/grafana-operator-experience-squad,false,false,false kubernetesAggregator,experimental,@grafana/grafana-app-platform-squad,false,true,false kubernetesAggregatorCapTokenAuth,experimental,@grafana/grafana-app-platform-squad,false,true,false -expressionParser,experimental,@grafana/grafana-app-platform-squad,false,true,false groupByVariable,experimental,@grafana/dashboards-squad,false,false,false scopeFilters,experimental,@grafana/dashboards-squad,false,false,false oauthRequireSubClaim,experimental,@grafana/identity-access-team,false,false,false diff --git a/pkg/services/featuremgmt/toggles_gen.go b/pkg/services/featuremgmt/toggles_gen.go index da56b91fa61..c6b40779158 100644 --- a/pkg/services/featuremgmt/toggles_gen.go +++ b/pkg/services/featuremgmt/toggles_gen.go @@ -447,10 +447,6 @@ const ( // Enable CAP token based authentication in grafana's embedded kube-aggregator FlagKubernetesAggregatorCapTokenAuth = "kubernetesAggregatorCapTokenAuth" - // FlagExpressionParser - // Enable new expression parser - FlagExpressionParser = "expressionParser" - // FlagGroupByVariable // Enable groupBy variable support in scenes dashboards FlagGroupByVariable = "groupByVariable" diff --git a/pkg/services/featuremgmt/toggles_gen.json b/pkg/services/featuremgmt/toggles_gen.json index 367fcdf165a..26caa2e068e 100644 --- a/pkg/services/featuremgmt/toggles_gen.json +++ b/pkg/services/featuremgmt/toggles_gen.json @@ -1259,7 +1259,8 @@ "metadata": { "name": "expressionParser", "resourceVersion": "1753448760331", - "creationTimestamp": "2024-02-17T00:59:11Z" + "creationTimestamp": "2024-02-17T00:59:11Z", + "deletionTimestamp": "2025-08-25T16:14:10Z" }, "spec": { "description": "Enable new expression parser",