Deprecate old reparent metrics and replace with new ones (#16031)

Signed-off-by: Manan Gupta <manan@planetscale.com>

changelog/20.0/20.0.0/summary.md

@@ -140,6 +140,9 @@ The following metric names have been changed in VTOrc. The old metrics are still
 |          `discoveries.recent_count`          |         `DiscoveriesRecentCount`         |          `vtorc_discoveries_recent_count`          |  
 |               `instance.read`                |              `InstanceRead`              |               `vtorc_instance_read`                |  
 |           `instance.read_topology`           |          `InstanceReadTopology`          |           `vtorc_instance_read_topology`           |
+|         `emergency_reparent_counts`          |        `EmergencyReparentCounts`         |         `vtorc_emergency_reparent_counts`          |
+|          `planned_reparent_counts`           |         `PlannedReparentCounts`          |          `vtorc_planned_reparent_counts`           |
+|      `reparent_shard_operation_timings`      |     `ReparentShardOperationTimings`      |  `vtorc_reparent_shard_operation_timings_bucket`   |
 		
 		
 

go/stats/counter.go

@@ -55,10 +55,10 @@ func NewCounterWithDeprecatedName(name string, deprecatedName string, help strin
 	if deprecatedName == "" || GetSnakeName(name) != GetSnakeName(deprecatedName) {
 		panic(fmt.Sprintf("New name for deprecated metric doesn't have the same snake case - %v", deprecatedName))
 	}
-	v := &Counter{help: help}
-	// We want to publish the deprecated name for backward compatibility.
+
+	v := NewCounter(deprecatedName, help)
+	// We have already published the deprecated name for backward compatibility.
 	// At the same time we want the new metric to be visible on the `/debug/vars` page, so we publish the new name in expvar.
-	publish(deprecatedName, v)
 	expvar.Publish(name, v)
 	return v
 }
@@ -161,11 +161,10 @@ func NewGaugeWithDeprecatedName(name string, deprecatedName string, help string)
 	if deprecatedName == "" || GetSnakeName(name) != GetSnakeName(deprecatedName) {
 		panic(fmt.Sprintf("New name for deprecated metric doesn't have the same snake case - %v", deprecatedName))
 	}
-	v := &Gauge{Counter: Counter{help: help}}
 
-	// We want to publish the deprecated name for backward compatibility.
+	v := NewGauge(deprecatedName, help)
+	// We have already published the deprecated name for backward compatibility.
 	// At the same time we want the new metric to be visible on the `/debug/vars` page, so we publish the new name in expvar.
-	publish(deprecatedName, v)
 	expvar.Publish(name, v)
 	return v
 }

go/stats/counters.go

@@ -18,6 +18,7 @@ package stats
 
 import (
 	"bytes"
+	"expvar"
 	"fmt"
 	"strings"
 	"sync"
@@ -185,6 +186,21 @@ func NewCountersWithMultiLabels(name, help string, labels []string) *CountersWit
 	return t
 }
 
