UnmarshalKey implemented using pure reflection #28821

dustmop · 2024-08-27T19:58:02Z

What does this PR do?

This version of UnmarshalKey doesn't rely upon the config having an internal map[string]interface{} to store data. Instead, it is implemented using reflection only and demonstrates the a node-based view of the config's data model.

Motivation

Preparation for the new config model

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

Functionality is covered by unit tests

This version of UnmarshalKey doesn't rely upon the config having an internal map[string]interface{} to store data. Instead, it is implemented using reflection only and demonstrates the a node-based view of the config's data model.

pr-commenter · 2024-08-27T20:30:54Z

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=43752364 --os-family=ubuntu

Note: This applies to commit 29acfc7

pr-commenter · 2024-08-27T20:56:06Z

Regression Detector

Regression Detector Results

Run ID: d9cea281-468c-4ab3-bb1c-eb959235b595 Metrics dashboard Target profiles

Baseline: 0cce24d
Comparison: ce51c6c

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

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI	trials	links
➖	file_tree	memory utilization	+1.85	[+1.76, +1.95]	1	Logs
➖	basic_py_check	% cpu utilization	+1.19	[-1.84, +4.21]	1	Logs
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	+0.73	[-0.13, +1.58]	1	Logs
➖	pycheck_lots_of_tags	% cpu utilization	+0.22	[-2.22, +2.67]	1	Logs
➖	idle	memory utilization	+0.20	[+0.16, +0.24]	1	Logs
➖	tcp_dd_logs_filter_exclude	ingress throughput	+0.00	[-0.01, +0.01]	1	Logs
➖	uds_dogstatsd_to_api	ingress throughput	-0.00	[-0.00, +0.00]	1	Logs
➖	otel_to_otel_logs	ingress throughput	-0.45	[-1.26, +0.35]	1	Logs
➖	tcp_syslog_to_blackhole	ingress throughput	-1.79	[-14.62, +11.04]	1	Logs

Bounds Checks

perf	experiment	bounds_check_name	replicates_passed
❌	idle	memory_usage	1/10

Explanation

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".

GustavoCaso · 2024-08-29T09:55:18Z

pkg/config/structure/unmarshal.go

+	if v.Kind() == reflect.Struct {
+		return &nodeimpl{typ: structNodeType, val: v}
+	}
+	if v.Kind() == reflect.Map {
+		return &nodeimpl{typ: mapNodeType, val: v}
+	}
+	if v.Kind() == reflect.Slice {
+		return &nodeimpl{typ: listNodeType, val: v}
+	}
+	if isScalar(v) {
+		return &nodeimpl{typ: scalarNodeType, val: v}
+	}


Nitpick suggestion

Suggested change

if v.Kind() == reflect.Struct {

return &nodeimpl{typ: structNodeType, val: v}

}

if v.Kind() == reflect.Map {

return &nodeimpl{typ: mapNodeType, val: v}

}

if v.Kind() == reflect.Slice {

return &nodeimpl{typ: listNodeType, val: v}

}

if isScalar(v) {

return &nodeimpl{typ: scalarNodeType, val: v}

}

nodeType := invalidNodeType

if v.Kind() == reflect.Struct {

nodeType = structNodeType

}

if v.Kind() == reflect.Map {

nodeType = mapNodeType

}

if v.Kind() == reflect.Slice {

nodeType = listNodeType

}

if isScalar(v) {

nodeType = scalarNodeType

}

if nodeType == invalidNodeType {

return nil, fmt.Errorf("could not create node from: %v of type %T and kind %v", v, v, v.Kind())

}

return &nodeimpl{typ: nodeType, val: v}

Now this method returns a different implementation based upon the value's kind.

GustavoCaso · 2024-08-29T09:56:11Z

pkg/config/structure/unmarshal.go

+	if isScalar(v) {
+		return &nodeimpl{typ: scalarNodeType, val: v}
+	}
+	panic(fmt.Errorf("could not create node from: %v of type %T and kind %v", v, v, v.Kind()))


Do we want to panic? Should we return (node, error) and handle the case of invalid node type?

I agree, maybe returning an error would be better.

Removed all panics from the file, returning errors everywhere now.

GustavoCaso · 2024-08-29T09:57:26Z

