elastic / apm

Elastic Application Performance Monitoring - resources and general issue tracking for Elastic APM.
https://www.elastic.co/apm
Apache License 2.0
384 stars 114 forks source link

Ensure a minimum retry interval of 5 seconds in fetching central configuration #690

Open estolfo opened 2 years ago

estolfo commented 2 years ago

Description

The central config spec has recently been updated to say that regardless of what the Cache-Control headers are, agents should only retry at a maximum every 5 seconds to fetch the central configuration. I.e. if the Cache-Control headers specify a value less than 5, the default retry interval should be used.

Agent Issues

beniwohli commented 2 years ago

A question that came up while implementing this. The spec says

If the Cache-Control header is zero (or less than zero)

However, the relevant RFC says that max-age is of type delta-seconds:

The delta-seconds rule specifies a non-negative integer, representing time in seconds.

So, negative values aren't allowed. Is the intention of the spec to behave reasonably in the face of misbehaving servers/proxies, or could we simplify this a bit?

FWIW, the Python agent behaves as spec'd by accident, as the regex fails to parse negative values and falls back to the default interval of 300s.

felixbarny commented 2 years ago

Is the intention of the spec to behave reasonably in the face of misbehaving servers/proxies, or could we simplify this a bit?

As guarding against negative values doesn't seem to increase the complexity, it seems like a good idea to do that.

FWIW, the Python agent behaves as spec'd by accident, as the regex fails to parse negative values and falls back to the default interval of 300s.

This sounds like a valid way to handle it 👍