From 059f6262fe46dc7a2b358fa2c35476ad43336cbf Mon Sep 17 00:00:00 2001
From: David Goffredo <david.goffredo@datadoghq.com>
Date: Tue, 27 Jun 2023 19:24:06 +0000
Subject: [PATCH] datadog: sample_rate omitted by default

---
 internal/ingress/controller/config/config.go | 17 ++--
 internal/ingress/controller/nginx.go         | 81 ++++++++++++--------
 2 files changed, 57 insertions(+), 41 deletions(-)

diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go
index ec44b08ed..30d30f3f7 100644
--- a/internal/ingress/controller/config/config.go
+++ b/internal/ingress/controller/config/config.go
@@ -87,6 +87,11 @@ const (
 	// Parameters for a shared memory zone that will keep states for various keys.
 	// http://nginx.org/en/docs/http/ngx_http_limit_conn_module.html#limit_conn_zone
 	defaultLimitConnZoneVariable = "$binary_remote_addr"
+
+	// Default sentinel value for Configuration.DatadogSampleRate, indicating
+	// that the Datadog tracer should consult the Datadog Agent for sampling
+	// rates rather than use a configured value.
+	DatadogDynamicSampleRate = -1.0
 )
 
 // Configuration represents the content of nginx.conf file
@@ -706,15 +711,8 @@ type Configuration struct {
 	// Default: nginx.handle
 	DatadogOperationNameOverride string `json:"datadog-operation-name-override"`
 
-	// DatadogPrioritySampling specifies to use client-side sampling
-	// If true disables client-side sampling (thus ignoring sample_rate) and enables distributed
-	// priority sampling, where traces are sampled based on a combination of user-assigned
-	// Default: true
-	DatadogPrioritySampling bool `json:"datadog-priority-sampling"`
-
 	// DatadogSampleRate specifies sample rate for any traces created.
-	// This is effective only when datadog-priority-sampling is false
-	// Default: 1.0
+	// Default: use a dynamic rate instead
 	DatadogSampleRate float32 `json:"datadog-sample-rate"`
 
 	// MainSnippet adds custom configuration to the main section of the nginx configuration
@@ -1001,8 +999,7 @@ func NewDefault() Configuration {
 		DatadogEnvironment:                     "prod",
 		DatadogCollectorPort:                   8126,
 		DatadogOperationNameOverride:           "nginx.handle",
-		DatadogSampleRate:                      1.0,
-		DatadogPrioritySampling:                true,
+		DatadogSampleRate:                      DatadogDynamicSampleRate,
 		LimitReqStatusCode:                     503,
 		LimitConnStatusCode:                    503,
 		SyslogPort:                             514,
diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go
index 80693db5c..41903fb0a 100644
--- a/internal/ingress/controller/nginx.go
+++ b/internal/ingress/controller/nginx.go
@@ -1051,16 +1051,6 @@ const jaegerTmpl = `{
   }
 }`
 
-const datadogTmpl = `{
-  "service": "{{ .DatadogServiceName }}",
-  "agent_host": "{{ .DatadogCollectorHost }}",
-  "agent_port": {{ .DatadogCollectorPort }},
-  "environment": "{{ .DatadogEnvironment }}",
-  "operation_name_override": "{{ .DatadogOperationNameOverride }}",
-  "sample_rate": {{ .DatadogSampleRate }},
-  "dd.priority.sampling": {{ .DatadogPrioritySampling }}
-}`
-
 const otelTmpl = `
 exporter = "otlp"
 processor = "batch"
@@ -1084,37 +1074,66 @@ ratio = {{ .OtelSamplerRatio }}
 parent_based = {{ .OtelSamplerParentBased }}
 `
 
-func createOpentracingCfg(cfg ngx_config.Configuration) error {
-	var tmpl *template.Template
-	var err error
+func datadogOpentracingCfg(cfg ngx_config.Configuration) (string, error) {
+	jsn := map[string]interface{}{
+		"service": cfg.DatadogServiceName,
+		"agent_host": cfg.DatadogCollectorHost,
+		"agent_port": cfg.DatadogCollectorPort,
+		"environment": cfg.DatadogEnvironment,
+		"operation_name_override": cfg.DatadogOperationNameOverride,
+	}
 
-	if cfg.ZipkinCollectorHost != "" {
-		tmpl, err = template.New("zipkin").Parse(zipkinTmpl)
-		if err != nil {
-			return err
-		}
-	} else if cfg.JaegerCollectorHost != "" || cfg.JaegerEndpoint != "" {
-		tmpl, err = template.New("jaeger").Parse(jaegerTmpl)
-		if err != nil {
-			return err
-		}
-	} else if cfg.DatadogCollectorHost != "" {
-		tmpl, err = template.New("datadog").Parse(datadogTmpl)
-		if err != nil {
-			return err
-		}
-	} else {
-		tmpl, _ = template.New("empty").Parse("{}")
+	// Omit "sample_rate" if the configuration's sample rate is set to the
+	// default sentinel value DatadogDynamicSampleRate (-1).
+	// Omitting "sample_rate" from the plugin JSON indicates to the tracer that
+	// it should use dynamic rates instead of a configured rate.
+	if cfg.DatadogSampleRate != ngx_config.DatadogDynamicSampleRate {
+		jsn["sample_rate"] = cfg.DatadogSampleRate
+	}
+
+	jsnBytes, err := json.Marshal(jsn)
+	if err != nil {
+		return "", err
+	}
+
+	return string(jsnBytes), nil
+}
+
+func opentracingCfgFromTemplate(cfg ngx_config.Configuration, tmplName string, tmplText string) (string, error) {
+	tmpl, err := template.New(tmplName).Parse(tmplText)
+	if err != nil {
+		return "", err
 	}
 
 	tmplBuf := bytes.NewBuffer(make([]byte, 0))
 	err = tmpl.Execute(tmplBuf, cfg)
+	if err != nil {
+		return "", err
+	}
+
+	return tmplBuf.String(), nil
+}
+
+func createOpentracingCfg(cfg ngx_config.Configuration) error {
+	var fileContent string
+	var err error
+
+	if cfg.ZipkinCollectorHost != "" {
+		fileContent, err = opentracingCfgFromTemplate(cfg, "zipkin", zipkinTmpl)
+	} else if cfg.JaegerCollectorHost != "" || cfg.JaegerEndpoint != "" {
+		fileContent, err = opentracingCfgFromTemplate(cfg, "jaeger", jaegerTmpl)
+	} else if cfg.DatadogCollectorHost != "" {
+		fileContent, err = datadogOpentracingCfg(cfg)
+	} else {
+		fileContent = "{}"
+	}
+
 	if err != nil {
 		return err
 	}
 
 	// Expand possible environment variables before writing the configuration to file.
-	expanded := os.ExpandEnv(tmplBuf.String())
+	expanded := os.ExpandEnv(fileContent)
 
 	return os.WriteFile("/etc/nginx/opentracing.json", []byte(expanded), file.ReadWriteByUser)
 }