pkg/config/structure/unmarshal.go

+	if outValue.Kind() == reflect.Map {
+		return copyMap(outValue, source)
+	}
+	if outValue.Kind() == reflect.Struct {
+		return copyStruct(outValue, source)
+	}
+	if outValue.Kind() == reflect.Slice {
+		return copyList(outValue, source.AsList())
+	}


Nitpick: Could we use a switch statement here?

Yes, good call, done.

GustavoCaso · 2024-08-29T10:03:17Z

pkg/config/structure/unmarshal.go

+		}
+		return keys
+	}
+	return nil


Is return nil the same same as a empty slice of strings?

I was using nil to mean "no children" before, but now this function returns an error instead in that case.

GustavoCaso · 2024-08-29T10:11:03Z

pkg/config/structure/unmarshal.go

+		}
+		scalar := child.AsScalar()
+		if scalar != nil {
+			mval, _ := scalar.GetString()


If understand correctly, GetString would return an empty string if leaf can not get the string value. Do we want to store empty string as map values?

Do we need this conversion to string ? Why not use the original scalar type ?

Changed this call to GetString so that we check the error return. This call isn't quite a conversion: the var scalar represents a leaf node, and this call is getting the string value of that leaf node.

GustavoCaso · 2024-08-29T10:15:17Z

pkg/config/structure/unmarshal.go

+// GetInt returns the scalar as a int, or an error otherwise
+func (n *leafimpl) GetInt() (int, error) {
+	switch n.val.Kind() {
+	case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
+		return int(n.val.Int()), nil
+	case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64:
+		return int(n.val.Uint()), nil
+	}
+	return 0, conversionError(n.val, "int")
+}


YAML supports floats. Do we care about floats for our use case?

We seem to use float values on our configuration template pkg/config/config_template.yaml

Added float support to the various places that needed it.

GustavoCaso · 2024-08-29T10:17:49Z

pkg/config/structure/unmarshal.go

+	}
+	elemType := target.Type()
+	elemType = elemType.Elem()
+	results := reflect.MakeSlice(reflect.SliceOf(elemType), source.Size(), source.Size())


We are accessing souorce.Size() three times, should hosting the size value on a variable?

Sure, done, now numElems is used for this value.

GustavoCaso

Awesome work. The code is very easy to follow 🎉

Left a few questions 😄

pkg/config/structure/unmarshal.go

hush-hush · 2024-09-02T14:02:37Z

pkg/config/structure/unmarshal.go

+	if isScalar(v) {
+		return &nodeimpl{typ: scalarNodeType, val: v}
+	}
+	panic(fmt.Errorf("could not create node from: %v of type %T and kind %v", v, v, v.Kind()))


I agree, maybe returning an error would be better.

pkg/config/structure/unmarshal.go

hush-hush · 2024-09-02T14:56:47Z

pkg/config/structure/unmarshal.go

