Merge pull request #371 from azhiltsov/ref_sendGlobAsIs

Refactor sendGlobsAsIs

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.