When using throttles with a long period, a porting issue from the JS
based throttle code meant that we'd compute a TTL in *seconds* but tell
redis that its value is in *milliseconds*, resulting in a much shorter
expiration time than desired, allowing more messages to pass the
throttling condition.
This issue doesn't apply to the redis-cell based throttle
implementation.
The fix here is simply to switch the `PX` (TTL in milliseconds) to
`EX` (TTL in seconds).
Possibly this was the source of the flakeyness that we could never
run down in https://github.com/KumoCorp/kumomta/issues/297
This is paired with a corresponding field in the memoize options
struct, which is in turn exposed to the shaping helper shaping_data
cache via the `allow_stale_shaping_data` field.
The effect of setting this to true is:
* Undoes the effect of invalidate_with_epoch, as stale reads cannot
be epoch based
* If a semaphore wait times out, and a stale value was present in
the cache, that stale value will be returned to the caller
To facilitate this behavior, the lruttl cache introduces a new
state for a cached value: `Refreshing`, which is a combination
of both `Present` and `Pending`.
When stale reads are allowed, we take care to avoid unilaterally
replacing `Present` with `Pending` when an item has expired.
Whenever we satisfy a lookup with a stale value, we will bump a new
lruttl_stale_count counter for that cache. Previously, we would
classify those as errors.
Something else that changed as part of this commit, is that we now set
an overall deadline for the semaphore wait operation; previously, each
wait would allow for up to the specified semaphore timeout, but now the
total wait time for that lookup will be bounded to the specified
timeout.
This commit continues in the theme of
18e510ebe2 to make us a bit more
discerning about how often the readyq maintainer is run.
In this case, we're adding an explicit delay in the maintainer itself
when there is a max_connection_rate defined for that particular site.
This will reduce overall cpu utilization in the case that the readyq
size suggests a larger number of connections than can be established
immediately, due to the max_connection_rate throttle.
As with the referenced earlier commit, this is an approximation; it may
cause more latency than is strictly required, but the consequences are
low because connection rates tend to have a short period, and the impact
is only a delay, not a suppresion of a wakeup. While the maintainer
task is delaying, any additional wakeups are queued and will effective
coalesce into a lower volume and rate of wakeups.
The throttling behavior is explicitly local-only, and doesn't use
the redis throttle backend, as the intent is to reduce the workload
rather than increase the workload.
TL;DR: you can easily halve your system performance by logging headers
vs. logging meta.
This is one of those things that is easy to overlook or forget,
but: whenever you need to operate on the message data, rather
than its metadata, the aggregate cost is high.
In this case, we were recently troubleshooting a system where
the CPU was bogged down and we traced it to the logging configuration: a
number of message headers were being logged in a configuration that
made heavy use of throttles and limits in its traffic shaping, and
thus had a large number of Delayed and TransientFailure events being
written to the logs.
When logging headers, each one of those events requires loading
the message from the spool and parsing out the headers. When the
average message size is ~100KB this imposes a notable overhead
on the CPU and IO utilization of the system.
What we recommend instead of logging headers directly is capturing
the information that you want to log into the message metadata
at the time that the message is received.
The message meta is usually already loaded, but is also typically
much smaller and easier to decode than the full message content
in the cases where it is not loaded.
As a result, it is much cheaper to log meta than to log headers.
This commit adds some warnings and cross links to help folks
be aware of this, and to generally navigate related meta and logging
topics more easily via tags.
This simplifies the overall build process.
ghcr.io/kumocorp/kumomta:main is the most recent commit to the
main branch
ghcr.io/kumocorp/kumomta:latest is the most recently tagged
stable commit.
I'll manually trigger a run like this to ensure that we actually
publish, then I will update the docs to reflect it.
I noticed that sometimes CI will fail with an error mentioning
the accounting db and a timeout. It appears as though that is
a secondary issue masking the true problem.
Revise the logic to use spawn_blocking around the inherently
blocking sqlite query operation so that we can reveal the
true issue.
These allow optionally reducing how aggressively the dispatcher and
maintainers will be awoken when message(s) are submitted to the ready
queue.
The default behavior remains the same; the new thing here is the
ability to make it more relaxed, which should reduce some CPU
overheads for very busy systems with many queues.
Making things more relaxed does introduce a possibility for higher
outbound latency in some edge cases with low or bursty traffic.
This is to help humans grok a diagnostic log file a bit better.
This isn't a supported product surface and isn't deployed with
kumomta. It is intended primarily as a log debugging aid.
If you have a queue that throttles that rate at which messages can move
into the ready queue, when message(s) for that queue are ticked out of
the singleton wheel, they can block the reinsertion of subsequently
ticked messages because the insertion call is "blocking" (puts back
pressure on) the caller.
That's fine in the hot path where the insertion is coupled with the
incoming protocol session, but for the wheel tick we need to guarantee
that we're making progress with the rest of the messages.
This commit breaks apart the ticked batch by queue and spawns a new
tokio task per queue. That way a throttle on any given queue will only
affect the rest of the messages in that particular queue (which it will
do anyway), and not prevent the other queues from making progress.
These measures the difference between the due time and the current
time at the point where a message is popped from the singleton
wheel (timeq_pop_tardiness) and the time when we initiate a
call to reinsert the message into the ready queue
(timeq_reinsert_tardiness).
Since the wheel ticks every 3 seconds, we typically expect to
see the maximum value in timeq_pop_tardiness being ~3 seconds.
timeq_reinsert_tardiness should not generally be significantly
larger than this, but since there is some uncertainty, we need
this metric.
Make things a bit more compact while still informative when
displaying the size of the different parts of the state on
both startup and shutdown.
Similarly, show when we prune records on startup.
We were blanket-applying the text charset conversion and it was
interpreting the binary bytes as windows-1252 content, which could cause
us to then re-encode the content as the utf8 equivalent of the 1252
interpretation of those bytes.
This moves another set of data from the sqlite db and into our
own data structures.
The sqlite data will be copied into the new structure on startup
if we haven't already populated that data.
In a pathological configuration (essentially unbounded and continually
increasing number of campaigns, tenants, coupled with a synthetic
10% rate of triggering drastic bounce automation rules) I observed
that tsa could trip over itself when locking the sqlite db to
maintain the event history table. This in turn causes delays
in responding to the shaping data endpoint, leading to timeout
errors in kumod.
This commit removes the sqlite-based event_history table that was
the source of that contention and replaces it with a much more
compact and easier to reason about set of in-memory data structures
built around a sharded hash map. This makes it very quick to
insert and return the current count when assessing rules with
triggering thresholds.
This new state is serialized using msgpack and periodically saved
in the background, as well as on shutdown.
This is lingering from the past; we don't need LocalSet any
more. I thought this might be causing some scheduling issues,
but it was neutral overall, so this commit is really just
a code cleanup.
For sites with large numbers of admin bounce entries, it is
desirable to negatively cache the lookup so that we can avoid
extraneous matching overhead.
We use a generation counter to determine if any rules have changed
and allow that to drive the invalidation of negative caching.
Otherwise, we cache the last lookup and, if the generation is the
same, then we cache the prior result and respect its expiration.
After running this at a couple of sites, this appears to be
good overall, so let's remove the option, reduce complexity
and make things better by default.
I noticed on a busy system with over 1500 active admin bounce
entries (created via automation) that cpu was quite busy in qmaint.
I believe that is because we were potentially spawning a new
async qmaint task for every message moving from the wheel and into
the ready queue.
This commit removes that logic, and replaces it with a per-message
check in insert_ready.
Queue insertion can generate log records if queues are full,
so holding a backlog permit could lead to blocking-like
behavior when the system is relatively saturated.
Let's release the permit prior to insertion; that way
it still provides some benefit (limiting the should-enq hook
calls) while not getting in the way.
Wez Furlong
Kyle Renfrow