Add baseline Dynamic Tests for CNM - #54938
Conversation
|
@codex review |
github-actions
Bot
added
the
short review
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Files inventory check summaryFile checks results against ancestor a614467d: Results for datadog-agent_7.84.0~devel.git.272.3f6a287.pipeline.131208369-1_amd64.deb:No change detected Results for datadog-iot-agent_7.84.0~devel.git.272.3f6a287.pipeline.131208369-1_amd64.deb:No change detected |
|
@codex make a comprehensive code and security review Classify findings as P0 (critical), P1 (high), P2 (substantive), or P3 (optional). Include the priority, file, line, failure scenario, impact, and whether the finding is in scope for this PR. Focus especially on keeping the implementation minimal and feature-scoped, preserving existing behavior when baseline_tests.enabled is false, bounded-memory selection, one-shot/window semantics, and test evidence. |
github-actions
Bot
removed
the
short review
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Introduces an opt-in “baseline” mode for Cloud Network Monitoring (CNM) Dynamic Tests, enabling a small bounded set of one-shot path tests while keeping recurring Dynamic Tests disabled by default.
Changes:
- Add config/schema/docs/release notes for
network_path.connections_monitoring.baseline_tests.enabled. - Resolve effective Dynamic Tests state (off/baseline/standard) and wire it into system-probe module enabling and the network path collector scheduling logic.
- Add baseline candidate signal extraction + bounded baseline selector, plus payload metadata (
dynamic_test_profile) and related metrics/tests.
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| releasenotes/notes/add-cnm-baseline-dynamic-tests-ac7c4a3d73c865b4.yaml | Release note announcing baseline CNM Dynamic Tests behavior and config knob. |
| pkg/system-probe/config/config_test.go | Adds tests asserting Traceroute module enablement under baseline/standard dynamic test states. |
| pkg/system-probe/config/config.go | Enables Traceroute module based on resolved dynamic tests state and publishes derived sysprobe enablement. |
| pkg/system-probe/config/BUILD.bazel | Adds dependency on pkg/networkpath/config for state resolution. |
| pkg/process/checks/net.go | Populates baseline-ranking signals onto network path connections produced by process-agent. |
| pkg/networkpath/payload/payload_test.go | Adds serialization test for the new dynamic_test_profile JSON field. |
| pkg/networkpath/payload/pathevent.go | Introduces DynamicTestProfile and adds dynamic_test_profile to payload. |
| pkg/networkpath/payload/BUILD.bazel | Adds testify/assert dependency for new payload test. |
| pkg/networkpath/config/state_test.go | Adds unit tests for dynamic tests state resolution. |
| pkg/networkpath/config/state.go | New resolver combining core + sysprobe flags into an effective dynamic tests state. |
| pkg/networkpath/config/BUILD.bazel | Bazel targets for the new networkpath config package and tests. |
| pkg/network/sender/sender_linux.go | Populates baseline-ranking signals onto network path connections produced by system-probe sender. |
| pkg/config/setup/config_test.go | Ensures baseline flag default is false. |
| pkg/config/schema/yaml/core_schema.yaml | Adds schema for baseline_tests.enabled under connections monitoring. |
| pkg/config/example/datadog-agent_windows.yaml.example | Documents baseline tests config option in Windows example config. |
| pkg/config/example/datadog-agent_linux.yaml.example | Documents baseline tests config option in Linux example config. |
| comp/networkpath/npcollector/model/connection_test.go | Tests new baseline signal normalization and saturation behavior. |
| comp/networkpath/npcollector/model/connection.go | Adds baseline signal fields, saturation logic, and a shared SetBaselineSignals helper. |
| comp/networkpath/npcollector/model/BUILD.bazel | Adds go_test rule for the new model unit test. |
| comp/networkpath/npcollector/impl/pathteststore/pathteststore_test.go | Adds tests for one-shot dispatch/deletion and deadline expiry metrics. |
| comp/networkpath/npcollector/impl/pathteststore/pathteststore.go | Adds one-shot execution deadline handling, baseline metrics, and dedupe behavior for one-shots. |
| comp/networkpath/npcollector/impl/pathteststore/BUILD.bazel | Adds payload + teststatsd deps for new pathteststore tests. |
| comp/networkpath/npcollector/impl/npcollectorcomp.go | Resolves dynamic tests state via core+sysprobe config and emits a state metric. |
| comp/networkpath/npcollector/impl/npcollector_testutils_test.go | Injects sysprobe config mock to support state resolution in component tests. |
| comp/networkpath/npcollector/impl/npcollector_test.go | Updates expected emitted payloads and expected pathtests to include dynamic_test_profile. |
| comp/networkpath/npcollector/impl/npcollector.go | Implements baseline scheduling window + bounded selector, local-vs-remote filter selection, and emits baseline execution metrics. |
| comp/networkpath/npcollector/impl/config_test.go | Updates config enablement tests to use dynamicTestsState instead of connections monitoring boolean. |
| comp/networkpath/npcollector/impl/config.go | Replaces connections-monitoring enable flag with an effective dynamicTestsState, and adds baseline window duration. |
| comp/networkpath/npcollector/impl/common/pathtest.go | Adds baseline metadata (profile, one-shot, execution deadline) to pathtest model. |
| comp/networkpath/npcollector/impl/baseline_window_test.go | Adds tests for baseline window behavior and filter bypass semantics. |
| comp/networkpath/npcollector/impl/baseline_selector_test.go | Adds unit tests and benchmark for bounded baseline selector behavior. |
| comp/networkpath/npcollector/impl/baseline_selector.go | New bounded baseline selector implementation using xxhash + space-saving style replacement. |
| comp/networkpath/npcollector/impl/BUILD.bazel | Adds new selector sources/tests and deps (xxhash, networkpath/config, sysprobeconfig). |
| comp/metadata/inventoryagent/impl/inventoryagent_test.go | Adds baseline tests flag to inventoryagent feature reporting tests. |
| comp/metadata/inventoryagent/impl/inventoryagent.go | Reports baseline tests enabled state in inventory metadata payload. |
| comp/metadata/inventoryagent/README.md | Documents the new inventory feature field for baseline tests. |
Suppressed comments (1)
pkg/system-probe/config/config.go:1
- This mutates
system_probe_config.enabledtwice during load, once purely to influenceResolveDynamicTestsState(), then again for the final value. That side-effect makes load ordering harder to reason about and increases risk if other code reads config during initialization. A cleaner approach is to avoid intermediatecfg.Set(...)and instead pass the derived sysprobe-enabled value into the resolver (e.g., via a small Reader wrapper that overridesGetBool(systemProbeKey)), then setsystem_probe_config.enabledexactly once after all modules (including traceroute) are finalized.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
[AI] review-fix-loop: blocked — 1 iteration (
|
![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: cbab65c040
ℹ️ 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".
|
🎯 Code Coverage (details) 🔗 Commit SHA: 3f6a287 | Docs | Datadog PR Page | Give us feedback! |
AlexandreYang
changed the title
Static quality checks✅ Please find below the results from static quality gates Successful checksInfo
11 successful checks with minimal change (< 2 KiB)
|
|
@codex review focus on the new review fixes: baseline-only rDNS activation, recurring TTL-boundary compatibility when baseline is disabled, the baseline packaged-Agent E2E/fakeintake assertions, and removal of inventory exposure. Also review the full PR for correctness, security, concurrency, disabled-state regressions, and test coverage. |
|
@copilot review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5e261bf3a
ℹ️ 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".
Regression DetectorRegression Detector ResultsMetrics dashboard Baseline: a614467 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | quality_gate_logs | % cpu utilization | +2.82 | [+1.95, +3.68] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_metrics_logs | memory utilization | +0.38 | [+0.13, +0.62] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_idle | memory utilization | +0.05 | [-0.06, +0.16] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle | memory utilization | +0.04 | [-0.08, +0.15] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_idle_all_features | memory utilization | +0.01 | [-0.04, +0.05] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_mean_fs_load | memory utilization | -0.24 | [-0.31, -0.17] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_security_no_fs_load | memory utilization | -0.25 | [-0.39, -0.11] | 1 | Logs bounds checks dashboard |
| ➖ | quality_gate_private_action_runner | memory utilization | -0.53 | [-0.65, -0.40] | 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 | 4 = 4 | bounds checks dashboard |
| ✅ | quality_gate_idle | memory_usage | 10/10 | 171.25MiB ≤ 178MiB | bounds checks dashboard |
| ✅ | quality_gate_idle | total_bytes_received | 10/10 | 741.81KiB ≤ 819.20KiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | intake_connections | 10/10 | 4 = 4 | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | memory_usage | 10/10 | 515.09MiB ≤ 538MiB | bounds checks dashboard |
| ✅ | quality_gate_idle_all_features | total_bytes_received | 10/10 | 1.13MiB ≤ 1.25MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | intake_connections | 10/10 | 18 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_logs | memory_usage | 10/10 | 203.61MiB ≤ 229MiB | bounds checks dashboard |
| ✅ | quality_gate_logs | missed_bytes | 10/10 | 0B = 0B | bounds checks dashboard |
| ✅ | quality_gate_logs | total_bytes_received | 10/10 | 264.22MiB ≤ 292MiB | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | cpu_usage | 10/10 | 369.38 ≤ 2000 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | intake_connections | 10/10 | 17 ≤ 40 | bounds checks dashboard |
| ✅ | quality_gate_metrics_logs | memory_usage | 10/10 | 400.55MiB ≤ 439MiB | 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.94GiB ≤ 1.04GiB | bounds checks dashboard |
| ✅ | quality_gate_private_action_runner | memory_usage | 10/10 | 72.08MiB ≤ 76MiB | bounds checks dashboard |
| ✅ | quality_gate_security_idle | cpu_usage | 10/10 | 27.51 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_idle | memory_usage | 10/10 | 323.19MiB ≤ 335MiB | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | cpu_usage | 10/10 | 60.41 ≤ 200 | bounds checks dashboard |
| ✅ | quality_gate_security_mean_fs_load | memory_usage | 10/10 | 304.32MiB ≤ 314MiB | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | cpu_usage | 10/10 | 20.93 ≤ 100 | bounds checks dashboard |
| ✅ | quality_gate_security_no_fs_load | memory_usage | 10/10 | 312.08MiB ≤ 343MiB | 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".
CI Pass/Fail Decision
✅ Passed. All Quality Gates passed.
- quality_gate_metrics_logs, bounds check missed_bytes: 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 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 memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_idle, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_mean_fs_load, bounds check cpu_usage: 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 intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_logs, 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_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_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_all_features, bounds check intake_connections: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check memory_usage: 10/10 replicas passed. Gate passed.
- quality_gate_security_no_fs_load, bounds check cpu_usage: 10/10 replicas passed. Gate passed.
- quality_gate_private_action_runner, bounds check memory_usage: 10/10 replicas passed. Gate passed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Suppressed comments (3)
comp/networkpath/npcollector/impl/npcollector.go:292
- The baseline-mode selection is currently keyed off
!connectionsMonitoringEnabled, so if bothconnectionsMonitoringEnabledandbaselineTestsEnabledare true, baseline mode is silently disabled. If that “mutually exclusive” behavior is intended, make it explicit (e.g., warn/log and ignore baseline), or enforce it in config parsing; otherwise, keybaselineModeoffbaselineTestsEnabled(or add a clear precedence rule) so operators don’t end up with a surprising no-op.
func (s *npCollectorImpl) ScheduleNetworkPathTests(conns iter.Seq[npmodel.NetworkPathConnection]) {
if !s.collectorConfigs.connectionsMonitoringEnabled && !s.collectorConfigs.baselineTestsEnabled {
return
}
baselineMode := !s.collectorConfigs.connectionsMonitoringEnabled
s.scheduleNetworkPathTests(payload.PathOriginNetworkTraffic, conns, baselineMode)
}
pkg/networkpath/payload/payload_test.go:60
- Checking JSON field presence via
bytes.Containsis brittle (it can produce false positives/negatives if formatting or escaping changes). A more robust assertion is to unmarshal intomap[string]any(or a small struct alias) and assert key presence/absence directly.
func TestNetworkPathDynamicTestProfileJSON(t *testing.T) {
tests := []struct {
name string
profile DynamicTestProfile
expectField bool
}{
{name: "unset", expectField: false},
{name: "baseline", profile: DynamicTestProfileBaseline, expectField: true},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
raw, err := json.Marshal(NetworkPath{DynamicTestProfile: tt.profile})
require.NoError(t, err)
assert.Equal(t, tt.expectField, bytes.Contains(raw, []byte(`"dynamic_test_profile"`)))
})
}
}
test/new-e2e/tests/netpath/dynamic-tests/host_traffic_dynamic_path_test.go:102
- Passing an untyped empty string to a
payload.DynamicTestProfileparameter obscures intent (unset vs. a real profile). Consider adding/using an explicit “unset” value (e.g.,payload.DynamicTestProfile("")or a named constant likeDynamicTestProfileUnset) to make the expectation self-documenting.
match := assertHostTrafficNetworkPath(c, netpaths, "", "RC-admitted")
|
@codex make a comprehensive code and security review Classify findings as P0 (critical), P1 (high), P2 (substantive), or P3 (optional). Include the priority, file, line, failure scenario, impact, and whether the finding is in scope for this PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8afc6af3e0
ℹ️ 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".
- Keep baseline candidate admission governed by built-in and local filters so Dynamic RC remains exclusive to standard tests. - Add focused coverage proving RC includes and excludes cannot alter baseline selection. Source: review feedback Validation: bazel test --nocache_test_results //comp/networkpath/npcollector/impl:impl_test
|
[AI] review-fix-loop iteration 1 — fixed and pushed (
|
|
@codex make a comprehensive code and security review Classify findings as P0 (critical), P1 (high), P2 (substantive), or P3 (optional). Include the priority, file, line, failure scenario, impact, and whether the finding is in scope for this PR. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 30 changed files in this pull request and generated no new comments.
Suppressed comments (3)
comp/networkpath/npcollector/impl/npcollector.go:257
- Baseline mode evaluates
s.localFilterwithout any synchronization, while standard mode protectss.filterwithfilterMutex. This is only safe iflocalFilteris immutable for the lifetime of the collector and never mutated/replaced concurrently (e.g., by remote config updates or config reload). To avoid potential data races and to ensure baseline truly ignores RC, consider either (1) guaranteeinglocalFilteris a separate, immutable filter built from local config only, or (2) protectinglocalFilteraccess with the same mutex / an atomic pointer swap strategy used forfilter.
if baselineMode {
// Dynamic Remote Configuration admits standard tests only. Baseline
// selection remains governed by built-in and local filters.
included, testConfigID, tags = s.localFilter.EvaluateWithTags(conn.Domain, conn.Dest.Addr())
} else {
s.filterMutex.RLock()
included, testConfigID, tags = s.filter.EvaluateWithTags(conn.Domain, conn.Dest.Addr())
s.filterMutex.RUnlock()
}
comp/networkpath/npcollector/impl/npcollector.go:343
- In baseline mode,
evaluateNetworkPathForConncomputestestConfigIDandtags(viaEvaluateWithTags), but those values are never applied to thepathtestbefore it’s selected/scheduled. This drops any tags produced by local/built-in filters for baseline tests. Consider applyingevaluation.tags(and any other relevant fields) topathtestbefore callingaddBaselinePath, or extendingaddBaselinePathto accept and persist the evaluation output.
evaluation := s.evaluateNetworkPathForConn(conn, origin, vpcSubnets, baselineMode)
if !evaluation.shouldSchedule {
s.logger.Tracef("Skipped connection: addr=%s, protocol=%s", conn.Dest, conn.Type)
continue
}
pathtest := s.makePathtest(conn, origin)
if baselineMode {
selectedBaselineCandidates = addBaselinePath(selectedBaselineCandidates, pathtest, conn.Signals)
continue
}
comp/networkpath/npcollector/impl/baseline.go:70
- The baseline ranking byte score sums
SentBytes + RecvBytesdirectly. While overflow is unlikely, it would silently wrap if extremely large counters are present. Consider guarding against overflow (e.g., saturating atmath.MaxUint64or using a checked add) since the result is used for ordering.
func addBaselinePath(selected []baselineCandidate, path common.Pathtest, signals npmodel.ConnectionSignals) []baselineCandidate {
path.DynamicTestProfile = payload.DynamicTestProfileBaseline
return addBaselineCandidate(selected, baselineCandidate{
path: path,
pathHash: path.GetHash(),
diagnostic: signals.TimeoutCount > 0 || signals.RTOCount > 0 || signals.Retransmits > 0,
bytes: signals.SentBytes + signals.RecvBytes,
})
}
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This reverts commit db7b106.
- Document that baseline selection is independent of RC admission and attribution. - Exercise the real RC update path when verifying baseline provenance. Validation: bazel test --nocache_test_results //comp/networkpath/npcollector/impl:impl_test //pkg/networkpath/payload:payload_test
|
[AI] Baseline + Dynamic RC consistency update (
|
- Document the shared RC filter where baseline and standard scheduling diverge, making the attribution flow explicit. Validation: bazel test --nocache_test_results //comp/networkpath/npcollector/impl:impl_test
- Keep the compatibility invariant beside the baseline profile assignment where provenance must remain unchanged. Validation: bazel test --nocache_test_results //comp/networkpath/npcollector/impl:impl_test
|
@codex make a comprehensive code and security review Classify findings as P0 (critical), P1 (high), P2 (substantive), or P3 (optional). Include the priority, file, line, failure scenario, impact, and whether the finding is in scope for this PR. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Suppressed comments (5)
comp/networkpath/npcollector/impl/baseline_test.go:7
- This test file is guarded by
//go:build test, so it will be skipped by defaultgo testunless CI/Bazel explicitly sets thetestbuild tag. If the rest of the repo’s Go unit tests aren’t consistently run with that tag, consider removing the build tag (mandatory if it would otherwise be skipped), or ensure the Bazeldd_agent_go_testtarget for this package sets the required build tags so these baseline-selection tests always execute in CI.
//go:build test
package npcollectorimpl
pkg/networkpath/payload/payload_test.go:58
- Using
bytes.Containson the marshaled JSON is a bit brittle (it can miss subtle regressions like a renamed field, or pass accidentally if the substring appears elsewhere). A more robust test is to unmarshal intomap[string]any(or a small struct) and assert key presence/absence and, when present, the expected value (e.g.,\"baseline\").
raw, err := json.Marshal(NetworkPath{DynamicTestProfile: tt.profile})
require.NoError(t, err)
assert.Equal(t, tt.expectField, bytes.Contains(raw, []byte(`"dynamic_test_profile"`)))
})
test/new-e2e/tests/netpath/dynamic-tests/host_traffic_dynamic_path_common_test.go:261
- After
configureAgentResolver()the test only asserts DNS resolution, but it no longer verifies that the service is reachable via the domain name. Since the generator intentionally swallows request exceptions, a misroute/firewall issue can turn into a slow ‘no netpath events’ timeout later. Consider adding a fast reachability check here (e.g., an HTTP GET from the generator host tohttp://<domain>/) so failures are detected early with clearer logs.
func (s *hostTrafficDynamicPathBaseSuite) assertHostTrafficDomainResolves() {
output := s.Env().RemoteHost.MustExecute("getent ahostsv4 " + shellQuote(hostTrafficRemoteConfigDomain))
require.Contains(s.T(), output, s.Env().HTTPBinHost.Address)
}
test/new-e2e/tests/netpath/dynamic-tests/host_traffic_dynamic_path_common_test.go:69
- Now that
hostTrafficDynamicPathProvisioner(...)is reused by multiple suites (baseline + RC-admitted), the EC2 instance naming/resource identifiers are still hard-coded (hosttrafficdynamicpathvm,hosttraffichttpbinvm). If these suites ever run concurrently in the same Pulumi project/stack (or if the test runner changes parallelism), this increases the risk of resource-name collisions. Consider incorporating thenameparameter into the VM names/resource identifiers (e.g., prefix/suffix) to keep resources unique per suite.
params := ec2.GetParams(
ec2.WithName("hosttrafficdynamicpathvm"),
ec2.WithAgentOptions(
agentparams.WithAgentConfig(agentConfig),
agentparams.WithSystemProbeConfig(systemProbeConfig),
),
)
test/new-e2e/tests/netpath/dynamic-tests/host_traffic_dynamic_path_common_test.go:74
- Now that
hostTrafficDynamicPathProvisioner(...)is reused by multiple suites (baseline + RC-admitted), the EC2 instance naming/resource identifiers are still hard-coded (hosttrafficdynamicpathvm,hosttraffichttpbinvm). If these suites ever run concurrently in the same Pulumi project/stack (or if the test runner changes parallelism), this increases the risk of resource-name collisions. Consider incorporating thenameparameter into the VM names/resource identifiers (e.g., prefix/suffix) to keep resources unique per suite.
httpbinHost, err := ec2.NewVM(awsEnv, "hosttraffichttpbinvm")
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc92f11b85
ℹ️ 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".
- Recreate the original resolver symlink target without resolving relative paths from the command working directory. - Preserve resolver-manager ownership across same-infrastructure E2E retries. Source: review feedback Validation: bazel build //test/new-e2e/tests/netpath/dynamic-tests:dynamic-tests_test
|
[AI] review-fix-loop iteration 1 — fixed and pushed (
|
|
@codex make a comprehensive code and security review Classify findings as P0 (critical), P1 (high), P2 (substantive), or P3 (optional). Include the priority, file, line, failure scenario, impact, and whether the finding is in scope for this PR. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
test/new-e2e/tests/netpath/dynamic-tests/host_traffic_dynamic_path_common_test.go:257
- This helper now validates only DNS resolution, whereas the prior flow also validated the service was reachable via the configured domain after the resolver switch. Adding a small HTTP check here (using the same Python/urllib approach used elsewhere in this file) would fail fast with a clearer error when DNS/HTTP wiring is broken, instead of timing out later waiting for netpath events.
func (s *hostTrafficDynamicPathBaseSuite) assertHostTrafficDomainResolves() {
output := s.Env().RemoteHost.MustExecute("getent ahostsv4 " + shellQuote(hostTrafficRemoteConfigDomain))
require.Contains(s.T(), output, s.Env().HTTPBinHost.Address)
}
- Resolve saved relative symlink targets against /etc before validating them. - Restore the static backup when the original target disappeared, while preserving valid resolver-manager symlinks verbatim. Source: review feedback Validation: bazel build //test/new-e2e/tests/netpath/dynamic-tests:dynamic-tests_test
|
[AI] review-fix-loop iteration 2 — fixed and pushed (
|
|
@codex make a comprehensive code and security review Classify findings as P0 (critical), P1 (high), P2 (substantive), or P3 (optional). Include the priority, file, line, failure scenario, impact, and whether the finding is in scope for this PR. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Suppressed comments (3)
test/new-e2e/tests/netpath/dynamic-tests/host_traffic_dynamic_path_common_test.go:265
assertHostTrafficDomainResolvesnow only verifies DNS resolution, but not that the service is reachable via the domain after the resolver switch. This can lead to long test timeouts later (generator runs, fakeintake waits) when HTTP access fails despite successfulgetent. Consider adding a fast reachability check here (e.g., a shortpython3/curlrequest tohttp://<domain>/) so the suite fails early with clearer diagnostics.
func (s *hostTrafficDynamicPathBaseSuite) assertHostTrafficDomainResolves() {
output := s.Env().RemoteHost.MustExecute("getent ahostsv4 " + shellQuote(hostTrafficRemoteConfigDomain))
require.Contains(s.T(), output, s.Env().HTTPBinHost.Address)
}
test/new-e2e/tests/netpath/dynamic-tests/host_traffic_dynamic_path_common_test.go:103
- The refactor replaces the previous
curl-based checks/generator with multiplepython3invocations (HTTP server, DNS server, generator, reachability checks). If the E2E images ever change andpython3is missing on either host, failures will be less actionable (“command not found”) and harder to diagnose. Consider adding an explicit prerequisite check/install step (similar to the previous curl installation guard) forpython3on bothRemoteHostandHTTPBinHost, or at least failing fast with a targeted message whenpython3is unavailable.
func (s *hostTrafficDynamicPathBaseSuite) setupHostTraffic() {
s.startHostTrafficHTTPServer()
s.startHostTrafficDNSServer()
s.assertHostTrafficServiceReady()
s.assertHostTrafficServiceReachable()
s.configureAgentResolver()
s.assertHostTrafficDomainResolves()
}
pkg/networkpath/payload/payload_test.go:58
- Using
bytes.Containson the raw JSON to detect field presence is relatively brittle (it depends on string matching rather than JSON structure). A more robust approach is to unmarshal intomap[string]any(or a small struct) and assert whether thedynamic_test_profilekey exists, which will keep the test stable across potential encoding changes while still validatingomitemptybehavior.
raw, err := json.Marshal(NetworkPath{DynamicTestProfile: tt.profile})
require.NoError(t, err)
assert.Equal(t, tt.expectField, bytes.Contains(raw, []byte(`"dynamic_test_profile"`)))
})
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
[AI] review-fix-loop: clean — 3 iterations (2 repair iterations + final verification)
|
What does this PR do?
Adds opt-in baseline Dynamic Tests for Cloud Network Monitoring (CNM). Baseline mode selects a small, representative set of paths directly from each CNM connection snapshot when full connection-monitoring Dynamic Tests are disabled.
Traceroute must also be enabled in the system-probe configuration. If full connection-monitoring Dynamic Tests are enabled, they take precedence and the collector continues to use standard scheduling.
Motivation
Hosts without full Dynamic Tests currently have no included Network Path coverage for observed CNM traffic. Baseline mode provides limited recurring path visibility.
The selector favors connections that are more useful for diagnosis while keeping resource use predictable: timeout, RTO, and retransmit observations rank ahead of healthy traffic, then higher-volume connections rank first.
Design and key changes
Snapshot selection
Data boundary
Both CNM producers populate the same generic
ConnectionSignalsstructure onNetworkPathConnection: normalized timeout count, RTO count, retransmits, sent bytes, and received bytes. These are raw connection observations rather than baseline-specific derived state.The baseline selector in
baseline.gois the only layer that interprets those signals into the diagnostic and traffic-volume ranking dimensions. This keeps producer behavior equivalent without coupling the shared connection model to baseline policy.Scheduling and payloads
Standard, baseline, and NetFlow paths share the collector's existing eligibility checks, path construction, scheduling telemetry, deduplication, interval, TTL, context limit, and rate limit. Baseline mode adds only the snapshot-ranking step before selected paths enter that common machinery.
Baseline results are recurring Dynamic Tests and carry
dynamic_test_profile: "baseline"in Network Path payloads so downstream consumers can distinguish them from standard tests. Baseline-only operation also enables the collector and its reverse-DNS dependency.Baseline mode and Dynamic Remote Configuration filters are compatible and independent: baseline mode controls snapshot ranking, while the effective local-plus-RC filter controls path admission. When an RC rule admits the winning baseline candidate, its configuration ID, source, and tags are preserved alongside
dynamic_test_profile: "baseline".E2E coverage
Adds a packaged-Agent E2E suite that generates CNM host traffic and verifies through fakeintake that a Network Path event is emitted with the baseline profile.
The existing Remote Config host-traffic suite and the new baseline suite now share one fixture. The fixture replaces the GHCR-backed HTTP container and runtime
curlinstallation with Python standard-library HTTP and traffic generation, including readiness checks, cleanup, and failure logs.Manual validation
Use this minimal
datadog.yamlconfiguration:Enable traceroute in
system-probe.yaml:After generating outbound TCP traffic from the host, verify that Network Path events are emitted with
test_run_type: "dynamic"anddynamic_test_profile: "baseline". Up to three unique eligible paths should be selected from each CNM snapshot, subject to the existing collector filters and scheduling limits.Describe how you validated your changes
bazel test //comp/networkpath/npcollector/impl:impl_testAdditional Notes
The new configuration setting defaults to
falseand remains an internal rollout control omitted from generateddatadog.yamlexamples.