elastic / elastic/apm-agent-nodejs

invalid values for time/duration config vars gives surprising and unwanted behaviour

Open
#2,120 4 comments 0 reactions 0 assignees View on GitHub
agent-nodejs
Dominant language
JavaScript
Stars
594
Forks
244
Avg merge
1d 8h
Merged PRs (30d)
16

Description

There are a few config vars that support a time/duration as a string of the form ``, e.g. "10s" to mean 10 seconds. The supported units are "m" for minutes, "ms" for milliseconds, "s" for seconds. If the config value is a number or a string without units, it defaults to seconds.

Currently that parsing/conversion is done by `normalizeTime` in lib/config.js:

```
function normalizeTime (opts) {
for (const key of TIME_OPTS) {
if (key in opts) opts[key] = toSeconds(String(opts[key]))
}
}
```

and the relevant config vars are:

```
var TIME_OPTS = [
'abortedErrorThreshold',
'apiRequestTime',
'metricsInterval',
'serverTimeout',
'spanFramesMinDuration'
]
```

Currently `toSeconds()` above will return *`null`* if the string value is invalid (e.g. "10ss", "10h", "fuzzywuzzy"). There is no log warning. The results can be surprising: If this `null` is used in math (often that is the case as the seconds value is converted to milliseconds for usage in `setTimeout` and the like) or comparisons, JavaScript will treat it as zero, which sometimes is a sentinel value that means something unexpected. Specifically:

- `apiRequestTime`: Results in a value of `0` being used to the http-client ->
a `-1` sentinel value to StreamChopper, which results in time-based closing
of requests to APM server *not* happening which -- for low traffic apps --
results in `ERROR (elastic-apm-node): APM Server transport error: APM Server response timeout (30000ms)`.
which results in the stream chopper not
- `metricsInterval`: Results in null -> 0 -> turning *off* metrics collection.
- `serverTimeout`: Results in no socket timeout being set on the communication
with APM server which could rarely result in hung communication with APM server.
- `spanFramesMinDuration`: Results in possibly unexpected additional CPU usage
as the agent collections stack traces for all spans.
- `abortedErrorThreshold`: Not yet investigated. Comment indicate this is an
old undocumented(?) feature that "will be removed".

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.