Refactor sendGlobsAsIs

We have 3 parameters:
sendGlobsAsIs: true|false
alwaysSendGlobsAsIs: true|false
maxBatchSize: int

Their logic is convoluted
sendGlobsAsIs in true is working together with maxBatchSize
carbonapi is sending a find request and depending on amount of metrics returned from stores acts differently:

if amount of metrics is lower than maxBatchSize it sends request as is with globs
if amount of metrics is higher it uses list of metrics returned from find and query them one by one
sendGlobsAsIs in false is always sending find and ignoring maxBatchSize

alwaysSendGlobsAsIs is neglecting sendGlobsAsIs settings and always send a render request without 'find'

This commit is deprecating sendGlobsAsIs maxBatchSize and alwaysSendGlobsAsIs in favor of
resolveGlobs: int

resolveGlobs: = 0  -> send render query as it is
resolveGlobs: = 1  -> then send find request and send render for every metric separately
resolveGlobs: > 1  -> send find query and if:
count of metrics < resolveGlobs - send render as it is
count of metrics > resolveGlobs - send render query for each metric returned from the find query

Close #33

Author: azhiltsov

app/carbonapi/http_handlers.go

@@ -5,15 +5,16 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"github.com/bookingcom/carbonapi/pkg/handlerlog"
-	"go.uber.org/zap/zapcore"
 	"net/http"
 	"runtime/debug"
 	"strconv"
 	"strings"
 	"sync/atomic"
 	"time"
 
+	"github.com/bookingcom/carbonapi/pkg/handlerlog"
+	"go.uber.org/zap/zapcore"
+
 	"github.com/bookingcom/carbonapi/carbonapipb"
 	"github.com/bookingcom/carbonapi/date"
 	"github.com/bookingcom/carbonapi/expr"
