Fix diff around default and known settings #31658

hush-hush · 2024-12-02T14:29:54Z

What does this PR do?

We need to mimic the behavior from viper around default and known settings.

Fix the case where the user would BindEnv after setting a default and would override the default with nil:

config.SetDefault("a", 1234)
config.BindEnv("a")

Fix intermediate key not being known: SetKnown("a.b.c") should set a, a.b and a.b.c as known.
Fix AllSettings returning settings without values from BindEnv. This is done by adding a proper schema to differentiate settings with and without values. This answer the core difference betweenBindEnv add SetDefault. Once we enforce a default for all settings we will be able to remove this difference. but for now we must mimic Viper default.
Fix the config not using search paths to look for datadog.yaml
Fix IsSet returning false when given an inner node setting path

Describe how you validated your changes

Functionality is currently guarded behind a env var, so there should be no functional change. To test the behavior, use the DD_CONF_NODETREEMODEL env var, but be warned that there are likely bugs.

We need to mimic the behavior from viper around default and known settings.

agent-platform-auto-pr · 2024-12-02T14:57:44Z

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv create-vm --pipeline-id=50222769 --os-family=ubuntu

Note: This applies to commit 8795e69

cit-pr-commenter · 2024-12-02T15:19:21Z

Regression Detector

Regression Detector Results

Metrics dashboard
Target profiles
Run ID: a1b5a0e3-c5c2-456c-8480-7c19b4f5e602

Baseline: 55cc0d6
Comparison: 8795e69
Diff

Optimization Goals: ✅ No significant changes detected

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI	trials	links
➖	otel_to_otel_logs	ingress throughput	+1.33	[+0.64, +2.03]	1	Logs
➖	basic_py_check	% cpu utilization	+1.03	[-2.77, +4.83]	1	Logs
➖	file_to_blackhole_300ms_latency	egress throughput	+0.02	[-0.61, +0.65]	1	Logs
➖	file_to_blackhole_0ms_latency	egress throughput	+0.01	[-0.84, +0.86]	1	Logs
➖	uds_dogstatsd_to_api	ingress throughput	+0.00	[-0.11, +0.11]	1	Logs
➖	tcp_dd_logs_filter_exclude	ingress throughput	+0.00	[-0.01, +0.01]	1	Logs
➖	file_to_blackhole_100ms_latency	egress throughput	-0.03	[-0.76, +0.70]	1	Logs
➖	file_to_blackhole_1000ms_latency_linear_load	egress throughput	-0.20	[-0.66, +0.25]	1	Logs
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	-0.22	[-0.95, +0.52]	1	Logs
➖	file_to_blackhole_500ms_latency	egress throughput	-0.23	[-0.99, +0.53]	1	Logs
➖	file_to_blackhole_1000ms_latency	egress throughput	-0.26	[-1.04, +0.53]	1	Logs
➖	quality_gate_idle	memory utilization	-0.58	[-0.62, -0.53]	1	Logs bounds checks dashboard
➖	tcp_syslog_to_blackhole	ingress throughput	-0.71	[-0.78, -0.65]	1	Logs
➖	quality_gate_logs	% cpu utilization	-0.79	[-3.75, +2.17]	1	Logs
➖	file_tree	memory utilization	-0.79	[-0.93, -0.66]	1	Logs
➖	quality_gate_idle_all_features	memory utilization	-0.88	[-1.00, -0.76]	1	Logs bounds checks dashboard
➖	pycheck_lots_of_tags	% cpu utilization	-2.01	[-5.38, +1.35]	1	Logs

Bounds Checks: ❌ Failed

