consumption_metrics: next steps

[koivunej]

Next steps:

• remove the deduplication possibility, fix tests to expect all metrics to be always sent
• add more logging
• fix metric posting retry, align with proxy

PRs:

• feat(consumption_metrics): remove event deduplication support #5316
• fix(consumption_metrics): exp backoff retry #5317
• refactor(consumption_metrics): post-split cleanup #5327 or it's dependencies will handle adding logging

Rationale for deduplication removal

Before configuration change (private repo PR) we were running with configuration:

metric_collection_interval="X"
cached_metric_collection_interval="X"

This was thought to disable deduplication done in pageserver, but did not. Instead the cached_metric_collection_interval="0s" needs to be used. The deduplication was added originally as specified in #2941 in order to lower the load but since then the requirements have changed, and we are expected to send values even if there was no change.

We cannot just remove the cache, because the incremental metric MetricsKey::written_size_delta needs the previous timestamp it was sent and the previous MetricsKey::written_size.

At minimum we should fix the tests to reflect this new configuration and assert that all metrics are always sent. This is a bit tricky because synthetic_size will not be sent if it is zero.

Rationale for more logging

Currently we don't know anything about how the loop is doing, how long it is taking. We could add something like #5174 so next time it would be easy to rule out executors being very busy and forcing us to miss ticks, or posting just being very slow.

Rationale for retrying

In the logs we see mistakenly judged 502 Bad Gateway responses as "remote refused metrics" which are most likely due to redeploys. Instead we should retry these. It seems proxy is running with similar upload code, so perhaps we should both use something better.

[koivunej]

Now that I think of it, #3485 requested the opposite of deduplication at configurable intervals, so the dedup was always in from the start. From the original epic #2941:

One way to do that is to send info about all tenants on the start, cache values, and then filter out unchanged ones.

So the caching comes from there. Now it seems we no longer need it.

Description changed.