Oximeter: Cache FieldSet values.

[jmcarp]

Oximeter spends a meaningful chunk of cpu time, and a meaningful number of allocations, parsing FieldSet values from incoming json. Because we encounter the same FieldSet values many times each, we can avoid a measureable amount of work by caching parsed FieldSet values, keying on unparsed RawValue values.

Note: I'm marking this as a draft until I can also check in the benchmarking scripts I've been using to test this. So far, oximeter cpu/memory use look significantly better with this change, as does throughput, which should also help us #10552. But I want to be a bit more rigorous about the benchmarks before I call this change ready.

[jmcarp]

Results, comparing this branch to main under a few scenarios:

shape	procs	series	batch	interval	churn	n	RSS p50: base → cache	Δ	cores: base → cache	Δ
mgs-lowrate	2	4,900	1	10 s	—	2	154 ± 2 → 141 ± 1	−8%	0.07 → 0.04	−43%
mgs-mid	2	4,900	1	4 s	—	2	195 ± 17 → 144 ± 2	−26%	0.18 → 0.10	−44%
mgs	2	4,900	10	10 s	—	3	1529 ± 69 → 494 ± 30	−68%	0.51 → 0.16	−69%
mgs-churn	2	4,900	10	10 s	60 s	3	1428 ± 401 → 519 ± 40	~−60%	0.43 → 0.17	−61%
4x	20	2,000	1	1 s	60 s	2	917 ± 4 → 365 ± 13	−60%	1.49 → 0.93	−38%

Load generation comes from #10925, which attempts to simulate somewhat realistic metrics, based on the mgs producer, which is by far the highest-volume individual producer. We vary series cardinality, number of simulated producers, number of samples emitted per series per collection, to simulate different scenarios, with "mgs" being based observed load of mgs on colo. I also had claude write a harness for running and observing these tests at https://gist.github.com/jmcarp/728c7b4693446337708bb35902bd79f1—still deciding how/whether to clean them up and check them in.

My high-level summary is that caching FieldSet values consistently reduces cpu time across scenarios, and reduces memory use specifically for high-throughput scenarios. The cpu drop sort of doesn't matter, since oximeter cpu use is generally pretty low, but increasing throughput does help us avoid saturating the database queue. The drop in memory use probably matters more, since @hermanol and I just found a rack in the field using ~16.5gib for oximeter. Caching an Arc<FieldSet> also means that multiple destinations, including clickhouse-single and clickhouse-clustered today, but potentially involving a prometheus cache or a metrics push feature in the future, won't require cloning the full sample across each destination.

[bnaecker]

I just found a rack https://github.com/oxidecomputer/customer-support/issues/1428#issuecomment-5106206619 using ~16.5gib for oximeter

This is a good change, and I think it's good to keep pushing on it. But I want to clarify that this 16.5GiB number is for the zone, not the oximeter process itself. On dogfood (which may not be representative), the zone consumes about 25GiB, while oximeter consumes about 5GiB of that. So we won't see overall savings of 60%, more like 15%.

root@oxz_oximeter_58582c15:~# pgrep -l oximeter
29243 oximeter
root@oxz_oximeter_58582c15:~# prstat -p 29243 -c 1 1
Please wait...
   PID USERNAME  SIZE   RSS STATE  PRI NICE      TIME  CPU PROCESS/NLWP
 29243 root       26G 5324M sleep   59    0  82:54:16 0.3% oximeter/131
Total: 1 processes, 131 lwps, load averages: 0.32, 0.28, 0.36
root@oxz_oximeter_58582c15:~#
logout

[Connection to zone 'oxz_oximeter_58582c15-21f4-4778-90d6-0b77887bc936' pts/6 closed]
BRM42220016 # zonestat -z oxz_oximeter_58582c15-21f4-4778-90d6-0b77887bc936 5 1
Collecting data for first interval...
Interval: 1, Duration: 0:00:05
SUMMARY                                                     Cpus/Online: 128/128    Physical: 767G    Virtual: 1023G
                                                         ----------CPU---------- ----PHYSICAL----- -----VIRTUAL-----
                                                    ZONE  USED %PART  %CAP %SHRU  USED   PCT  %CAP  USED   PCT  %CAP
                                                 [total]  7.50 5.85%     -     -  714G 93.0%     -  767G 74.9%     -
                                                [system]  1.67 1.31%     -     -  693G 90.2%     -  694G 67.8%     -
       oxz_oximeter_58582c15-21f4-4778-90d6-0b77887bc936  0.64 0.50%     -     - 5446M 0.69%     - 25.9G 2.53%     -

[bnaecker]

Something about this doesn't quite make sense to me. I don't mean it's wrong or unhelpful. I think I'm mostly surprised at the effect size.

IIUC, we're creating a separate cache for each producer. So consider, MGS, our most prolific producer. The timeseries that MGS produces, say hardware_component:temperature, have their fields derived from completely static data, like the chassis_model or the sensor name. Unless I'm really missing something, that implies we should have a cache miss on the first collection, and never again. I think that we should very quickly see cache misses drop to zero, potentially after the very first collection. But that means the cache is static and storing all the field sets we parsed out before anyway, right? This is an extreme example, but I think the pattern in general holds: most producers I'm aware of don't change their fields very often, if ever. Why does the cache help us so much?

It seems like the most probable answer is that parsing the JSON fields consumes a bunch of memory, and that because those are all randomly distributed in time for each producer, the overall / average memory consumption is high. Can we prove that? If so, can we change that in a simpler way, say by using a different parsing crate?

[bnaecker]

Let's add a comment on this.

[bnaecker]

I feel like this whole type should be just in the oximeter-collector crate. It's only consumed there, and never appears in the public API. It's really an implementation detail of that program.

[bnaecker]

I'd probably remove this and #[derive(Default)] instead.

[bnaecker]

Can we track the change in cache size before and after this? It would be helpful to know if we're either (1) missing out on an opportunity by not holding cache entries longer or (2) if the cache size is constant, in which case we should dig more :)

[bnaecker]

I think the Box<str> is fine, but a little surprising (to me, anyway). Probably worth a comment about why this over a String.

[bnaecker]

I would love to see this log message on a real deployment, or as close as we can get.

[bnaecker]

I missed this on the first review. This explains why the cache actually helps, because every sample from one producer shares the same FieldSet. When each collection task pushes those samples onto the database-insertion queue, there will be lots of sharing here. This all makes more sense now. The cache is really there to reduce the duplicated fields on the insertion buffer, not inside each collection task itself.

[bnaecker]

I think the new types in this file should probably move to the oximeter-collector crate too, since it's also an implementation detail IMO.

[daniel-levin]

Drive-by comment: I have opened #10954 to track the general chattiness of Oximeter.