perf	experiment	bounds_check_name	replicates_passed	links
❌	file_to_blackhole_500ms_latency	lost_bytes	8/10
✅	file_to_blackhole_0ms_latency	lost_bytes	10/10
✅	file_to_blackhole_0ms_latency	memory_usage	10/10
✅	file_to_blackhole_1000ms_latency	memory_usage	10/10
✅	file_to_blackhole_1000ms_latency_linear_load	memory_usage	10/10
✅	file_to_blackhole_100ms_latency	lost_bytes	10/10
✅	file_to_blackhole_100ms_latency	memory_usage	10/10
✅	file_to_blackhole_300ms_latency	lost_bytes	10/10
✅	file_to_blackhole_300ms_latency	memory_usage	10/10
✅	file_to_blackhole_500ms_latency	memory_usage	10/10
✅	quality_gate_idle	memory_usage	10/10	bounds checks dashboard
✅	quality_gate_idle_all_features	memory_usage	10/10	bounds checks dashboard
✅	quality_gate_logs	lost_bytes	10/10
✅	quality_gate_logs	memory_usage	10/10

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_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
quality_gate_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.
quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.

dustmop · 2024-12-02T21:31:56Z

pkg/config/teeconfig/teeconfig.go

-		_, file, line, _ := runtime.Caller(2)
-		fileParts := strings.Split(file, "DataDog/datadog-agent/")
-		log.Warnf("difference in config: %s(%s) -> base: %v | compare %v from %s:%d", method, key, base, compare, fileParts[len(fileParts)-1], line)
+		log.Warnf("difference in config: %s(%s) -> base[%s]: %#v | compare[%s] %#v | from %s", method, key, t.baseline.GetSource(key), base, t.compare.GetSource(key), compare, getLocation(2))
 	}
 	return compare


This seems like a bug: compareResult is returning compare which is the nodetreemodel value. It should be returning base instead, which is the value from viper.

dustmop · 2024-12-02T21:32:34Z

pkg/config/teeconfig/teeconfig.go

