[Serverless] support DD_ENHANCED_METRICS env in lambda extension #8161
Conversation
maxday
changed the title
maxday
changed the title
maxday
changed the title
| @@ -846,6 +846,7 @@ func InitConfig(config Config) { | |||
|
|
|||
| // Serverless Agent | |||
| config.BindEnvAndSetDefault("serverless.logs_enabled", true) | |||
| config.BindEnvAndSetDefault("enhanced_metrics", true) | |||
There was a problem hiding this comment.
We should consider making this serverless.enhanced_metrics. If I understand correctly, this will allow us to configure using the environment variable DD_SERVERLESS_ENHANCED_METRICS but also the configuration file like so:
serverless:
logs_enabled: false
enhanced_metrics: true
The downside is that the environment variable would be different from what is used in the Lambda Library :/
What are your thoughts?
There was a problem hiding this comment.
Is it possible to respect both values? And give DD_SERVERLESS_ENHANCED_METRICS higher precedence?
| @@ -130,3 +130,13 @@ func calculateEstimatedCost(billedDurationMs float64, memorySizeMb float64) floa | |||
| // on some arch. (i.e. 1.00000000000002 values) | |||
| return math.Round((baseLambdaInvocationPrice+(gbSeconds*lambdaPricePerGbSecond))*10e12) / 10e12 | |||
| } | |||
|
|
|||
| // generateEnhancedMetrics generates enhanced metrics from logs and dispatch them to the chan | |||
pkg/serverless/protocol.go
Outdated
| @@ -335,6 +316,33 @@ func (l *LogsCollection) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
| } | |||
| } | |||
|
|
|||
| // processMessage performs logic about metrics and tags on the message | |||
| func enhanceMessage(message aws.LogMessage, arn string, lastRequestID string, functionName string, computeEnhancedMetrics bool, metricTags []string, metricsChan chan []metrics.MetricSample) { | |||
There was a problem hiding this comment.
The docstring says processMessage but the function is called enhanceMessage...
I think processMessage is a better name. This function does other things besides enhancing the message -- it also sets the global request ID, cold start variable, among other things...
pkg/serverless/protocol.go
Outdated
| sendLogsToIntake := config.Datadog.GetBool("serverless.logs_enabled") | ||
| computeEnhancedMetrics := config.Datadog.GetBool("enhanced_metrics") |
There was a problem hiding this comment.
I would rename computeEnhancedMetrics to enhancedMetricsEnabled and rename sendLogsToIntake to logsEnabled -- to make it more clear that they are not functions but booleans.
pkg/serverless/protocol.go
Outdated
|
|
||
| func areEnhancedMetricsEnabled() bool { | ||
| priorityEnv := "serverless.enhanced_metrics" | ||
| nonPriorityEnv := "enhanced_metrics" |
There was a problem hiding this comment.
I think this approach of allowing either environment variable could make sense. I think we should consider the pros and cons of which one should be prioritized.
I do think that in terms of documentation, we probably want to document only the DD_ENHANCED_METRICS environment variable:
- Most customers will likely prefer to configure the extension using environment variables rather than a config file
DD_ENHANCED_METRICSwill enable/disable enhanced metrics in both the Lambda Library and the Extension, whereas if they usedDD_SERVERLESS_ENHANCED_METRICS, it would only affect the extension and they would need to addDD_ENHANCED_METRICSanyway.
Because of that, maybe we should prioritize DD_ENHANCED_METRICS? And then fall back to DD_SERVERLESS_ENHANCED_METRICS if that is present in the config file?
Another option would be to ditch this logic of two different environment variables and just support DD_ENHANCED_METRICS. The only downside is that the config file would have to look like this:
enhanced_metrics: true
serverless:
logs_enabled: true
However, most customers won't use the config file anyway -- and if they do, it won't apply to the Library, so it's not the best way to change this setting.
Sorry for all of the debating and back-and-forth around how to best handle this -- I do think it makes sense to think this through very carefully because once we add these environment variables/config options, they cannot be easily changed or removed.
There was a problem hiding this comment.
Thanks for raising this flag, this is indeed an important decision.
IMO we should
- leave the two env variables because some customers are used to DD_ENHANCED_METRICS but we also have consistency in the config file
- set DD_SERVERLESS_ENHANCED_METRICS with a higher priority than DD_ENHANCED_METRICS since I agree that not a lot of customers will use the configuration file but if they do, I think they might not expect any override be applied afterwards
Does it make sense ?
There was a problem hiding this comment.
Let's go with only one env variable
What does this PR do?
Support DD_ENHANCED_METRICS env variable in Lambda Extension
Motivation
Consistency with dd-lambda-*
Describe how to test your changes
1- aws.lambda.invocations are sent no matter what
2-aws.lambda.enhanced.duration are sent only when the variable was set to true