+		}
+		scalar := child.AsScalar()
+		if scalar != nil {
+			mval, _ := scalar.GetString()


Do we need this conversion to string ? Why not use the original scalar type ?

pkg/config/structure/unmarshal.go

Instead of methods like AsList and AsScalar, rely on interface type checking. Each specific type of node uses its own implementation. Support struct tag specifiers like omitempty. Introduce errNotFound for GetChild when nodes aren't found

hush-hush

I added a couple of quesiton/nit-picks, but it looks good overall !0

pkg/config/structure/unmarshal.go

hush-hush · 2024-09-06T15:12:45Z

pkg/config/structure/unmarshal.go

+	mtype := reflect.MapOf(ktype, vtype)
+	results := reflect.MakeMap(mtype)
+
+	mapKeys, err := source.ChildrenKeys()


This would mean we could Unmarshal a struct into a map, no ? Should we prevent this and check that the source is also a map ?

Similar question for the other copy* func

I think this is okay? It matches how the standard Unmarshal family of functions work. We'll keep an eye on it in future PRs, as this new function is used in more places, to see if we should decide to disable this behavior.

dustmop · 2024-09-06T16:05:23Z

/merge

dd-devflow · 2024-09-06T16:05:29Z

🚂 MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

Use /merge -c to cancel this operation!

dd-devflow · 2024-09-06T17:01:11Z

🚂 MergeQueue: pull request added to the queue

The median merge time in main is 23m.

Use /merge -c to cancel this operation!

UnmarshalKey implemented using pure reflection

a27552d

This version of UnmarshalKey doesn't rely upon the config having an internal map[string]interface{} to store data. Instead, it is implemented using reflection only and demonstrates the a node-based view of the config's data model.

dustmop added changelog/no-changelog team/agent-shared-components qa/done Skip QA week as QA was done before merge and regressions are covered by tests labels Aug 27, 2024

dustmop added this to the 7.58.0 milestone Aug 27, 2024

dustmop requested a review from a team as a code owner August 27, 2024 19:58

Remove unused field to fix lint error

275dc5d

dustmop mentioned this pull request Aug 27, 2024

Use the new UnmarshalKey function in a few places to demonstrate its compatibility #28822

Closed

Allow boolean string to be parsed as a bool value

270fe28

GustavoCaso reviewed Aug 29, 2024

View reviewed changes

hush-hush reviewed Sep 2, 2024

View reviewed changes

dustmop added 3 commits September 3, 2024 14:31

Merge branch 'main' into dustin.long/unmarshal-reflect

9e42a19

Fix lint error about else return, more func docs

ce51c6c

hush-hush approved these changes Sep 6, 2024

View reviewed changes

Support bool values as 1 or 0 strings

29acfc7

dd-mergequeue bot merged commit 2a712cb into main Sep 6, 2024
197 of 217 checks passed

dd-mergequeue bot deleted the dustin.long/unmarshal-reflect branch September 6, 2024 17:21

dustmop mentioned this pull request Sep 11, 2024

Use new UnmarshalKey in a few locations. Fix edge cases. #29225

Merged

jeremy-hanna mentioned this pull request Sep 26, 2024

[ASCII-2262] Extend config.UnmarshalKey unit tests around primitives #29604

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UnmarshalKey implemented using pure reflection #28821

UnmarshalKey implemented using pure reflection #28821

dustmop commented Aug 27, 2024

pr-commenter bot commented Aug 27, 2024 •

edited

Loading

pr-commenter bot commented Aug 27, 2024 •

edited

Loading

Fine details of change detection per experiment

Explanation

GustavoCaso Aug 29, 2024

dustmop Sep 4, 2024

GustavoCaso Aug 29, 2024

hush-hush Sep 2, 2024

dustmop Sep 4, 2024

GustavoCaso Aug 29, 2024 •

edited

Loading

dustmop Sep 4, 2024

GustavoCaso Aug 29, 2024

dustmop Sep 4, 2024

GustavoCaso Aug 29, 2024

hush-hush Sep 2, 2024

dustmop Sep 4, 2024

GustavoCaso Aug 29, 2024

GustavoCaso Aug 29, 2024

dustmop Sep 4, 2024

GustavoCaso Aug 29, 2024

dustmop Sep 4, 2024

GustavoCaso left a comment

hush-hush Sep 2, 2024

hush-hush Sep 2, 2024

hush-hush left a comment

hush-hush Sep 6, 2024

dustmop Sep 6, 2024

dustmop commented Sep 6, 2024

dd-devflow bot commented Sep 6, 2024

dd-devflow bot commented Sep 6, 2024

UnmarshalKey implemented using pure reflection #28821

UnmarshalKey implemented using pure reflection #28821

Conversation

dustmop commented Aug 27, 2024

What does this PR do?

Motivation

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

pr-commenter bot commented Aug 27, 2024 • edited Loading

Test changes on VM

pr-commenter bot commented Aug 27, 2024 • edited Loading

Regression Detector

Regression Detector Results

No significant changes in experiment optimization goals

Fine details of change detection per experiment

Bounds Checks

Explanation

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

GustavoCaso Aug 29, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GustavoCaso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hush-hush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustmop commented Sep 6, 2024

dd-devflow bot commented Sep 6, 2024

dd-devflow bot commented Sep 6, 2024

pr-commenter bot commented Aug 27, 2024 •

edited

Loading

pr-commenter bot commented Aug 27, 2024 •

edited

Loading

GustavoCaso Aug 29, 2024 •

edited

Loading