@@ -169,22 +175,22 @@ func compareResult(key, method string, base, compare interface{}) interface{} {
 func (t *teeConfig) Get(key string) interface{} {
 	base := t.baseline.Get(key)
 	compare := t.compare.Get(key)
-	return compareResult(key, "Get", base, compare)
+	return t.compareResult(key, "Get", base, compare)


Which means here the future value is being returned when teeconfig is enabled.

dustmop · 2024-12-02T21:33:34Z

pkg/config/setup/config.go

+	config.SetDefault("proxy.http", nil)
+	config.SetDefault("proxy.https", nil)
+	config.SetDefault("proxy.no_proxy", nil)
+	//config.SetDefault("proxy.http", "")


Remove these commented out lines.

dustmop · 2024-12-02T21:34:28Z

pkg/config/nodetreemodel/read_config_file.go

-		// check if the key is know in the defaults
-		defaultNode, err := defaults.GetChild(key)
+		// check if the key is know in the schema
+		defaultNode, err := schema.GetChild(key)


How about also renaming defaultNode.

dustmop · 2024-12-02T21:35:09Z

pkg/config/nodetreemodel/read_config_file.go

+
+	if strictErr := yaml.UnmarshalStrict(content, &obj); strictErr != nil {
+		log.Errorf("warning reading config file: %v\n", strictErr)
+		if err := yaml.UnmarshalStrict(content, &obj); err != nil {


Why call this a second time? Is it supposed to be calling yaml.Unmarshal instead?

dustmop · 2024-12-02T21:38:00Z

pkg/config/nodetreemodel/read_config_file.go

@@ -19,7 +20,12 @@ import (

 func (c *ntmConfig) getConfigFile() string {
 	if c.configFile == "" {
-		return "datadog.yaml"
+		for _, path := range c.configPaths {
+			configFilePath := filepath.Join(path, c.configName+".yaml")


What if c.configName is not set? Should it default to "datadog" in that case?

dustmop · 2024-12-02T21:39:00Z

pkg/config/nodetreemodel/config.go

-func (c *ntmConfig) leafAtPath(key string) LeafNode {
-	return c.leafAtPathFromNode(key, c.root)
-}
-
 func (c *ntmConfig) leafAtPathFromNode(key string, curr Node) LeafNode {


Is this called anywhere using a value for curr that isn't equal to c.root?

dustmop · 2024-12-02T21:42:35Z

pkg/config/nodetreemodel/config_test.go

@@ -326,12 +326,14 @@ func TestIsSet(t *testing.T) {
 	cfg := NewConfig("test", "TEST", nil)
 	cfg.SetDefault("a", 0)
 	cfg.SetDefault("b", 0)
+	cfg.SetKnown("c")


How about adding a check for cfg.IsKnown for completeness.

dustmop · 2024-12-02T21:44:49Z

pkg/config/nodetreemodel/config.go

@@ -131,11 +143,13 @@ func (c *ntmConfig) OnUpdate(callback model.NotificationReceiver) {
 	c.notificationReceivers = append(c.notificationReceivers, callback)
 }

-func (c *ntmConfig) setDefault(key string, value interface{}) {
+func (c *ntmConfig) addToSchema(key string, source model.Source) {
 	parts := splitKey(key)
 	// TODO: Ensure that for default tree, setting nil to a node will not override


I think this comment is unnecessary, it's about the default tree but this method is adding to the schema.

dustmop · 2024-12-02T21:46:42Z

pkg/config/nodetreemodel/config.go

@@ -51,6 +51,18 @@ type ntmConfig struct {
 	// Bellow are all the different configuration layers. Each layers represents a source for our configuration.
 	// They are merge into the 'root' tree following order of importance (see pkg/model/viper.go:sourcesPriority).

+	// schema holds all the settings with or without value. Settings are added to the schema through BindEnv and


Is this table an accurate summation?

+---------+------------+ | BindEnv | SetDefault | +---------+------------+ AllKeys | yes | yes | AlSettings | - | yes | Get | * | yes | schema | yes | yes | defaults | - | yes | *: Yes, but if and only if an env var is set with a value

Fix diff around default and known settings

155e357

We need to mimic the behavior from viper around default and known settings.

hush-hush requested a review from a team as a code owner December 2, 2024 14:29

hush-hush requested a review from dustmop December 2, 2024 14:29

hush-hush added changelog/no-changelog team/agent-shared-components qa/done QA done before merge and regressions are covered by tests labels Dec 2, 2024

github-actions bot added the short review PR is simple enough to be reviewed quickly label Dec 2, 2024

Fix AllSettings returning settings without values

3fefa77

github-actions bot added medium review PR review might take time and removed short review PR is simple enough to be reviewed quickly labels Dec 2, 2024

hush-hush added 6 commits December 2, 2024 17:24

Fix IsSet method to use actual value instead of known keys

c4c98b2

Replicate viper usage of UnmarshalStrict

06ce309

WIP: Fix the setup setting a value for an inner node

9cc5928

WIP: Fix IsSet returning false when given a inner node path

63b1ce6

WIP: Fix config not looking through search paths for the default config

11f6f0d

WIP: expand debug info from teeconfig

8795e69

dustmop reviewed Dec 2, 2024

View reviewed changes

github-actions bot added long review PR is complex, plan time to review it and removed medium review PR review might take time labels Dec 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix diff around default and known settings #31658

Fix diff around default and known settings #31658

hush-hush commented Dec 2, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Dec 2, 2024 •

edited

Loading

cit-pr-commenter bot commented Dec 2, 2024 •

edited

Loading

Fine details of change detection per experiment

Explanation

dustmop Dec 2, 2024

dustmop Dec 2, 2024

dustmop Dec 2, 2024

dustmop Dec 2, 2024

dustmop Dec 2, 2024

dustmop Dec 2, 2024

dustmop Dec 2, 2024

dustmop Dec 2, 2024

dustmop Dec 2, 2024

dustmop Dec 2, 2024

Fix diff around default and known settings #31658

Are you sure you want to change the base?

Fix diff around default and known settings #31658

Conversation

hush-hush commented Dec 2, 2024 • edited Loading

What does this PR do?

Describe how you validated your changes

agent-platform-auto-pr bot commented Dec 2, 2024 • edited Loading

Test changes on VM

cit-pr-commenter bot commented Dec 2, 2024 • edited Loading

Regression Detector

Regression Detector Results

Optimization Goals: ✅ No significant changes detected

Fine details of change detection per experiment

Bounds Checks: ❌ Failed

Explanation

CI Pass/Fail Decision

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hush-hush commented Dec 2, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Dec 2, 2024 •

edited

Loading

cit-pr-commenter bot commented Dec 2, 2024 •

edited

Loading