@@ -164,7 +165,7 @@ func (app *App) renderHandler(w http.ResponseWriter, r *http.Request, logger *za
 		}
 		//2xx response code is treated as success
 		if toLog.HttpCode/100 == 2 {
-			if toLog.TotalMetricCount < int64(app.config.MaxBatchSize) {
+			if toLog.TotalMetricCount < int64(app.config.ResolveGlobs) {
 				app.prometheusMetrics.RenderDurationExpSimple.Observe(time.Since(t0).Seconds())
 				app.prometheusMetrics.RenderDurationLinSimple.Observe(time.Since(t0).Seconds())
 			} else {
@@ -700,11 +701,11 @@ func (app *App) renderWriteBody(results []*types.MetricData, form renderForm, r
 }
 
 func (app *App) sendGlobs(glob dataTypes.Matches) bool {
-	if app.config.AlwaysSendGlobsAsIs {
+	if app.config.ResolveGlobs == 0 {
 		return true
+	} else {
+		return len(glob.Matches) < app.config.ResolveGlobs
 	}
-
-	return app.config.SendGlobsAsIs && len(glob.Matches) < app.config.MaxBatchSize
 }
 
 func (app *App) resolveGlobsFromCache(metric string) (dataTypes.Matches, error) {
@@ -759,7 +760,7 @@ func (app *App) resolveGlobs(ctx context.Context, metric string, useCache bool,
 
 func (app *App) getRenderRequests(ctx context.Context, m parser.MetricRequest, useCache bool,
 	toLog *carbonapipb.AccessLogDetails) ([]string, error) {
-	if app.config.AlwaysSendGlobsAsIs {
+	if app.config.ResolveGlobs == 0 {
 		return []string{m.Metric}, nil
 	}
 	if !strings.ContainsAny(m.Metric, "*{") {
@@ -813,7 +814,7 @@ func (app *App) findHandler(w http.ResponseWriter, r *http.Request, logger *zap.
 	logLevel := zap.InfoLevel
 	defer func() {
 		if toLog.HttpCode/100 == 2 {
-			if toLog.TotalMetricCount < int64(app.config.MaxBatchSize) {
+			if toLog.TotalMetricCount < int64(app.config.ResolveGlobs) {
 				app.prometheusMetrics.FindDurationLinSimple.Observe(time.Since(t0).Seconds())
 			} else {
 				app.prometheusMetrics.FindDurationLinComplex.Observe(time.Since(t0).Seconds())

cfg/api.go

@@ -60,9 +60,7 @@ func DefaultAPIConfig() API {
 	cfg := API{
 		Zipper: fromCommon(DefaultCommonConfig()),
 
-		SendGlobsAsIs:       false,
-		AlwaysSendGlobsAsIs: false,
-		MaxBatchSize:        100,
+		ResolveGlobs: 100,
 		Cache: CacheConfig{
 			Type:              "mem",
 			DefaultTimeoutSec: 60,
@@ -86,9 +84,7 @@ type API struct {
 
 	// TODO (grzkv): Move backends list to a single backend here
 
-	SendGlobsAsIs           bool          `yaml:"sendGlobsAsIs"`
-	AlwaysSendGlobsAsIs     bool          `yaml:"alwaysSendGlobsAsIs"`
-	MaxBatchSize            int           `yaml:"maxBatchSize"`
+	ResolveGlobs            int           `yaml:"resolveGlobs"`
 	Cache                   CacheConfig   `yaml:"cache"`
 	TimezoneString          string        `yaml:"tz"`
 	PidFile                 string        `yaml:"pidFile"`

cfg/api_test.go

@@ -31,8 +31,8 @@ graphite:
     prefix: "carbon.api"
     pattern: "{prefix}.{fqdn}"
 
-maxBatchSize: 100
-sendGlobsAsIs: true
+resolveGlobs: 100
+
 cache:
    type: "memcache"
    size_mb: 0
@@ -87,9 +87,7 @@ loggerConfig:
 			},
 		},
 
-		SendGlobsAsIs:       true,
-		AlwaysSendGlobsAsIs: false,
-		MaxBatchSize:        100,
+		ResolveGlobs: 100,
 		Cache: CacheConfig{
 			Type: "memcache",
 			Size: 0,
@@ -125,8 +123,7 @@ cache:
        - host2:1234
 cpus: 16
 tz: "UTC+1,3600"
-sendGlobsAsIs: true
-maxBatchSize: 100
+resolveGlobs: 100
 ignoreClientTimeout: true
 graphite:
     host: "localhost:3002"
@@ -190,9 +187,7 @@ loggerConfig:
 			},
 		},
 
-		SendGlobsAsIs:       true,
-		AlwaysSendGlobsAsIs: false,
-		MaxBatchSize:        100,
+		ResolveGlobs: 100,
 		Cache: CacheConfig{
 			Type: "memcache",
 			Size: 0,

config/carbonapi.yaml

@@ -29,29 +29,37 @@ cache:
        - "127.0.0.1:11211"
 # Amount of CPUs to use. 0 - unlimited
 cpus: 0
-#graphiteWeb: "graphiteWeb.example.yaml"
+
 # Timezone, default - local
 tz: ""
-# If 'true', carbonapi will send requests as is, with globs and braces
-# Otherwise for each request it will generate /metrics/find and then /render
-# individual metrics.
-# true --- faster, but will cause carbonzipper to consume much more RAM.
-#
-# For some backends (e.x. graphite-clickhouse) this *MUST* be set to true in order
-# to get reasonable performance
+
+# Deprecated and removed:
+#  sendGlobsAsIs: true|false
+#  alwaysSendGlobsAsIs: true|false
+#  maxBatchSize: int
+# Migration path
+#  alwaysSendGlobsAsIs: true                -> resolveGlobs: 0
+#  sendGlobsAsIs: false                     -> resolveGlobs: 1
+#  sendGlobsAsIs: true && maxBatchSize: int -> resolveGlobs: int
+
+resolveGlobs: 100
+# = 0 - faster (no 'find in advance', direct render)
+# = 1 - slower (always 'find in advance' and every metric rendered individually)
+# > 1 - slower (always 'find in advance' and then render strategy depend on amount of metrics) 
+# If resolveGlobs is = 0 (zero) then /metrics/find request won't be send and a query will be passed
+# to a /render as it is.
 #
-# For go-carbon --- it depends on how you use it.
-sendGlobsAsIs: true
-# If sendGlobsAsIs is set and resulting response will be larger than maxBatchSize
-# carbonapi will revert to old behavir. This allows you to use benifits of passing
-# globs as is but keep memory usage in sane limits.
+# If resolveGlobs is = 1 (zero) then send /metrics/find request and send /render for every metric separately
 #
-# For go-carbon you might want it to keep in some reasonable limits, 100 is good "safe" defaults
+# If resolveGlobs is set > 1 (one) then carbonapi will send a /metrics/find request and it will check
+# the resulting response if it contain more than resolveGlobs metrics
+#  If find returns MORE metrics than resolveGlobs - carbonapi will query metrics one by one
+#  If find returns LESS metrics than resolveGlobs - revert to the old behaviour and send the query as it is.
+# This allows you to use benifits of passing globs as is but keep memory usage in carbonzipper within sane limits.
 #
-# For some backends (e.x. graphite-clickhouse) you might want to set it to some insanly high value, like 100000
-maxBatchSize: 100
-
-# alwaysSendGlobsAsIs: false
+# For go-carbon you might want to keep it in some reasonable limits, 100 is a good "safe" default
+# For some backends you might want to set it to 0
+# If you noticing carbonzipper OOM then this is a parameter to tune.
 
 # functionsConfigs:
 #     graphiteWeb: ./graphiteWeb.example.yaml
@@ -77,7 +85,7 @@ upstreams:
     # Number of 100ms buckets to track request distribution in. Used to build
     # 'carbon.zipper.hostname.requests_in_0ms_to_100ms' metric and friends.
     buckets: 10
-#    maxBatchSize: 200
+#    resolveGlobs: 200
 #    concurrencyLimitPerServer: 100
     timeouts:
 #        # Maximum backend request time for find requests.