Configuration settings should take units in the value, not put units in the setting name #27
Assignees
Labels
Comments
|
Okay, I wrote a lot of these configuration settings, so I'd be happy to fix this. |
|
Sounds good to me. Do you think we ought to interpret these settings with a default unit in cases where one is not provided (so as to not break existing configs), or treat missing units as a parse error (cc @olix0r)? |
khappucino
pushed a commit
to Nordstrom/linkerd2
that referenced
this issue
Mar 5, 2019
Configuration values that take durations are currently specified as time values with no units. So `600` may mean 600ms in some contexts and 10 minutes in others. In order to avoid this problem, this change now requires that configurations provide explicit units for time values such as '600ms' or 10 minutes'. Fixes linkerd#27.
khappucino
pushed a commit
to Nordstrom/linkerd2
that referenced
this issue
Mar 5, 2019
Required for linkerd#1322. Currently, the proxy places a limit on the number of active routes in the route cache. This limit defaults to 100 routes, and is intended to prevent the proxy from requesting more than 100 lookups from the Destination service. However, in some cases, such as Prometheus scraping a large number of pods, the proxy hits this limit even though none of those requests actually result in requests to service discovery (since Prometheus scrapes pods by their IP addresses). This branch implements @briansmith's suggestion in linkerd#1322 (comment). It splits the router capacity limit to two separate, configurable limits, one that sets an upper bound on the number of concurrently active destination lookups, and one that limits the capacity of the router cache. I've done some preliminary testing using the `lifecycle` tests, where a single Prometheus instance is configured to scrape a very large number of proxies. In these tests, neither limit is reached. Furthermore, I've added integration tests in `tests/discovery` to exercise the destination service query limit. These tests ensure that query capacity is released when inactive routes which create queries are evicted from the router cache, and that the limit does _not_ effect DNS queries. This branch obsoletes and closes linkerd#27, which contained an earlier version of these changes. Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Consider
CONDUIT_PROXY_EVENT_BUFFER_CAPACITY. What units is this setting measured in? Currently the units are implicitly bytes, e.g.CONDUIT_PROXY_EVENT_BUFFER_CAPACITY=10000means 10,000 bytes. Instead of having implicit units, we should use explicit units so we can write, e.g.CONDUIT_PROXY_EVENT_BUFFER_CAPACITY=10kb(for base-2 kilobytes) orCONDUIT_PROXY_EVENT_BUFFER_CAPACITY=10000b(for bytes), etc.Similarly we have
CONDUIT_PROXY_METRICS_FLUSH_INTERVAL_SECS. This is better because the name of the setting gives the units so one doesn't have to guess. However, we've already run into a case during testing where we want to use a sub-second unit. This setting should instead by namedCONDUIT_PROXY_METRICS_FLUSH_INTERVALand accept values with units specified, e.g.10sfor 10 seconds or200msfor 200 milliseconds.Similar concerns apply to other settings, not just in the proxy, but also in the other components.
The text was updated successfully, but these errors were encountered: