diff --git a/metrics/query.response-time/response_time.go b/metrics/query.response-time/response_time.go index 0ff8a61..35be210 100644 --- a/metrics/query.response-time/response_time.go +++ b/metrics/query.response-time/response_time.go @@ -7,6 +7,7 @@ import ( "database/sql" "fmt" "math" + "strconv" "strings" "time" @@ -24,6 +25,8 @@ const ( OPT_TRUNCATE_TABLE = "truncate-table" OPT_TRUNCATE_TIMEOUT = "truncate-timeout" + defaultPercentile = "p999" + ERR_NO_TABLE = "table-not-exist" ERR_TRUNCATE_FAILED = "truncate-timeout" @@ -63,6 +66,22 @@ func NewResponseTime(db *sql.DB) *ResponseTime { } } +func percentileMetrics(metrics []string) ([]sqlutil.P, error) { + if len(metrics) == 0 { + metrics = []string{defaultPercentile} + } + for _, metric := range metrics { + if len(metric) < 2 || (metric[0] != 'p' && metric[0] != 'P') { + return nil, fmt.Errorf("invalid percentile metric %q: expected pN where N is an integer from 1 through 999", metric) + } + n, err := strconv.Atoi(metric[1:]) + if err != nil || n < 1 || n > 999 { + return nil, fmt.Errorf("invalid percentile metric %q: expected pN where N is an integer from 1 through 999", metric) + } + } + return sqlutil.PercentileMetrics(metrics) +} + // Domain returns the Blip metric domain name (DOMAIN const). func (c *ResponseTime) Domain() string { return DOMAIN @@ -165,8 +184,9 @@ LEVEL: config.lockWaitQuery = fmt.Sprintf(LOCKWAIT_QUERY, int64(lockWaitTimeout)) } - // Process list of percentiles metrics into a list of names and values - p, err := sqlutil.PercentileMetrics(dom.Metrics) + // Process the configured percentile metrics into names and values, + // applying and validating the documented default during preparation. + p, err := percentileMetrics(dom.Metrics) if err != nil { return nil, err } diff --git a/metrics/query.response-time/response_time_test.go b/metrics/query.response-time/response_time_test.go index c93a364..9252f10 100644 --- a/metrics/query.response-time/response_time_test.go +++ b/metrics/query.response-time/response_time_test.go @@ -4,12 +4,119 @@ package queryresponsetime import ( "context" + "strings" "testing" + "github.com/cashapp/blip/v2" "github.com/cashapp/blip/v2/sqlutil" "github.com/cashapp/blip/v2/test" ) +func TestPrepareDefaultsToP999(t *testing.T) { + c := NewResponseTime(nil) + plan := blip.Plan{ + Levels: map[string]blip.Level{ + "kpi": { + Name: "kpi", + Collect: map[string]blip.Domain{ + DOMAIN: {}, + }, + }, + }, + } + + _, err := c.Prepare(context.Background(), plan) + if err != nil { + t.Fatal(err) + } + + percentiles := c.atLevel["kpi"].percentiles + if len(percentiles) != 1 { + t.Fatalf("prepared %d percentiles, expected 1: %+v", len(percentiles), percentiles) + } + if percentiles[0].formatted != defaultPercentile { + t.Errorf("prepared percentile %q, expected %q", percentiles[0].formatted, defaultPercentile) + } + wantQuery := BASE_QUERY + " WHERE bucket_quantile >= 0.999000 ORDER BY bucket_number LIMIT 1" + if percentiles[0].query != wantQuery { + t.Errorf("prepared query %q, expected %q", percentiles[0].query, wantQuery) + } +} + +func TestPrepareRejectsInvalidPercentileMetrics(t *testing.T) { + for _, metric := range []string{"99", "p0", "p1000", "p99.9", "pfoo"} { + t.Run(metric, func(t *testing.T) { + c := NewResponseTime(nil) + plan := blip.Plan{ + Levels: map[string]blip.Level{ + "kpi": { + Name: "kpi", + Collect: map[string]blip.Domain{ + DOMAIN: {Metrics: []string{metric}}, + }, + }, + }, + } + + _, err := c.Prepare(context.Background(), plan) + if err == nil { + t.Fatalf("Prepare accepted invalid percentile metric %q", metric) + } + if !strings.Contains(err.Error(), "expected pN where N is an integer from 1 through 999") { + t.Errorf("Prepare error %q does not explain the percentile metric format", err) + } + }) + } +} + +func TestCollectDefaultsToP999(t *testing.T) { + _, db, err := test.Connection("mysql80") + if err != nil { + t.Skip("mysql80 not running") + } + defer db.Close() + + c := NewResponseTime(db) + plan := blip.Plan{ + Levels: map[string]blip.Level{ + "kpi": { + Name: "kpi", + Collect: map[string]blip.Domain{ + DOMAIN: { + Options: map[string]string{OPT_TRUNCATE_TABLE: "no"}, + }, + }, + }, + }, + } + + _, err = c.Prepare(context.Background(), plan) + if err != nil { + t.Fatal(err) + } + metrics, err := c.Collect(context.Background(), "kpi") + if err != nil { + t.Fatal(err) + } + if len(metrics) != 1 { + t.Fatalf("collected %d metrics, expected 1: %+v", len(metrics), metrics) + } + if metrics[0].Name != defaultPercentile { + t.Errorf("collected metric %q, expected %q", metrics[0].Name, defaultPercentile) + } + realPercentile, ok := metrics[0].Meta[defaultPercentile] + if !ok { + t.Fatalf("metric meta does not have key %q: %+v", defaultPercentile, metrics[0].Meta) + } + value, err := sqlutil.ParsePercentileStr(realPercentile) + if err != nil { + t.Fatal(err) + } + if value < 0.999 { + t.Errorf("real percentile %s is %f, expected at least 0.999", realPercentile, value) + } +} + func TestCollectP(t *testing.T) { _, db, err := test.Connection("mysql80") if err != nil {