chore(antithesis): Bound contexts in runs - #2216
Conversation
dd-octo-sts
Bot
added
the
area/test
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Binary Size Analysis (Agent Data Plane)Baseline: 20f18cf · Comparison: 39f3b2a · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
This comment has been minimized.
This comment has been minimized.
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sampling three independent per-kind maxima permits the intake to retain up to 3,000,000 contexts, rather than the stated 1,000,000 total bound, so long runs can still exceed the memory and cardinality budget this change is intended to enforce.
🤖 Datadog Autotest · Commit 34de63f · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| #[must_use] | ||
| pub fn sample<R: Rng + ?Sized>(rng: &mut R) -> Self { | ||
| Self { | ||
| metric_contexts: sample_cap(rng), |
There was a problem hiding this comment.
Antithesis runs can still exceed the intended memory and cardinality budget by three times, undermining the context-bounding change.
Assertion details
- Input: A sampled context source whose metric, event, and service-check caps each reach 1,000,000, followed by enough requests to fill all three pools.
- Expected:
The shared pool retains at most 1,000,000 contexts in total, matching the PR's stated bound. - Actual:
Each of the three caps is sampled independently up to 1,000,000, and Pool stores a separate vector for each kind, permitting 3,000,000 retained contexts.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (5)Experiments configured
Bounds Checks: ✅ Passed (5)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34de63f221
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// forwards. Content carries delimiters, so a raw render may occasionally drop; the repair | ||
| /// loop is the sorter and must always yield a forwardable line. | ||
| #[test] | ||
| fn render_wellformed_always_forwards(seed: u64, kind in any_kind()) { |
There was a problem hiding this comment.
property_test_
The three new proptests in this module, plus empty_pool_yields_empty_payload in payload/dogstatsd.rs, lack the required literal property_test_ prefix. The Makefile filters on that name, so these tests run in the ordinary debug-mode test job and are omitted from the dedicated release-mode test-property job; rename all four new property-test functions so they receive the intended coverage.
AGENTS.md reference: AGENTS.md:L39-L41
Useful? React with 👍 / 👎.
| return context; | ||
| } | ||
| } | ||
| Context::Metric(metric::MetricContext::fallback()) |
There was a problem hiding this comment.
When all remint attempts are exhausted for an event or service check, this unconditional metric fallback is inserted into the event or service-check vector selected by Pool::serve. On those random branches, later requests for that kind therefore return metrics instead, and the actual number of metric identities can exceed metric_contexts while the advertised event/service-check working set is missing an entry; provide a valid fallback for each requested Kind rather than changing the variant.
Useful? React with 👍 / 👎.
e4ff7fa to
f89d2be
Compare
34de63f to
a827899
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a82789926a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| out.truncate(start); | ||
| } | ||
| out.extend_from_slice(FALLBACK_LINE); |
There was a problem hiding this comment.
When all eight per-occurrence render attempts are malformed—for example, repeated delimiter-bearing values for a pooled set metric—this emits the fixed harness.fallback metric rather than the context fetched from /contexts. That introduces a metric identity outside the shared pool and can make the observed metric inventory exceed its configured cap; unlike the already-flagged mint-time fallback, this happens later while rendering an otherwise accepted pooled context. Use a well-formed rendering that preserves the selected context's identity instead.
Useful? React with 👍 / 👎.
| metric_contexts: sample_cap(rng), | ||
| event_contexts: sample_cap(rng), | ||
| service_check_contexts: sample_cap(rng), |
There was a problem hiding this comment.
When the independently sampled caps are large, this configuration permits up to one million metrics, one million events, and one million service checks, so the pool's total bound is three million contexts rather than the intended one-million run-wide ceiling. Because the pool retains every minted context and each can carry several kilobytes of nested byte vectors, long runs can consume several times the memory implied by the advertised cap; sample or enforce a shared total budget instead of applying the full ceiling independently to every kind.
Useful? React with 👍 / 👎.
a827899 to
e45d94e
Compare
f89d2be to
07e1995
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e45d94e437
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }; | ||
| let mut probe = Vec::new(); | ||
| context.render(rng, &mut probe); | ||
| if probe.len() <= MAX_CONTEXT_RENDER && is_malformed(&probe).is_ok() { |
There was a problem hiding this comment.
When dogstatsd_buffer_size is sampled below 6,000 bytes (it can be as low as 128), this accepts contexts whose stable name, tags, or options already exceed the driver's payload_byte_limit. write_payload then rejects the first rendered line before adding anything; with the commonly sampled one-context working set, every configured datagram is empty and the invocation silently generates no load. Pass the sampled payload limit into context admission or otherwise ensure every served context has a rendering that fits that timeline's limit.
Useful? React with 👍 / 👎.
| 1 => (b"c:", common::optional_text(rng)), | ||
| 2 => (b"e:", common::optional_text(rng)), | ||
| _ => (b"card:", common::optional_text(rng)), |
There was a problem hiding this comment.
When one of these generated extension bodies contains |#..., which is possible because optional_text deliberately samples protocol delimiters, the parser treats the suffix as a second tag extension after self.tags and replaces the pooled tag set with it. The service-check message has the same problem because it is also delimiter-bearing and serialized after its fixed tags. A recurring pooled context can therefore produce different actual identities on each render, growing observed cardinality beyond the configured pool cap and defeating multi-point recurrence; prevent per-occurrence fields from injecting extension separators or pool the resulting parsed identity.
Useful? React with 👍 / 👎.
| /// Distinct metric contexts the pool holds. | ||
| pub metric_contexts: usize, |
There was a problem hiding this comment.
This public YAML field and its two sibling caps only state what they count; they do not document the generated default/range, accepted boundary values, behavioral impact, or guidance for changing them. In particular, Deserialize accepts 0 even though serving that kind then evaluates random_range(0..0) and panics, so a consumer cannot determine the safe contract from the API. Document the 1..=1_000_000 generated behavior, unsupported or validated edges, and the memory-versus-recurrence trade-off.
AGENTS.md reference: AGENTS.md:L149-L154
Useful? React with 👍 / 👎.
07e1995 to
b943695
Compare
e45d94e to
cb1f9df
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb1f9df376
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Distinct contexts a driver invocation fetches from the shared pool as its working set, each | ||
| /// boundary-biased log-uniform in `1..=MAX_WORKING_SET`. | ||
| pub context_count: usize, |
There was a problem hiding this comment.
context_count boundaries
When a restored or hand-authored driver.yaml sets context_count to 0 or above 65_536, DriverConfig::read accepts it, but the intake rejects every /contexts request; the shared fetch loop then waits for its 30-second budget and both Antithesis drivers return empty stats without generating DogStatsD load. Document the accepted boundaries, invalid-value behavior, and workload-size trade-off here, or validate the field during deserialization, rather than describing only the range produced by the sampler.
AGENTS.md reference: AGENTS.md:L149-L154
Useful? React with 👍 / 👎.
b943695 to
a6dd29d
Compare
cb1f9df to
639f6b7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 639f6b7fee
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let ext_count = EXT_COUNTS[rng.random_range(0..EXT_COUNTS.len())]; | ||
| for _ in 0..ext_count { | ||
| out.push(b'|'); | ||
| ext_chunk(rng, out); |
There was a problem hiding this comment.
When EXT_COUNTS selects 127 or 255, the freshly generated extension bodies can make a later rendering exceed MAX_CONTEXT_RENDER even though the same identity passed minting with a shorter probe render. write_payload then stops at that first non-fitting line and, if it was the first line, sends an empty datagram; this silently discards configured load for sampled receive limits and can occur even at the 8,192-byte ceiling. Enforce the size bound on each render or retry/skip an oversized occurrence instead of ending the payload.
Useful? React with 👍 / 👎.
639f6b7 to
39f3b2a
Compare
212be47 to
57f3d76
Compare
39f3b2a to
a8467bf
Compare
57f3d76 to
170d7ed
Compare
a8467bf to
75b7b70
Compare
This commit introduces a new protocol to the intake node to allow for context bounding in runs. The purposes are twofold. First, while it's interesting to explore timelines in which there are too many contexts for either lane to handle these are less relevant to me _now_, so I've capped total contexts at 1M. Second, in order for timeseries equality to _work_ I need to be certain that I have at least _some_ timeseries with more than one point emitted into them. Previously drivers were emitting essentially random contexts and it was very unlikely for more than one point to be emitted per context. Oops. Drivers now request N contexts from the intake service for each sub-kind of dogstatsd they emit, meaning all contexts are sourced from a single spot in a topology and not from pure randomness.
170d7ed to
56c2a83
Compare
75b7b70 to
9adb823
Compare
1 participant
Summary
This commit introduces a new protocol to the intake node to allow for
context bounding in runs. The purposes are twofold. First, while it's
interesting to explore timelines in which there are too many contexts
for either lane to handle these are less relevant to me now, so I've
capped total contexts at 1M. Second, in order for timeseries equality
to work I need to be certain that I have at least some timeseries
with more than one point emitted into them. Previously drivers were
emitting essentially random contexts and it was very unlikely for more
than one point to be emitted per context. Oops.
Drivers now request N contexts from the intake service for each sub-kind
of dogstatsd they emit, meaning all contexts are sourced from a single
spot in a topology and not from pure randomness.
Change Type
How did you test this PR?
References