From 059f6262fe46dc7a2b358fa2c35476ad43336cbf Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Tue, 27 Jun 2023 19:24:06 +0000 Subject: [PATCH 1/4] 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) } From fb1dcff2f3a3b1ca8e8157afb47dd21f470c89cb Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Tue, 27 Jun 2023 22:45:50 +0000 Subject: [PATCH 2/4] config: use *float32 with nil instead of float32 with sentinel value --- internal/ingress/controller/config/config.go | 9 ++------- internal/ingress/controller/nginx.go | 7 +++---- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/internal/ingress/controller/config/config.go b/internal/ingress/controller/config/config.go index 30d30f3f7..49230013b 100644 --- a/internal/ingress/controller/config/config.go +++ b/internal/ingress/controller/config/config.go @@ -87,11 +87,6 @@ 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 @@ -713,7 +708,7 @@ type Configuration struct { // DatadogSampleRate specifies sample rate for any traces created. // Default: use a dynamic rate instead - DatadogSampleRate float32 `json:"datadog-sample-rate"` + DatadogSampleRate *float32 `json:"datadog-sample-rate",omitempty` // MainSnippet adds custom configuration to the main section of the nginx configuration MainSnippet string `json:"main-snippet"` @@ -999,7 +994,7 @@ func NewDefault() Configuration { DatadogEnvironment: "prod", DatadogCollectorPort: 8126, DatadogOperationNameOverride: "nginx.handle", - DatadogSampleRate: DatadogDynamicSampleRate, + DatadogSampleRate: nil, LimitReqStatusCode: 503, LimitConnStatusCode: 503, SyslogPort: 514, diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index 41903fb0a..c797eb122 100644 --- a/internal/ingress/controller/nginx.go +++ b/internal/ingress/controller/nginx.go @@ -1083,12 +1083,11 @@ func datadogOpentracingCfg(cfg ngx_config.Configuration) (string, error) { "operation_name_override": cfg.DatadogOperationNameOverride, } - // Omit "sample_rate" if the configuration's sample rate is set to the - // default sentinel value DatadogDynamicSampleRate (-1). + // Omit "sample_rate" if the configuration's sample rate is unset (nil). // 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 + if cfg.DatadogSampleRate != nil { + jsn["sample_rate"] = *cfg.DatadogSampleRate } jsnBytes, err := json.Marshal(jsn) From e7f45a7f70eaa8528f85a9efd91f4f7d4c9a6eb2 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Wed, 28 Jun 2023 17:39:06 +0000 Subject: [PATCH 3/4] change some names --- internal/ingress/controller/nginx.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index c797eb122..a21f2f5e2 100644 --- a/internal/ingress/controller/nginx.go +++ b/internal/ingress/controller/nginx.go @@ -1075,7 +1075,7 @@ parent_based = {{ .OtelSamplerParentBased }} ` func datadogOpentracingCfg(cfg ngx_config.Configuration) (string, error) { - jsn := map[string]interface{}{ + m := map[string]interface{}{ "service": cfg.DatadogServiceName, "agent_host": cfg.DatadogCollectorHost, "agent_port": cfg.DatadogCollectorPort, @@ -1087,15 +1087,15 @@ func datadogOpentracingCfg(cfg ngx_config.Configuration) (string, error) { // 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 != nil { - jsn["sample_rate"] = *cfg.DatadogSampleRate + m["sample_rate"] = *cfg.DatadogSampleRate } - jsnBytes, err := json.Marshal(jsn) + buf, err := json.Marshal(m) if err != nil { return "", err } - return string(jsnBytes), nil + return string(buf), nil } func opentracingCfgFromTemplate(cfg ngx_config.Configuration, tmplName string, tmplText string) (string, error) { @@ -1114,17 +1114,17 @@ func opentracingCfgFromTemplate(cfg ngx_config.Configuration, tmplName string, t } func createOpentracingCfg(cfg ngx_config.Configuration) error { - var fileContent string + var configData string var err error if cfg.ZipkinCollectorHost != "" { - fileContent, err = opentracingCfgFromTemplate(cfg, "zipkin", zipkinTmpl) + configData, err = opentracingCfgFromTemplate(cfg, "zipkin", zipkinTmpl) } else if cfg.JaegerCollectorHost != "" || cfg.JaegerEndpoint != "" { - fileContent, err = opentracingCfgFromTemplate(cfg, "jaeger", jaegerTmpl) + configData, err = opentracingCfgFromTemplate(cfg, "jaeger", jaegerTmpl) } else if cfg.DatadogCollectorHost != "" { - fileContent, err = datadogOpentracingCfg(cfg) + configData, err = datadogOpentracingCfg(cfg) } else { - fileContent = "{}" + configData = "{}" } if err != nil { @@ -1132,7 +1132,7 @@ func createOpentracingCfg(cfg ngx_config.Configuration) error { } // Expand possible environment variables before writing the configuration to file. - expanded := os.ExpandEnv(fileContent) + expanded := os.ExpandEnv(configData) return os.WriteFile("/etc/nginx/opentracing.json", []byte(expanded), file.ReadWriteByUser) } From 7d60dc40fc117876747855f96ad019290255bd13 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Thu, 29 Jun 2023 20:26:05 +0000 Subject: [PATCH 4/4] gofmt -s -w internal/ingress/controller/nginx.go --- internal/ingress/controller/nginx.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/ingress/controller/nginx.go b/internal/ingress/controller/nginx.go index a21f2f5e2..4ecc0f1d0 100644 --- a/internal/ingress/controller/nginx.go +++ b/internal/ingress/controller/nginx.go @@ -1076,10 +1076,10 @@ parent_based = {{ .OtelSamplerParentBased }} func datadogOpentracingCfg(cfg ngx_config.Configuration) (string, error) { m := map[string]interface{}{ - "service": cfg.DatadogServiceName, - "agent_host": cfg.DatadogCollectorHost, - "agent_port": cfg.DatadogCollectorPort, - "environment": cfg.DatadogEnvironment, + "service": cfg.DatadogServiceName, + "agent_host": cfg.DatadogCollectorHost, + "agent_port": cfg.DatadogCollectorPort, + "environment": cfg.DatadogEnvironment, "operation_name_override": cfg.DatadogOperationNameOverride, }