proposal: adding DD_PROFILING_ENABLED environment variable to control profiling

[korECM]

I propose adding a DD_PROFILING_ENABLED environment variable to control profiling, similar to how DD_TRACE_ENABLED works for tracing.

Currently, there is no official environment variable to control profiling. This means that applications need to receive a configuration setting and decide whether to start profiling based on that setting in the application code. I believe adding a DD_PROFILING_ENABLED environment variable, like the one used in Python dd-trace, would be very helpful.

Has there been any previous discussion on this topic? If not, would it be alright if I take on the task of adding this feature?

This addition would allow users to easily enable or disable profiling without modifying application code, providing more flexibility and consistency with other Datadog environment variables.

If this proposal is accepted, I'd be happy to work on implementing this feature.

[felixge]

@korECM thanks for raising this issue and offering your help. We should definitely implement this. The fact that it was never implemented is a historical accident.

@nsrip-dd what do you think about this in the light of DataDog/orchestrion#178 ? I would assume that there is no harm in checking for DD_PROFILING_ENABLED in both dd-trace-go AND orchestrion, right? However, once we have it in dd-trace-go, we might be able to remove it from orchestrion?

[nsrip-dd]

TBH I'm not sure it makes much sense to me for this library. As it stands, DD_PROFILING_ENABLED=true would not be sufficient, on its own, to enable profiling. You need to add a profiler.Start call to enable profiling. At that point the only use for DD_PROFILING_ENABLED I see would be to turn off profiling if it's set to false. And what happens to existing users of this library who aren't setting the environment variable? Does the absence of DD_PROFILING_ENABLED mean the same thing as setting it to true?

[nsrip-dd]

Half-baked idea: Maybe it'd be better if we had a simpler API for setting up profiling? Like, you underscore-import it and it adds an init function to start profiling, guarded by the environment variable.

import _ "gopkg.in/DataDog/dd-trace-go.v1/proflier/auto"

For users who are okay with the default configuration for everything, using just a few env vars to set their DD_ENV, DD_SERVICE, etc?

[felixge]

At that point the only use for DD_PROFILING_ENABLED I see would be to turn off profiling if it's set to false. And what happens to existing users of this library who aren't setting the environment variable?

I suspect that's the use case. But maybe @korECM can confirm?

Does the absence of DD_PROFILING_ENABLED mean the same thing as setting it to true?

Yes, I think that would make sense.

Half-baked idea: Maybe it'd be better if we had a simpler API for setting up profiling?

That sounds interesting, but I think that would be an orthogonal topic.

For now I see this issue more about bringing the Go profiler in line with our other profilers as well as the Go tracer (see DD_TRACE_ENABLED env var).

[korECM]

As @felixge mentioned, I indeed want behavior similar to the DD_TRACE_ENABLED env var.

The link provided explains it as follows:

DD_TRACE_ENABLED
Default: true
Enable web framework and library instrumentation. When false, the application code doesn't generate any traces.

Using this environment variable allows the application code to always Start the tracer, while the actual tracing can be controlled via the environment variable. This eliminates the need for the application to separately receive an environment variable and decide whether to Start the tracer based on its value.

However, for the profiler, there is no such variable, so the application needs to separately receive an environment variable and decide whether to Start based on its value. This becomes quite cumbersome and tedious when using the profiler in multiple applications.

Ultimately, the desired behavior, as @nsrip-dd mentioned, is as follows:

The default value of DD_PROFILING_ENABLED is true. If the user hasn't set it explicitly, the code will be profiled if profiler.Start() is called in the application code, and it won't be if it's not called.

If the user sets DD_PROFILING_ENABLED to false, the code won't be profiled even if profiler.Start() is called in the application code.

This allows the application code to specify what to profile and always call Start(), while dynamically adjusting profiling through the environment variable.

I've confirmed that this environment variable already exists in orchestrion, but I'm not sure whether DD_PROFILING_ENABLED should ultimately be included in dd-trace-go and removed from orchestrion. I trust you all to determine the right direction for this.

[nsrip-dd]

The default value of DD_PROFILING_ENABLED is true. If the user hasn't set it explicitly, the code will be profiled if profiler.Start() is called in the application code, and it won't be if it's not called.

If the user sets DD_PROFILING_ENABLED to false, the code won't be profiled even if profiler.Start() is called in the application code.

Okay, this sounds reasonable to me. Please feel free to send a PR. Thanks!