+// NewCountersWithMultiLabelsWithDeprecatedName returns a new CountersWithMultiLabels that also has a deprecated name that can be removed in a future release.
+// It is important to ensure that we only call this function with values for name and deprecatedName such that they match to the same
+// metric name in snake case.
+func NewCountersWithMultiLabelsWithDeprecatedName(name string, deprecatedName string, help string, labels []string) *CountersWithMultiLabels {
+	// Ensure that the snake case for the deprecated name and the new name are the same.
+	if deprecatedName == "" || GetSnakeName(name) != GetSnakeName(deprecatedName) {
+		panic(fmt.Sprintf("New name for deprecated metric doesn't have the same snake case - %v", deprecatedName))
+	}
+	t := NewCountersWithMultiLabels(deprecatedName, help, labels)
+	// We have already published the deprecated name for backward compatibility.
+	// At the same time we want the new metric to be visible on the `/debug/vars` page, so we publish the new name in expvar.
+	expvar.Publish(name, t)
+	return t
+}
+
 // Labels returns the list of labels.
 func (mc *CountersWithMultiLabels) Labels() []string {
 	return mc.labels

go/stats/counters_test.go

@@ -18,14 +18,17 @@ package stats
 
 import (
 	"expvar"
+	"fmt"
 	"math/rand/v2"
 	"reflect"
 	"sort"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestCounters(t *testing.T) {
@@ -269,3 +272,49 @@ func TestCountersCombineDimension(t *testing.T) {
 	c4.Add([]string{"c4", "c2", "c5"}, 1)
 	assert.Equal(t, `{"all.c2.all": 2}`, c4.String())
 }
+
+func TestNewCountersWithMultiLabelsWithDeprecatedName(t *testing.T) {
+	clearStats()
+	Register(func(name string, v expvar.Var) {})
+
+	testcases := []struct {
+		name           string
+		deprecatedName string
+		shouldPanic    bool
+	}{
+		{
+			name:           "counterWithMultiLabels_new_name",
+			deprecatedName: "counterWithMultiLabels_deprecatedName",
+			shouldPanic:    true,
+		},
+		{
+			name:           "counterWithMultiLabels-metricName_test",
+			deprecatedName: "counterWithMultiLabels_metric.name-test",
+			shouldPanic:    false,
+		},
+		{
+			name:           "CounterWithMultiLabelsMetricNameTesting",
+			deprecatedName: "counterWithMultiLabels.metric.name.testing",
+			shouldPanic:    false,
+		},
+	}
+
+	for _, testcase := range testcases {
+		t.Run(fmt.Sprintf("%v-%v", testcase.name, testcase.deprecatedName), func(t *testing.T) {
+			wg := sync.WaitGroup{}
+			wg.Add(1)
+			panicReceived := false
+			go func() {
+				defer func() {
+					if x := recover(); x != nil {
+						panicReceived = true
+					}
+					wg.Done()
+				}()
+				NewCountersWithMultiLabelsWithDeprecatedName(testcase.name, testcase.deprecatedName, "help", []string{"1", "2", "3"})
+			}()
+			wg.Wait()
+			require.EqualValues(t, testcase.shouldPanic, panicReceived)
+		})
+	}
+}

go/stats/timings.go

@@ -18,6 +18,7 @@ package stats
 
 import (
 	"encoding/json"
+	"expvar"
 	"fmt"
 	"sync"
 	"sync/atomic"
@@ -61,6 +62,21 @@ func NewTimings(name, help, label string, categories ...string) *Timings {
 	return t
 }
 
+// NewTimingsWithDeprecatedName returns a new Timings that also has a deprecated name that can be removed in a future release.
+// It is important to ensure that we only call this function with values for name and deprecatedName such that they match to the same
+// metric name in snake case.
+func NewTimingsWithDeprecatedName(name string, deprecatedName string, help, label string, categories ...string) *Timings {
+	// Ensure that the snake case for the deprecated name and the new name are the same.
+	if deprecatedName == "" || GetSnakeName(name) != GetSnakeName(deprecatedName) {
+		panic(fmt.Sprintf("New name for deprecated metric doesn't have the same snake case - %v", deprecatedName))
+	}
+	t := NewTimings(deprecatedName, help, label, categories...)
+	// We have already published the deprecated name for backward compatibility.
+	// At the same time we want the new metric to be visible on the `/debug/vars` page, so we publish the new name in expvar.
+	expvar.Publish(name, t)
+	return t
+}
+
 // Reset will clear histograms and counters: used during testing
 func (t *Timings) Reset() {
 	t.mu.RLock()

go/stats/timings_test.go

@@ -18,11 +18,14 @@ package stats
 
 import (
 	"expvar"
+	"fmt"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestTimings(t *testing.T) {
@@ -99,3 +102,49 @@ func TestTimingsCombineDimension(t *testing.T) {
 	want = `{"TotalCount":1,"TotalTime":1,"Histograms":{"all.c2.all":{"500000":1,"1000000":0,"5000000":0,"10000000":0,"50000000":0,"100000000":0,"500000000":0,"1000000000":0,"5000000000":0,"10000000000":0,"inf":0,"Count":1,"Time":1}}}`
 	assert.Equal(t, want, t3.String())
 }
+
+func TestNewTimingsWithDeprecatedName(t *testing.T) {
+	clearStats()
+	Register(func(name string, v expvar.Var) {})
+
+	testcases := []struct {
+		name           string
+		deprecatedName string
+		shouldPanic    bool
+	}{
+		{
+			name:           "timings_new_name",
+			deprecatedName: "timings_deprecatedName",
+			shouldPanic:    true,
+		},
+		{
+			name:           "timings-metricName_test",
+			deprecatedName: "timings_metric.name-test",
+			shouldPanic:    false,
+		},
+		{
+			name:           "TimingsMetricNameTesting",
+			deprecatedName: "timings.metric.name.testing",
+			shouldPanic:    false,
+		},
+	}
+
+	for _, testcase := range testcases {
+		t.Run(fmt.Sprintf("%v-%v", testcase.name, testcase.deprecatedName), func(t *testing.T) {
+			wg := sync.WaitGroup{}
+			wg.Add(1)
+			panicReceived := false
+			go func() {
+				defer func() {
+					if x := recover(); x != nil {
+						panicReceived = true
+					}
+					wg.Done()
+				}()
+				NewTimingsWithDeprecatedName(testcase.name, testcase.deprecatedName, "help", "label", []string{"1", "2", "3"}...)
+			}()
+			wg.Wait()
+			require.EqualValues(t, testcase.shouldPanic, panicReceived)
+		})
+	}
+}

go/test/endtoend/vtorc/primaryfailure/primary_failure_test.go

@@ -101,6 +101,22 @@ func TestDownPrimary(t *testing.T) {
 	utils.VerifyWritesSucceed(t, clusterInfo, replica, []*cluster.Vttablet{crossCellReplica}, 10*time.Second)
 	utils.WaitForSuccessfulRecoveryCount(t, vtOrcProcess, logic.RecoverDeadPrimaryRecoveryName, 1)
 	utils.WaitForSuccessfulERSCount(t, vtOrcProcess, keyspace.Name, shard0.Name, 1)
+	t.Run("Check ERS and PRS Vars and Metrics", func(t *testing.T) {
+		// These are vars that will be deprecated in v21.
+		utils.CheckVarExists(t, vtOrcProcess, "emergency_reparent_counts")
+		utils.CheckVarExists(t, vtOrcProcess, "planned_reparent_counts")
+		utils.CheckVarExists(t, vtOrcProcess, "reparent_shard_operation_timings")
+
+		// Newly added vars
+		utils.CheckVarExists(t, vtOrcProcess, "EmergencyReparentCounts")
+		utils.CheckVarExists(t, vtOrcProcess, "PlannedReparentCounts")
+		utils.CheckVarExists(t, vtOrcProcess, "ReparentShardOperationTimings")
+
+		// Metrics registered in prometheus
+		utils.CheckMetricExists(t, vtOrcProcess, "vtorc_emergency_reparent_counts")
+		utils.CheckMetricExists(t, vtOrcProcess, "vtorc_planned_reparent_counts")
+		utils.CheckMetricExists(t, vtOrcProcess, "vtorc_reparent_shard_operation_timings_bucket")
+	})
 }
 
 // bring down primary before VTOrc has started, let vtorc repair.

go/test/endtoend/vtorc/utils/utils.go

@@ -1057,7 +1057,7 @@ func CheckVarExists(t *testing.T, vtorcInstance *cluster.VTOrcProcess, metricNam
 	t.Helper()
 	vars := vtorcInstance.GetVars()
 	_, exists := vars[metricName]
-	assert.True(t, exists)
+	assert.True(t, exists, vars)
 }
 
 // CheckMetricExists checks whether the given metric exists or not in /metrics.

go/vt/vtctl/reparentutil/emergency_reparenter.go

@@ -68,7 +68,7 @@ type EmergencyReparentOptions struct {
 }
 
 // counters for Emergency Reparent Shard
-var ersCounter = stats.NewCountersWithMultiLabels("emergency_reparent_counts", "Number of times Emergency Reparent Shard has been run",
+var ersCounter = stats.NewCountersWithMultiLabelsWithDeprecatedName("EmergencyReparentCounts", "emergency_reparent_counts", "Number of times Emergency Reparent Shard has been run",
 	[]string{"Keyspace", "Shard", "Result"},
 )
 

go/vt/vtctl/reparentutil/planned_reparenter.go

@@ -41,7 +41,7 @@ import (
 
 // counters for Planned Reparent Shard
 var (
-	prsCounter = stats.NewCountersWithMultiLabels("planned_reparent_counts", "Number of times Planned Reparent Shard has been run",
+	prsCounter = stats.NewCountersWithMultiLabelsWithDeprecatedName("PlannedReparentCounts", "planned_reparent_counts", "Number of times Planned Reparent Shard has been run",
 		[]string{"Keyspace", "Shard", "Result"},
 	)
 )

go/vt/vtctl/reparentutil/util.go

@@ -44,7 +44,7 @@ import (
 )
 
 var (
-	reparentShardOpTimings = stats.NewTimings("reparent_shard_operation_timings", "Timings of reparent shard operations", "Operation")
+	reparentShardOpTimings = stats.NewTimingsWithDeprecatedName("ReparentShardOperationTimings", "reparent_shard_operation_timings", "Timings of reparent shard operations", "Operation")
 	failureResult          = "failure"
 	successResult          = "success"
 )