From 457a1dc108df679376249bd9389c53c549071150 Mon Sep 17 00:00:00 2001 From: Elom Gomez Date: Tue, 25 Aug 2026 20:04:36 -0500 Subject: [PATCH] Forward opaque metrics selectors to the API Avoid rejecting valid query and workflow identifiers based on incomplete local assumptions or a single page of workflows. Co-authored-by: Cursor --- internal/cmd/metrics/metrics_test.go | 64 ++++++++++++++-------------- internal/cmd/metrics/show.go | 5 +-- internal/cmd/metrics/specialized.go | 41 +----------------- 3 files changed, 33 insertions(+), 77 deletions(-) diff --git a/internal/cmd/metrics/metrics_test.go b/internal/cmd/metrics/metrics_test.go index e8bc0a34..f5b2be58 100644 --- a/internal/cmd/metrics/metrics_test.go +++ b/internal/cmd/metrics/metrics_test.go @@ -75,6 +75,22 @@ func TestShowCmd_JSONPreservesSeries(t *testing.T) { c.Assert(response.Series[0].Points, qt.HasLen, 3) } +func TestShowCmd_ForwardsOpaqueQueryID(t *testing.T) { + c := qt.New(t) + service := &mock.MetricsService{ + GetSeriesFn: func(ctx context.Context, req *ps.GetMetricSeriesRequest) (*ps.MetricSeries, error) { + c.Assert(req.QueryIDs, qt.DeepEquals, []string{"59801dae501c"}) + return sampleSeries(), nil + }, + } + + var buf bytes.Buffer + cmd := ShowCmd(metricsTestHelper(&buf, printer.JSON, &ps.Client{Metrics: service})) + cmd.SetArgs([]string{"mydb", "main", "--metric", "queries", "--query-id", "59801dae501c"}) + c.Assert(cmd.Execute(), qt.IsNil) + c.Assert(service.GetSeriesFnInvoked, qt.IsTrue) +} + func TestShowCmd_HumanSummarizesSeries(t *testing.T) { c := qt.New(t) service := &mock.MetricsService{ @@ -317,16 +333,6 @@ func TestStorageMetricsCommands_PreserveResponse(t *testing.T) { func TestTabletsCmd_ForwardsSupportedFilters(t *testing.T) { c := qt.New(t) - workflows := &mock.WorkflowsService{ - ListFn: func(ctx context.Context, req *ps.ListWorkflowsRequest) ([]*ps.Workflow, error) { - c.Assert(req.Database, qt.Equals, "mydb") - return []*ps.Workflow{{ - ID: "opaque-workflow-id", - Name: "move-tables", - Branch: ps.DatabaseBranch{Name: "main"}, - }}, nil - }, - } service := &mock.MetricsService{ GetTabletSeriesFn: func(ctx context.Context, req *ps.GetTabletMetricSeriesRequest) (*ps.MetricSeries, error) { c.Assert(req.Metrics, qt.DeepEquals, []string{"replication_lag", "pod_cpu_usage", "vreplication_lag"}) @@ -342,7 +348,7 @@ func TestTabletsCmd_ForwardsSupportedFilters(t *testing.T) { } var buf bytes.Buffer - cmd := TabletsCmd(metricsTestHelper(&buf, printer.JSON, &ps.Client{Metrics: service, Workflows: workflows})) + cmd := TabletsCmd(metricsTestHelper(&buf, printer.JSON, &ps.Client{Metrics: service})) cmd.SetArgs([]string{ "mydb", "main", "--metric", "replication_lag,pod_cpu_usage,vreplication_lag", @@ -356,7 +362,6 @@ func TestTabletsCmd_ForwardsSupportedFilters(t *testing.T) { }) c.Assert(cmd.Execute(), qt.IsNil) c.Assert(service.GetTabletSeriesFnInvoked, qt.IsTrue) - c.Assert(workflows.ListFnInvoked, qt.IsTrue) } func TestTabletsInstantCmd_UsesNestedUX(t *testing.T) { @@ -382,31 +387,25 @@ func TestTabletsInstantCmd_UsesNestedUX(t *testing.T) { c.Assert(service.GetInstantTabletsFnInvoked, qt.IsTrue) } -func TestTabletsCmd_RejectsUnknownWorkflow(t *testing.T) { +func TestTabletsCmd_DoesNotPrevalidateWorkflow(t *testing.T) { c := qt.New(t) - workflows := &mock.WorkflowsService{ - ListFn: func(context.Context, *ps.ListWorkflowsRequest) ([]*ps.Workflow, error) { - return []*ps.Workflow{}, nil - }, - } service := &mock.MetricsService{ - GetTabletSeriesFn: func(context.Context, *ps.GetTabletMetricSeriesRequest) (*ps.MetricSeries, error) { - c.Fatal("Metrics.GetTabletSeries should not be called") - return nil, nil + GetTabletSeriesFn: func(_ context.Context, req *ps.GetTabletMetricSeriesRequest) (*ps.MetricSeries, error) { + c.Assert(req.Workflow, qt.Equals, "workflow-on-a-later-page") + return sampleSeries(), nil }, } cmd := TabletsCmd(metricsTestHelper(&bytes.Buffer{}, printer.JSON, &ps.Client{ - Metrics: service, - Workflows: workflows, + Metrics: service, })) cmd.SetArgs([]string{ "mydb", "main", "--metric", "vreplication_lag", - "--workflow", "missing", + "--workflow", "workflow-on-a-later-page", }) - c.Assert(cmd.Execute(), qt.ErrorMatches, "workflow missing does not exist on branch main") - c.Assert(service.GetTabletSeriesFnInvoked, qt.IsFalse) + c.Assert(cmd.Execute(), qt.IsNil) + c.Assert(service.GetTabletSeriesFnInvoked, qt.IsTrue) } func TestTagsCmd_ForwardsSupportedFilters(t *testing.T) { @@ -444,21 +443,20 @@ func TestTagsCmd_ForwardsSupportedFilters(t *testing.T) { c.Assert(service.GetTagSeriesFnInvoked, qt.IsTrue) } -func TestQueriesCmd_RejectsShortQueryID(t *testing.T) { +func TestQueriesCmd_ForwardsOpaqueQueryID(t *testing.T) { c := qt.New(t) service := &mock.MetricsService{ GetQuerySeriesFn: func(ctx context.Context, req *ps.GetQueryMetricSeriesRequest) (*ps.MetricSeries, error) { - c.Fatal("Metrics.GetQuerySeries should not be called") - return nil, nil + c.Assert(req.QueryIDs, qt.DeepEquals, []string{"59801dae501c"}) + return sampleSeries(), nil }, } var buf bytes.Buffer cmd := QueriesCmd(metricsTestHelper(&buf, printer.JSON, &ps.Client{Metrics: service})) cmd.SetArgs([]string{"mydb", "main", "--metric", "queries", "--query-id", "59801dae501c"}) - err := cmd.Execute() - c.Assert(err, qt.IsNotNil) - c.Assert(err.Error(), qt.Contains, `invalid --query-id "59801dae501c"; expected -`) + c.Assert(cmd.Execute(), qt.IsNil) + c.Assert(service.GetQuerySeriesFnInvoked, qt.IsTrue) } func TestParseTagSets(t *testing.T) { @@ -480,7 +478,7 @@ func TestParseTagSets(t *testing.T) { func TestValidateQuerySelector(t *testing.T) { c := qt.New(t) - validID := strings.Repeat("a", 64) + "-commerce" + validID := "59801dae501c" c.Assert(validateQuerySelector([]string{validID}, "", ""), qt.IsNil) c.Assert(validateQuerySelector(nil, "fingerprint", "commerce"), qt.IsNil) diff --git a/internal/cmd/metrics/show.go b/internal/cmd/metrics/show.go index 64c61eaf..6defc8de 100644 --- a/internal/cmd/metrics/show.go +++ b/internal/cmd/metrics/show.go @@ -51,9 +51,6 @@ func ShowCmd(ch *cmdutil.Helper) *cobra.Command { if cmd.Flags().Changed("steps") && flags.steps <= 0 { return fmt.Errorf("--steps must be greater than zero") } - if err := validateQueryIDs(flags.queryIDs); err != nil { - return err - } client, err := ch.Client() if err != nil { @@ -115,7 +112,7 @@ func ShowCmd(ch *cmdutil.Helper) *cobra.Command { cmd.Flags().StringVar(&flags.container, "container", "", "Filter by container") cmd.Flags().StringVar(&flags.pod, "pod", "", "Filter by one pod") cmd.Flags().StringSliceVar(&flags.pods, "pods", nil, "Filter by pods (repeat or comma-separate)") - cmd.Flags().StringSliceVar(&flags.queryIDs, "query-id", nil, "Filter by query pattern ID as - (repeat or comma-separate)") + cmd.Flags().StringSliceVar(&flags.queryIDs, "query-id", nil, "Filter by query pattern ID (repeat or comma-separate)") cmd.Flags().StringVar(&flags.fingerprint, "fingerprint", "", "Filter by query fingerprint") cmd.Flags().StringVar(&flags.budgetID, "budget-id", "", "Filter by traffic budget ID") cmd.Flags().StringVar(&flags.ruleID, "rule-id", "", "Filter by traffic rule ID") diff --git a/internal/cmd/metrics/specialized.go b/internal/cmd/metrics/specialized.go index e9118caa..8252970d 100644 --- a/internal/cmd/metrics/specialized.go +++ b/internal/cmd/metrics/specialized.go @@ -2,7 +2,6 @@ package metrics import ( "fmt" - "regexp" "strings" "github.com/spf13/cobra" @@ -44,9 +43,6 @@ func QueriesCmd(ch *cmdutil.Helper) *cobra.Command { if err := validateSpecializedSeriesFlags(cmd, flags.specializedSeriesFlags); err != nil { return err } - if err := validateQueryIDs(flags.queryIDs); err != nil { - return err - } if err := validateQuerySelector(flags.queryIDs, flags.fingerprint, flags.keyspace); err != nil { return err } @@ -91,7 +87,7 @@ func QueriesCmd(ch *cmdutil.Helper) *cobra.Command { addSpecializedSeriesFlags(cmd, &flags.specializedSeriesFlags) addQueryDimensionFlags(cmd, &flags.queryDimensionFlags) - cmd.Flags().StringSliceVar(&flags.queryIDs, "query-id", nil, "Filter by query pattern ID as - (repeat or comma-separate)") + cmd.Flags().StringSliceVar(&flags.queryIDs, "query-id", nil, "Filter by query pattern ID (repeat or comma-separate)") cmd.Flags().StringVar(&flags.fingerprint, "fingerprint", "", "Filter by query fingerprint") cmd.Flags().StringVar(&flags.keyspace, "keyspace", "", "Keyspace for the query fingerprint") cmd.MarkFlagRequired("metric") // nolint:errcheck @@ -138,27 +134,6 @@ func TabletsCmd(ch *cmdutil.Helper) *cobra.Command { } database, branch := args[0], args[1] - if flags.workflow != "" { - workflows, err := client.Workflows.List(cmd.Context(), &ps.ListWorkflowsRequest{ - Organization: ch.Config.Organization, - Database: database, - }) - if err != nil { - return cmdutil.HandleError(err) - } - - found := false - for _, workflow := range workflows { - if workflow.Name == flags.workflow && workflow.Branch.Name == branch { - found = true - break - } - } - if !found { - return fmt.Errorf("workflow %s does not exist on branch %s", printer.BoldBlue(flags.workflow), printer.BoldBlue(branch)) - } - } - end := specializedMetricsProgress(ch, database, branch, "tablet") defer end() @@ -316,20 +291,6 @@ func TagsCmd(ch *cmdutil.Helper) *cobra.Command { return cmd } -// queryPatternIDPattern matches the API's query pattern ID: a 64 character -// fingerprint joined to the keyspace by a dash. Anything else is silently -// ignored by the API, which returns an empty series instead of an error. -var queryPatternIDPattern = regexp.MustCompile(`^[0-9a-fA-F]{64}-.+$`) - -func validateQueryIDs(ids []string) error { - for _, id := range ids { - if !queryPatternIDPattern.MatchString(id) { - return fmt.Errorf("invalid --query-id %q; expected -, for example %s-mykeyspace. Use --fingerprint and --keyspace instead, or take both from `pscale insights queries`", id, strings.Repeat("a", 64)) - } - } - return nil -} - func validateQuerySelector(ids []string, fingerprint, keyspace string) error { hasIDs := len(ids) > 0 hasFingerprint := fingerprint != ""