SVLS-8857: serverless-init on the Fx forwarder/demux/DogStatsD bundle#52329
Draft
apiarian-datadog wants to merge 11 commits into
Draft
SVLS-8857: serverless-init on the Fx forwarder/demux/DogStatsD bundle#52329apiarian-datadog wants to merge 11 commits into
apiarian-datadog wants to merge 11 commits into
Conversation
Add forwarder_stop_wait_for_inflight (default false, preserving existing behavior): when set, Worker.Stop waits for in-flight HTTP transactions to complete instead of cancelling them. This gates the new shutdown-flush behavior used by serverless-init. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When forwarder_stop_wait_for_inflight is set, Worker.Stop signals the Start goroutine via a dedicated stopChan, lets in-flight HTTP requests complete, and cancels workerCtx last — so a final flush is not aborted by a racing context cancel. The outer bound remains forwarder_stop_timeout. When the flag is unset (default), Stop keeps the original cancel-immediately semantics. Tests are rewritten to drive Stop concurrently and assert the wait/cancel behavior; the testTransaction block primitive moves from shouldBlock to a release channel that models an in-flight request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When dogstatsd_flush_incomplete_buckets is enabled, each worker flushes its batcher into the time sampler as it exits its run loop on stop. stop() closes stopChan and then waits on a workerWg so every worker's flush completes before the demultiplexer is torn down — guaranteeing the batched samples reach the sampler before its own final flush, with no dropped metrics on the shutdown of a short-lived process (serverless-init). The flush is gated by the existing dogstatsd_flush_incomplete_buckets setting rather than a new key: that setting already flushes the sampler's final bucket, and draining the server batchers one layer upstream is the same "report everything we have on stop" intent. Long-running agents leave it off and are unaffected. The legacy ServerlessFlush mechanism is left in place for now; it is removed once serverless-init stops using it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add StopTimeout to StartServerlessTraceAgentArgs (defaults to 3s when zero) so callers can bound how long Stop waits for the trace agent's Run loop to exit. serverless-init uses this to keep shutdown within the platform grace window. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Shrink ServerlessMetricAgent to a demultiplexer plus the three tag sets it applies (bundled in a new Tags struct), and add New() to construct it. The agent no longer builds its own demultiplexer/forwarder or DogStatsD server — those are now Fx-wired and injected. Add a metricstest package providing the Fx bundle used by the serverless metric tests, and update the metric/otlp tests to construct the agent on top of it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Construct the forwarder, demultiplexer and DogStatsD server via Fx and rework shutdown so the OnStop cascade (dsdServer.stop -> demux.Stop -> forwarder.Stop) drains and flushes the final samples instead of dropping them. preloadEarly applies the config overrides the pipeline reads at construction time (forwarder_stop_wait_for_inflight, dogstatsd_flush_on_stop, dogstatsd_flush_incomplete_buckets, stop timeouts), and the explicit ServerlessFlush/last-flush orchestration is replaced by the Fx lifecycle. CloudService.Shutdown now takes a nilable *ServerlessMetricAgent (nil when no API key is configured) and the ShouldForceFlushAllOnForceFlushToSerializer hook is removed. log.CreateConfig takes an explicit flush timeout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
With serverless-init on the standard Fx pipeline, the legacy serverless-only code paths are dead. Remove them: - pkg/aggregator: ServerlessDemultiplexer (and its test) - comp/forwarder/defaultforwarder: SyncForwarder - comp/dogstatsd/server: NewServerlessServer / ServerlessDogstatsd and the ServerlessFlush interface method + serverlessFlushChan worker path - comp/dogstatsd/serverDebug: NewServerlessServerDebug - pkg/serverless: the FlushableAgent interface DogStatsD shutdown flushing now goes solely through the dogstatsd_flush_on_stop per-worker path; the old externally-driven ServerlessFlush is gone. BUILD.bazel files are updated to drop the removed sources. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
@codex review |
Contributor
|
Contributor
Go Package Import DifferencesBaseline: 5a863ec
|
Contributor
Files inventory check summaryFile checks results against ancestor 5a863ec0: Results for datadog-agent_7.82.0~devel.git.96.556a749.pipeline.119175539-1_amd64.deb:No change detected |
Contributor
Author
Contributor
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
11 successful checks with minimal change (< 2 KiB)
|
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: 5a863ec Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +0.68 | [-0.38, +1.74] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | -0.08 | [-0.14, -0.03] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | -0.21 | [-0.25, -0.17] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | -0.63 | [-0.88, -0.38] | 1 | Logs bounds checks dashboard |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gate_idle | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 144.52MiB ≤ 147MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 579.70KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 3 ≤ 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 478.34MiB ≤ 495MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 0.89MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 3 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 184.22MiB ≤ 195MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 264.18MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 333.24 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 4 ≤ 6 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 384.54MiB ≤ 430MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | total_bytes_received | 10/10 | 0.86GiB ≤ 1.04GiB | bounds checks dashboard |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
Replicate Execution Details
We run multiple replicates for each experiment/variant. However, we allow replicates to be automatically retried if there are any failures, up to 8 times, at which point the replicate is marked dead and we are unable to run analysis for the entire experiment. We call each of these attempts at running replicates a replicate execution. This section lists all replicate executions that failed due to the target crashing or being oom killed.
Note: In the below tables we bucket failures by experiment, variant, and failure type. For each of these buckets we list out the replicate indexes that failed with an annotation signifying how many times said replicate failed with the given failure mode. In the below example the baseline variant of the experiment named experiment_with_failures had two replicates that failed by oom kills. Replicate 0, which failed 8 executions, and replicate 1 which failed 6 executions, all with the same failure mode.
| Experiment | Variant | Replicates | Failure | Logs | Debug Dashboard |
|---|---|---|---|---|---|
| experiment_with_failures | baseline | 0 (x8) 1 (x6) | Oom killed | Debug Dashboard |
The debug dashboard links will take you to a debugging dashboard specifically designed to investigate replicate execution failures.
❌ Retried Profiling Replicate Execution Failures (ddprof)
Note: Profiling replicas may still be executing. See the debug dashboard for up to date status.
| Experiment | Variant | Replicates | Failure | Debug Dashboard |
|---|---|---|---|---|
| quality_gate_idle_all_features | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_idle_all_features | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_logs | comparison | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | baseline | 10 | Oom killed | Debug Dashboard |
| quality_gate_metrics_logs | comparison | 10 | Oom killed | Debug Dashboard |
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_idle_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_idle_all_features, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check total_bytes_received: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_metrics_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Moves serverless-init off its bespoke metrics stack (
ServerlessDemultiplexer,NewServerlessServer,SyncForwarder) and onto the standard Fx forwarder/demultiplexer/DogStatsD bundle the core agent uses. Lifecycle is owned by Fx: components start duringfxutil.OneShotand theirOnStophooks fire in reverse construction order on shutdown (DogStatsD server → demultiplexer → forwarder). The old explicit orchestration (Flush,WaitForPendingSamples,Stop) is gone; the equivalent draining now happens inside each component's stop hook, gated by config so only serverless-init opts in.This is the serverless-init follow-up to the upstream metrics-agent changes; it also removes the now-unused bespoke serverless plumbing.
Motivation
Simplify serverless-init and handle shutdown correctly in ephemeral environments (e.g. Cloud Run jobs) where shutdowns happen often. Splits the original #52267 into reviewable chunks.
Stacked PR — keep as draft until the upstream PRs merge
This branch is stacked on the two upstream PRs and will stay a draft until they land:
forwarder_stop_wait_for_inflight(forwarderWorker.Stop)dogstatsd_flush_incomplete_buckets,workerWg)Until those merge, this PR's diff includes their commits. Once they merge, rebase onto
mainand the diff collapses to the serverless-init changes only.Relative to #52267, serverless-init now gates the shutdown flush on the single
dogstatsd_flush_incomplete_bucketssetting (the newdogstatsd_flush_on_stopkey was dropped per review), and thepreloadEarly/shutdown-sequence comments are updated to describe theworkerWgcompletion barrier.Describe how you validated your changes
dda inv test --targets=./cmd/serverless-init— 95 passdda inv test --targets=./pkg/serverless/metrics,./pkg/serverless/trace— passdda inv test --targets=./comp/dogstatsd/server/impl— 247 passdda inv linter.goon./cmd/serverless-init— clean🤖 Generated with Claude Code