AI-6485 Add buffered Datadog metrics delivery - #25274
Conversation
e2bbe08 to
35c9290
Compare
✅ Dispatcher tests: passed
✅ 855 passed · nothing failed Batches · ✅ batch-01 240/240 · ✅ batch-02 217/217 · ✅ batch-03 220/220 · ✅ batch-04 178/178 Dispatcher finished on |
evalya-impact-summaryevalya impact analysis |
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 2efcfad | Docs | View more details | Give us feedback! |
35c9290 to
98dbbb4
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98dbbb45e8
ℹ️ 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".
7c1ca11 to
0a654ac
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a654aced1
ℹ️ 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".
| def record(self, record: MetricRecord) -> None: | ||
| if self._closed: | ||
| return | ||
| try: | ||
| self._queue.put_nowait(record) |
There was a problem hiding this comment.
Serialize record enqueueing with shutdown
When record() races with close(), it can observe _closed == False, pause, and then enqueue after close() has inserted the sentinel. The worker exits when it reaches that sentinel, leaving the accepted record behind it permanently and reporting neither delivery nor a drop. This can lose an in-flight metric whenever runtime shutdown overlaps an emitter; protect the closed check and enqueue with the same shutdown synchronization or otherwise ensure records cannot be placed after the sentinel.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f801294e4d. The closed check, enqueue, and sentinel insertion share a lock in both exporters.
The workers also recheck the queue before exiting after an empty poll. Without that check, a record accepted between the poll and shutdown could still be skipped even though it was ahead of the sentinel. Coverage includes both interleavings and draining when a full queue prevents sentinel insertion.
| assert len(submitter.series) == 5 | ||
| assert len(submitter.metric_requests[0]) == 5 |
There was a problem hiding this comment.
Verify the intake options this test claims to preserve
These assertions only prove that five series were emitted in one request; a regression that discards or overwrites every tag, interval, unit, or metric kind would still pass. Assert the distinct serialized options for each series, or remove this redundant test, so it verifies the observable preservation behavior named by the test.
AGENTS.md reference: AGENTS.md:L202-L204
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The test only counted series and did not verify the behavior in its name. Removed it rather than duplicate existing coverage: the v2 payload test checks type, interval, and unit; the queued-record test checks timestamp and tags; the v1 payload test checks distribution serialization.
8996bd0 to
74f5795
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74f5795b75
ℹ️ 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".
| if record.kind is MetricKind.DISTRIBUTION: | ||
| distributions.add(self._distribution_series(record)) | ||
| elif record.kind is MetricKind.COUNT: | ||
| rejected = counts.add(record, time.monotonic(), time.time()) |
There was a problem hiding this comment.
Bucket counts by emission time
When the worker is blocked in a synchronous intake request, count records can remain queued for longer than COUNT_WINDOW_SECONDS; once the request returns, this line timestamps and groups every queued increment using the worker's current time rather than record.timestamp. Counts emitted across multiple ten-second periods can therefore be collapsed into one later window, distorting their timing and rate. Base count windows on emission time (or capture an emission-time monotonic value) instead of dequeue time.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f801294e4d. Count windows use sink acceptance time, captured under the same lock as enqueueing. Monotonic time determines window membership; the paired wall time timestamps the count point. A producer paused before acceptance joins the window that accepts it, and slow intake cannot move an accepted increment into a later window.
Idle expiry uses that same lock so it cannot close a window while a producer holds a captured timestamp but has not yet enqueued. Tests cover producer overtaking, that idle-expiry race, and a backlog spanning three windows behind blocked intake. Gauges and distributions keep their original emission timestamps.
f3931a7 to
695f39f
Compare
695f39f to
f801294
Compare
Validation ReportAll 21 validations passed. Show details
|
What does this PR do?
Adds a centralized, buffered Datadog metrics sink to
ddev.monitoring.count()as an increment, aggregating one total per series over a 10-second collection window based on sink acceptance time. Clock capture and enqueueing share a lock so concurrent producers cannot reorder collection times. Counts and gauges use Datadog v2; distributions use v1. Gauge and distribution points retain their emission timestamps.datadog-metricscomponent logger.--dry-runand--resolve-only.MonitoringRuntimeowns and drains its metrics sink before detaching caller-owned log handlers. The Dispatcher then drains the Datadog log handler it created. The application display remains the diagnostic fallback before runtime initialization.This adds infrastructure only, with no Dispatcher execution metric call sites. Count aggregation is local to one runtime; cross-process counter identity remains out of scope.
Motivation
AI-6485 needs a failure-isolated metrics transport before Dispatcher metric call sites are added.
Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged