kafka_consumer: store broker_timestamps as marshal instead of json#24471
Conversation
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: cb18e07 | Docs | Datadog PR Page | Give us feedback! |
83d9caa to
aa959ec
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa959ece3d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for topic_partition, content in json.loads(self.read_persistent_cache(persistent_cache_key)).items(): | ||
| for offset, timestamp in content.items(): | ||
| broker_timestamps[topic_partition][int(offset)] = timestamp | ||
| broker_timestamps.update(pickle.loads(base64.b64decode(self.read_persistent_cache(persistent_cache_key)))) |
There was a problem hiding this comment.
Avoid unpickling the persistent cache
When the Agent reads a cache file from its persistent cache directory, this now deserializes it with pickle.loads; if that cache entry is ever writable by another local process/container sharing the Agent run volume, the next kafka_consumer check run will execute attacker-controlled pickle opcodes under the Agent process. This cache previously used json.loads, so tampering could only corrupt the cache; use a non-executing binary format such as msgpack/struct-packed data, or authenticate the blob before unpickling.
Useful? React with 👍 / 👎.
aa959ec to
9ef88e1
Compare
The DSM broker_timestamps cache was serialized with json.dumps on every check run. At high partition counts json.dumps is the dominant allocation source (profiler flagged _save_broker_timestamps -> json.dumps -> iterencode as the top allocator, feeding glibc arena fragmentation and CCR OOMs, #incident-57455). Persist with marshal (base64-wrapped to ride the string persistent-cache API) instead. marshal is a compact binary codec that avoids JSON's per-number text encoding, cutting serialization ~100x on the save path with no change to emitted metrics. The on-disk format changes and is version-specific, so a cache written by an older agent is discarded once (rebuilt next run). Tests seed and read the cache in the marshal format with integer offset keys. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9ef88e1 to
cb18e07
Compare
Validation ReportAll 21 validations passed. Show details
|
What does this PR do?
Persists the
kafka_consumerDSMbroker_timestampscache withmarshal(a compact binary codec, base64-wrapped so it still rides the string-based persistent-cache API) instead ofjson.No change to emitted metrics —
kafka.estimated_consumer_lagand friends are computed from the same data.Motivation
Cluster-check runners running
kafka_consumerwithenable_cluster_monitoringon large clusters have been OOMing (#incident-57455). Investigation (repro pod + live/proc+ the Datadog continuous profiler) showed the growth is glibc malloc fragmentation fed by very high allocation churn — not a leak. The profiler's top Python allocator was_save_broker_timestamps -> json.dumps -> iterencode: every run re-serialized the entire (growing) timestamps dict, and JSON encodes every offset/timestamp as text.Benchmarked at the incident's real cardinality (31,870 partitions), the
broker_timestampsserialization on the save path dropped ~100x withmarshal(and per-run load/save churn dropped several-fold overall).marshalkeeps integer offsets and float timestamps native, avoiding JSON's per-number text encoding.This is the source-side half of the mitigation; the allocator-side half (
MALLOC_ARENA_MAX=2on the runners) is a separate ops PR.Why
marshal(and notpickle)picklewould be faster still, but it executes arbitrary opcodes on load — deserializing an on-disk cache file withpickle.loadsturns a cache-tampering risk into remote code execution in the Agent process if anything else can write the run volume.marshalonly (de)serializes basic built-in types and cannot construct/execute objects, so a tampered cache cannot run code (worst case is a crash — a large step down from RCE).marshalis also stdlib, so it adds no dependency and keeps thetest-minimum-base-packagejob green (msgpack/orjsonare not guaranteed in the minimum base).Trade-offs, called out for review:
marshalis not hardened against maliciously-crafted input — a tampered blob could crash the check (DoS), but not execute code.marshalformat is Python-version-specific, so the cache is discarded once (and rebuilt) after a Python upgrade or a format change. Handled by thetry/exceptin_load_broker_timestamps.Review checklist (to be filled by reviewers)
test_unit.pyandtest_integration.pyto seed/read the cache in the marshal format with integer offset keys. Full unit suite (144) passes; lint clean.qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. →qa/required(agent-impacting runtime change; validate memory reduction + noestimated_consumer_lagregression on a live cluster).backport/<branch-name>label.