From 6994492ed16656b64ba44e52a5e8d32e7c994d42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Geisend=C3=B6rfer?= Date: Thu, 8 Apr 2021 13:35:42 +0200 Subject: [PATCH 1/8] profiler: WithAPIKey Warning, WithAgentlessUpload This pull request warns users of WithAPIKey that the option alone will no longer enable agentless uploading by default in the future. It can still be enabled via WithAgentlessUpload but this is discouraged and not officially supported. --- profiler/options.go | 23 +++++++++++++++++++---- profiler/profiler.go | 7 +++++++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/profiler/options.go b/profiler/options.go index 33f7889027..205cdcb537 100644 --- a/profiler/options.go +++ b/profiler/options.go @@ -77,7 +77,8 @@ var defaultClient = &http.Client{ var defaultProfileTypes = []ProfileType{MetricsProfile, CPUProfile, HeapProfile} type config struct { - apiKey string + apiKey string + agentless bool // targetURL is the upload destination URL. It will be set by the profiler on start to either apiURL or agentURL // based on the other options. targetURL string @@ -209,15 +210,29 @@ func WithAgentAddr(hostport string) Option { } } -// WithAPIKey is deprecated and might be removed in future versions of this -// package. It allows to skip the agent and talk to the Datadog API directly -// using the provided API key. +// WithAPIKey sets a datadog API key and enables agentless uploading by +// default. This option takes precedence over the DD_API_KEY env variable. +// WARNING: In the next release of this library setting an API Key will no +// longer enable agentless uploading and default to agent based uploading +// instead. If you don't have an agent running on the default localhost:8126 +// hostport you need to set it up, or use WithAgentAddr to specify the hostport +// you are using. See WithAgentlessUpload for more information. func WithAPIKey(key string) Option { return func(cfg *config) { cfg.apiKey = key } } +// WithAgentlessUpload is currently for internal usage only and not officially +// supported. You should not enable it unless somebody at Datadog instructed +// you to do so. It allows to skip the agent and talk to the Datadog API +// directly using the provided API key. +func WithAgentlessUpload() Option { + return func(cfg *config) { + cfg.agentless = true + } +} + // WithURL specifies the HTTP URL for the Datadog Profiling API. func WithURL(url string) Option { return func(cfg *config) { diff --git a/profiler/profiler.go b/profiler/profiler.go index 0383f3c8fd..a1fffcdf0f 100644 --- a/profiler/profiler.go +++ b/profiler/profiler.go @@ -82,6 +82,13 @@ func newProfiler(opts ...Option) (*profiler, error) { if !isAPIKeyValid(cfg.apiKey) { return nil, errors.New("API key has incorrect format") } + if cfg.agentless { + log.Warn("profiler.WithAgentlessUpload is currently for internal usage only and not officially supported. You should not enable it unless somebody at Datadog instructed you to do so.") + } else { + // TODO(fg) once this has been released, make a new PR to default to agent + // based uploading unless agentless is explicitly configured. + log.Warn("The next version of this library might break your profiling integration. Please check the go documentation for profiler.WithAPIKey of the dd-trace-go version you are using for more information.") + } cfg.targetURL = cfg.apiURL } else { cfg.targetURL = cfg.agentURL From 76370016b6dfc7777d369d1854f986c18a11b2c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Geisend=C3=B6rfer?= Date: Fri, 9 Apr 2021 13:19:38 +0200 Subject: [PATCH 2/8] env var --- profiler/options.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/profiler/options.go b/profiler/options.go index 205cdcb537..f200affab9 100644 --- a/profiler/options.go +++ b/profiler/options.go @@ -159,6 +159,9 @@ func defaultConfig() (*config, error) { if v := os.Getenv("DD_API_KEY"); v != "" { WithAPIKey(v)(&c) } + if v := os.Getenv("DD_PROFILING_AGENTLESS"); v == "true" { + WithAgentlessUpload()(&c) + } if v := os.Getenv("DD_SITE"); v != "" { WithSite(v)(&c) } From 96641172a2c467363e24fa31ed0446b42edae3cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Geisend=C3=B6rfer?= Date: Mon, 19 Apr 2021 12:27:23 +0200 Subject: [PATCH 3/8] profiler: Change WithAPIKey default behavior --- profiler/options.go | 14 +++++++------- profiler/profiler.go | 24 +++++++++++++++--------- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/profiler/options.go b/profiler/options.go index f200affab9..a12d7663d9 100644 --- a/profiler/options.go +++ b/profiler/options.go @@ -213,13 +213,13 @@ func WithAgentAddr(hostport string) Option { } } -// WithAPIKey sets a datadog API key and enables agentless uploading by -// default. This option takes precedence over the DD_API_KEY env variable. -// WARNING: In the next release of this library setting an API Key will no -// longer enable agentless uploading and default to agent based uploading -// instead. If you don't have an agent running on the default localhost:8126 -// hostport you need to set it up, or use WithAgentAddr to specify the hostport -// you are using. See WithAgentlessUpload for more information. +// WithAPIKey sets the Datadog API Key and takes precedence over the DD_API_KEY +// env variable. Historically this option was used to enable agentless +// uploading, but as of dd-trace-go v1.30.0 the behavior has changed to always +// default to agent based uploading which doesn't require an API key. So if you +// currently don't have an agent running on the default localhost:8126 hostport +// you need to set it up, or use WithAgentAddr to specify the hostport location +// of the agent. See WithAgentlessUpload for more information. func WithAPIKey(key string) Option { return func(cfg *config) { cfg.apiKey = key diff --git a/profiler/profiler.go b/profiler/profiler.go index a1fffcdf0f..94780ae856 100644 --- a/profiler/profiler.go +++ b/profiler/profiler.go @@ -78,19 +78,25 @@ func newProfiler(opts ...Option) (*profiler, error) { if os.Getenv("DD_PROFILING_WAIT_PROFILE") != "" { cfg.addProfileType(expGoroutineWaitProfile) } - if cfg.apiKey != "" { + // Agentless upload is disabled by default as of v1.30.0, but + // WithAgentlessUpload can be used to enable it for testing and debugging. + if cfg.agentless { if !isAPIKeyValid(cfg.apiKey) { - return nil, errors.New("API key has incorrect format") - } - if cfg.agentless { - log.Warn("profiler.WithAgentlessUpload is currently for internal usage only and not officially supported. You should not enable it unless somebody at Datadog instructed you to do so.") - } else { - // TODO(fg) once this has been released, make a new PR to default to agent - // based uploading unless agentless is explicitly configured. - log.Warn("The next version of this library might break your profiling integration. Please check the go documentation for profiler.WithAPIKey of the dd-trace-go version you are using for more information.") + return nil, errors.New("profiler.WithAgentlessUpload requires a valid API key. Use profiler.WithAPIKey or the DD_API_KEY env variable to set it.") } + // Always warn people against using this mode for now. All customers should + // use agent based uploading at this point. + log.Warn("profiler.WithAgentlessUpload is currently for internal usage only and not officially supported. You should not enable it unless somebody at Datadog instructed you to do so.") cfg.targetURL = cfg.apiURL } else { + // Historically people could use an API Key to enable agentless uploading. + // As of v1.30.0 customers the default behavior is to use agent based + // uploading regardless of the presence of an API key. So if we see an API + // key configured, we warn the customers that this is probably a + // misconfiguration. + if cfg.apiKey != "" { + log.Warn("You are currently setting profiler.WithAPIKey or the DD_API_KEY env variable, but as of dd-trace-go v1.30.0 this value is getting ignored by the profiler. Please see the profiler.WithAPIKey go docs and verify that your integration is still working. If you can't remove DD_API_KEY from your environment, you can use WithAPIKey(\"\") to silence this warning.") + } cfg.targetURL = cfg.agentURL } if cfg.hostname == "" { From 1aa0f155844f316dd067cce145ebff1cf4a676ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Geisend=C3=B6rfer?= Date: Mon, 19 Apr 2021 12:32:19 +0200 Subject: [PATCH 4/8] Fix test case --- profiler/profiler_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/profiler/profiler_test.go b/profiler/profiler_test.go index e6f63aca3b..61c6c00c2e 100644 --- a/profiler/profiler_test.go +++ b/profiler/profiler_test.go @@ -59,13 +59,16 @@ func TestStart(t *testing.T) { }) t.Run("options/GoodAPIKey", func(t *testing.T) { - err := Start(WithAPIKey("12345678901234567890123456789012")) + err := Start( + WithAPIKey("12345678901234567890123456789012"), + WithAgentlessUpload(), + ) defer Stop() assert.Nil(t, err) }) t.Run("options/BadAPIKey", func(t *testing.T) { - err := Start(WithAPIKey("aaaa")) + err := Start(WithAPIKey("aaaa"), WithAgentlessUpload()) defer Stop() assert.NotNil(t, err) From c43d1d341d1b34a3e914da164a7859f1c6d7cb14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Geisend=C3=B6rfer?= Date: Mon, 19 Apr 2021 12:36:29 +0200 Subject: [PATCH 5/8] Improve tests --- profiler/profiler_test.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/profiler/profiler_test.go b/profiler/profiler_test.go index 61c6c00c2e..b6fc4f756f 100644 --- a/profiler/profiler_test.go +++ b/profiler/profiler_test.go @@ -58,13 +58,21 @@ func TestStart(t *testing.T) { mu.Unlock() }) - t.Run("options/GoodAPIKey", func(t *testing.T) { + t.Run("options/GoodAPIKey/Agent", func(t *testing.T) { + err := Start(WithAPIKey("12345678901234567890123456789012")) + defer Stop() + assert.Nil(t, err) + assert.Equal(t, activeProfiler.cfg.agentURL, activeProfiler.cfg.targetURL) + }) + + t.Run("options/GoodAPIKey/Agentless", func(t *testing.T) { err := Start( WithAPIKey("12345678901234567890123456789012"), WithAgentlessUpload(), ) defer Stop() assert.Nil(t, err) + assert.Equal(t, activeProfiler.cfg.apiURL, activeProfiler.cfg.targetURL) }) t.Run("options/BadAPIKey", func(t *testing.T) { From a6651bce322476facda7a723f5b3f41030504a9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Geisend=C3=B6rfer?= Date: Mon, 19 Apr 2021 12:38:13 +0200 Subject: [PATCH 6/8] Make golint happy --- profiler/profiler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/profiler/profiler.go b/profiler/profiler.go index 94780ae856..a4dd106bd1 100644 --- a/profiler/profiler.go +++ b/profiler/profiler.go @@ -82,7 +82,7 @@ func newProfiler(opts ...Option) (*profiler, error) { // WithAgentlessUpload can be used to enable it for testing and debugging. if cfg.agentless { if !isAPIKeyValid(cfg.apiKey) { - return nil, errors.New("profiler.WithAgentlessUpload requires a valid API key. Use profiler.WithAPIKey or the DD_API_KEY env variable to set it.") + return nil, errors.New("profiler.WithAgentlessUpload requires a valid API key. Use profiler.WithAPIKey or the DD_API_KEY env variable to set it") } // Always warn people against using this mode for now. All customers should // use agent based uploading at this point. From 9979c9ec2a3ab24d4ee8bca690cbce7c46ae94fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Geisend=C3=B6rfer?= Date: Mon, 19 Apr 2021 14:32:31 +0200 Subject: [PATCH 7/8] Update profiler/profiler.go Co-authored-by: Gabriel Aszalos --- profiler/profiler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/profiler/profiler.go b/profiler/profiler.go index a4dd106bd1..2de7eb7884 100644 --- a/profiler/profiler.go +++ b/profiler/profiler.go @@ -86,7 +86,7 @@ func newProfiler(opts ...Option) (*profiler, error) { } // Always warn people against using this mode for now. All customers should // use agent based uploading at this point. - log.Warn("profiler.WithAgentlessUpload is currently for internal usage only and not officially supported. You should not enable it unless somebody at Datadog instructed you to do so.") + log.Warn("profiler.WithAgentlessUpload is currently for internal usage only and not officially supported.") cfg.targetURL = cfg.apiURL } else { // Historically people could use an API Key to enable agentless uploading. From ec0b13074e5a5b19b0974c4ef93f55b229cfa6cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Geisend=C3=B6rfer?= Date: Mon, 19 Apr 2021 16:44:59 +0200 Subject: [PATCH 8/8] Use internal.BoolEnv --- profiler/options.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/profiler/options.go b/profiler/options.go index a12d7663d9..a25012da54 100644 --- a/profiler/options.go +++ b/profiler/options.go @@ -18,6 +18,7 @@ import ( "time" "unicode" + "gopkg.in/DataDog/dd-trace-go.v1/internal" "gopkg.in/DataDog/dd-trace-go.v1/internal/globalconfig" "gopkg.in/DataDog/dd-trace-go.v1/internal/log" "gopkg.in/DataDog/dd-trace-go.v1/internal/version" @@ -159,7 +160,7 @@ func defaultConfig() (*config, error) { if v := os.Getenv("DD_API_KEY"); v != "" { WithAPIKey(v)(&c) } - if v := os.Getenv("DD_PROFILING_AGENTLESS"); v == "true" { + if internal.BoolEnv("DD_PROFILING_AGENTLESS", false) { WithAgentlessUpload()(&c) } if v := os.Getenv("DD_SITE"); v != "" {