Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

profiler: Disable agentless mode for WithAPIKey #906

Merged
merged 9 commits into from
Apr 21, 2021
Merged

profiler: Disable agentless mode for WithAPIKey #906

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6994492
profiler: WithAPIKey Warning, WithAgentlessUpload
felixge Apr 8, 2021
7637001
env var
felixge Apr 9, 2021
9664117
profiler: Change WithAPIKey default behavior
felixge Apr 19, 2021
1aa0f15
Fix test case
felixge Apr 19, 2021
c43d1d3
Improve tests
felixge Apr 19, 2021
a6651bc
Make golint happy
felixge Apr 19, 2021
9979c9e
Update profiler/profiler.go
felixge Apr 19, 2021
ec0b130
Use internal.BoolEnv
felixge Apr 19, 2021
4e19879
Merge branch 'v1' into felix.geisendoerfer/remove-automatic-agentless…
felixge Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions profiler/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -158,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" {
felixge marked this conversation as resolved.
Show resolved Hide resolved
WithAgentlessUpload()(&c)
}
if v := os.Getenv("DD_SITE"); v != "" {
WithSite(v)(&c)
}
Expand Down Expand Up @@ -209,15 +213,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 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
}
}

// 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) {
Expand Down
17 changes: 15 additions & 2 deletions profiler/profiler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +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")
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.")
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.")
Copy link
Contributor

@AlexJF AlexJF Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This might be too "aggressive" as it's rather big, specially on those cases where people have no control. You do mention they can remove the warning by changing but is it gaining us anything? Anyway not a big deal for me, I just know that some tracers care about reducing verbosity.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, why are we warning when agentless is still a legitimate use case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This might be too "aggressive" as it's rather big, specially on those cases where people have no control. You do mention they can remove the warning by changing but is it gaining us anything? Anyway not a big deal for me, I just know that some tracers care about reducing verbosity.

This PR introduces a backwards incompatible change in a minor release of the profiling library. Arguably a big warning is warranted. But if you think dropping the bit about removing the warning would help, I'd be okay with that. Just let me know.

I'm also okay to change the entire message if you have a different proposal, just let me know.

Also, why are we warning when agentless is still a legitimate use case?

There is no legitimate production use case at this point. The main use case is for our own internal testing, and perhaps to help customers troubleshoot connectivity issues. But none of that code should ever run in production, so I think the warning makes sense?

Copy link
Member

@pmbauer pmbauer Apr 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no legitimate production use case at this point.

Going to push back on that a bit. I can think of a number of use cases that don't run afoul of billing issues where a customer might not want to run the agent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Legitimate production use cases aside (I'm going to kind of disagree with that assertion), I do not think this should be a warning. Documented or not, the client does support running agentless, even if it is no longer the default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmbauer perhaps I don't fully understand the use cases you have in mind. Let's continue the discussion in the google doc thread and make sure we're aligned before continuing the code related discussions here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replied in the doc. The question of billing for profiles not submitted via the agent is a product and business decision. That in no way makes the production uses for agentless uploads illegitimate from a customer perspective.

Copy link
Member

@pmbauer pmbauer Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I do feel strongly about the business decision to force profiles through the agent (it's bad for customers and bad for business long term as severless becomes the norm) I don't feel strongly about the warning. Feel free to ship.

}
cfg.targetURL = cfg.agentURL
}
if cfg.hostname == "" {
Expand Down
15 changes: 13 additions & 2 deletions profiler/profiler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,25 @@ 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) {
err := Start(WithAPIKey("aaaa"))
err := Start(WithAPIKey("aaaa"), WithAgentlessUpload())
defer Stop()
assert.NotNil(t, err)

Expand Down