elastic / apm-nodejs-http-client

**Moved to elastic/apm-agent-nodejs.** A low-level Node.js HTTP client for communicating with the Elastic APM intake API
MIT License
21 stars 30 forks source link

fix: put [5s, 1d] range guard on the delay before re-fetching central config #185

Closed trentm closed 2 years ago

trentm commented 2 years ago

Refs: https://github.com/elastic/apm-agent-nodejs/issues/2941

The spec change requires a minimum 5s delay for refetching central config. The Node.js APM agent didn't have the "spin loop fetching central config from Cache-Control: max-age=0" issue. However, theoretically a large max-age value could overflow the max setTimeout delay (https://developer.mozilla.org/en-US/docs/Web/API/setTimeout#maximum_delay_value) which could result in a similar spin loop. I picked a maximum value of 1 day -- a large enough value that I cannot imagine it getting in a way of a legitimate max-age value from the APM server.

It might be worth adding a max guard to the spec.

apmmachine commented 2 years ago

:green_heart: Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

#### Build stats * Start Time: 2022-10-25T21:30:41.719+0000 * Duration: 9 min 45 sec #### Test stats :test_tube: | Test | Results | | ------------ | :-----------------------------: | | Failed | 0 | | Passed | 287 | | Skipped | 0 | | Total | 287 |

:robot: GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with: - `run` `elasticsearch-ci/docs` : Re-trigger the docs validation. (use unformatted text in the comment!)

trentm commented 2 years ago

Strings still result in NaN (per the previous algorithm), which is a little weird

Agreed. Thanks. I've improved it to return the default value for any non-positive number. From a test run:

# getCentralConfigIntervalS
ok 1 getCentralConfigIntervalS(-4) -> 300
ok 2 getCentralConfigIntervalS(-1) -> 300
ok 3 getCentralConfigIntervalS(0) -> 300
ok 4 getCentralConfigIntervalS(1) -> 5
ok 5 getCentralConfigIntervalS(2) -> 5
ok 6 getCentralConfigIntervalS(3) -> 5
ok 7 getCentralConfigIntervalS(4) -> 5
ok 8 getCentralConfigIntervalS(5) -> 5
ok 9 getCentralConfigIntervalS(6) -> 6
ok 10 getCentralConfigIntervalS(7) -> 7
ok 11 getCentralConfigIntervalS(8) -> 8
ok 12 getCentralConfigIntervalS(9) -> 9
ok 13 getCentralConfigIntervalS(10) -> 10
ok 14 getCentralConfigIntervalS(86398) -> 86398
ok 15 getCentralConfigIntervalS(86399) -> 86399
ok 16 getCentralConfigIntervalS(86400) -> 86400
ok 17 getCentralConfigIntervalS(86401) -> 86400
ok 18 getCentralConfigIntervalS(86402) -> 86400
ok 19 getCentralConfigIntervalS(86403) -> 86400
ok 20 getCentralConfigIntervalS(86404) -> 86400
ok 21 getCentralConfigIntervalS(null) -> 300
ok 22 getCentralConfigIntervalS(undefined) -> 300
ok 23 getCentralConfigIntervalS(false) -> 300
ok 24 getCentralConfigIntervalS(true) -> 300
ok 25 getCentralConfigIntervalS(a string) -> 300
ok 26 getCentralConfigIntervalS([object Object]) -> 300
ok 27 getCentralConfigIntervalS() -> 300