Use new UnmarshalKey in a few locations. Fix edge cases. #29225
Conversation
Some edge cases are fixed. In order to narrow and simplify the API of the config, only allow these edge case behaviors if certain Options are passed to UnmarshalKey. UnmarshalKey behavior changes: - The mapstructure "squash" feature requires EnableSquash - Case sensitive field matching is always allowed, might become an Option - Empty strings can be parsed as empty lists, might become an Option
Go Package Import DifferencesBaseline: bb34457
|
The Host/Port tests in config_keys_test reveal that the config should be handlin g keys using case-insensitive comparisons. Implementing this is tricky and will be done in a future PR.
dustmop
added
team/agent-shared-components
changelog/no-changelog
qa/done
Test changes on VMUse this command from test-infra-definitions to manually test this PR changes on a VM: inv create-vm --pipeline-id=45031842 --os-family=ubuntuNote: This applies to commit b2f7390 |
| @@ -133,6 +163,31 @@ feature: | |||
| assert.Equal(t, feature.Enabled, true) | |||
| } | |||
|
|
|||
| type FeatureConfigDiffCase struct { | |||
| ENaBLEd bool | |||
There was a problem hiding this comment.
Would it make sense to test a private field as well, maybe not?
There was a problem hiding this comment.
Yes, I think that would be good. There's another PR in the work to specifically add more test coverage that @jeremy-hanna is working on. Private fields just simply be ignored by the unmarshaller.
pkg/config/structure/unmarshal.go
Outdated
| // features allowed for handling edge-cases | ||
|
|
||
| type featureSet struct { | ||
| allowSquash bool | ||
| convertEmptyStrNil bool | ||
| } | ||
|
|
||
| // UnmarshalKeyOption is an option that affects the enabled features in UnmarshalKey | ||
| type UnmarshalKeyOption func(*featureSet) | ||
|
|
||
| // EnableSquash allows UnmarshalKey to take advantage of `mapstructure`s `squash` feature | ||
| // a squashed field hoists its fields up a level in the marshalled representation and directly embeds them | ||
| var EnableSquash UnmarshalKeyOption = func(fs *featureSet) { | ||
| fs.allowSquash = true | ||
| } | ||
|
|
||
| // ConvertEmptyStringToNil allows UnmarshalKey to implicitly convert empty strings into nil slices | ||
| var ConvertEmptyStringToNil UnmarshalKeyOption = func(fs *featureSet) { | ||
| fs.convertEmptyStrNil = true | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: Should we move it above the UnmarshalKey function definition?
I always find easier to check for type definitions and var definitions in the top level of the file or above the function they are used.
| // skip any additional specifiers such as ",omitempty" or ",squash" | ||
| // TODO: support multiple specifiers | ||
| var specifiers map[string]struct{} | ||
| if commaPos := strings.IndexRune(tagtext, ','); commaPos != -1 { |
There was a problem hiding this comment.
Question: Could there be more than one comma in a field tag?
There was a problem hiding this comment.
Yes, leaving a comment above TODO: support multiple specifiers for now. I think implementing this could wait for a future PR as it's a slightly larger change.
pkg/logs/diagnostic/go.mod
Outdated
| @@ -111,3 +112,5 @@ require ( | |||
| gopkg.in/yaml.v2 v2.4.0 // indirect | |||
| gopkg.in/yaml.v3 v3.0.1 // indirect | |||
| ) | |||
|
|
|||
| replace github.com/DataDog/datadog-agent/pkg/config/structure => ../../../pkg/config/structure | |||
There was a problem hiding this comment.
Should we move the replace directive with the rest of replace directives?
There was a problem hiding this comment.
Oops, missed this one, yes it should be fixed.
pkg/logs/message/go.mod
Outdated
| @@ -97,3 +98,5 @@ require ( | |||
| gopkg.in/yaml.v2 v2.4.0 // indirect | |||
| gopkg.in/yaml.v3 v3.0.1 // indirect | |||
| ) | |||
|
|
|||
| replace github.com/DataDog/datadog-agent/pkg/config/structure => ../../../pkg/config/structure | |||
There was a problem hiding this comment.
Should we move the replace directive with the rest of replace directives?
|
Are you planning on replacing the rest of |
This comment has been minimized.
This comment has been minimized.
Yes. The hope is to get a few usages in place, and catch any problems that may be caused by any unintended changes in behavior. Eventually all of the uses of |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -598,6 +599,7 @@ require ( | |||
| github.com/DataDog/datadog-agent/comp/core/tagger/utils v0.56.2 | |||
| github.com/DataDog/datadog-agent/comp/otelcol/ddflareextension/def v0.56.0-rc.3 | |||
| github.com/DataDog/datadog-agent/comp/otelcol/ddflareextension/impl v0.0.0-00010101000000-000000000000 | |||
| github.com/DataDog/datadog-agent/pkg/config/structure v0.0.0-00010101000000-000000000000 | |||
There was a problem hiding this comment.
Is there a reason why we're using v0.0.0-00010101000000-000000000000 instead of v0.57.0 here?
There was a problem hiding this comment.
My understanding is that v0.0.0-00010101000000-000000000000 is a special version for new modules (this one was added in #28821) that don't have a released version yet. Because we're using replace directives, the build system will use the version from the source repo anyway.
|
📥 📢 Info, this pull request increases the binary size of serverless extension by 29056 bytes. Each MB of binary size increase means about 10ms of additional cold start time, so this pull request would increase cold start time by 0ms. Debug infoIf you have questions, we are happy to help, come visit us in the #serverless slack channel and provide a link to this comment. We suggest you consider adding the |
Serverless Benchmark Results
tl;drUse these benchmarks as an insight tool during development.
What is this benchmarking?The The benchmark is run using a large variety of lambda request payloads. In the charts below, there is one row for each event payload type. How do I interpret these charts?The charts below comes from The benchstat docs explain how to interpret these charts.
I need more helpFirst off, do not worry if the benchmarks are failing. They are not tests. The intention is for them to be a tool for you to use during development. If you would like a hand interpreting the results come chat with us in Benchmark stats |
| // skip any additional specifiers such as ",omitempty" or ",squash" | ||
| // TODO: support multiple specifiers | ||
| var specifiers map[string]struct{} | ||
| if commaPos := strings.IndexRune(tagtext, ','); commaPos != -1 { |
There was a problem hiding this comment.
Yes, leaving a comment above TODO: support multiple specifiers for now. I think implementing this could wait for a future PR as it's a slightly larger change.
pkg/logs/diagnostic/go.mod
Outdated
| @@ -111,3 +112,5 @@ require ( | |||
| gopkg.in/yaml.v2 v2.4.0 // indirect | |||
| gopkg.in/yaml.v3 v3.0.1 // indirect | |||
| ) | |||
|
|
|||
| replace github.com/DataDog/datadog-agent/pkg/config/structure => ../../../pkg/config/structure | |||
There was a problem hiding this comment.
Oops, missed this one, yes it should be fixed.
| @@ -133,6 +163,31 @@ feature: | |||
| assert.Equal(t, feature.Enabled, true) | |||
| } | |||
|
|
|||
| type FeatureConfigDiffCase struct { | |||
| ENaBLEd bool | |||
There was a problem hiding this comment.
Yes, I think that would be good. There's another PR in the work to specifically add more test coverage that @jeremy-hanna is working on. Private fields just simply be ignored by the unmarshaller.
pkg/logs/message/go.mod
Outdated
| @@ -97,3 +98,5 @@ require ( | |||
| gopkg.in/yaml.v2 v2.4.0 // indirect | |||
| gopkg.in/yaml.v3 v3.0.1 // indirect | |||
| ) | |||
|
|
|||
| replace github.com/DataDog/datadog-agent/pkg/config/structure => ../../../pkg/config/structure | |||
| @@ -598,6 +599,7 @@ require ( | |||
| github.com/DataDog/datadog-agent/comp/core/tagger/utils v0.56.2 | |||
| github.com/DataDog/datadog-agent/comp/otelcol/ddflareextension/def v0.56.0-rc.3 | |||
| github.com/DataDog/datadog-agent/comp/otelcol/ddflareextension/impl v0.0.0-00010101000000-000000000000 | |||
| github.com/DataDog/datadog-agent/pkg/config/structure v0.0.0-00010101000000-000000000000 | |||
There was a problem hiding this comment.
My understanding is that v0.0.0-00010101000000-000000000000 is a special version for new modules (this one was added in #28821) that don't have a released version yet. Because we're using replace directives, the build system will use the version from the source repo anyway.
| source, err := newNode(reflect.ValueOf(cfg.Get(key))) | ||
| rawval := cfg.Get(key) | ||
| // Don't create a reflect.Value out of nil, just return immediately | ||
| if rawval == nil { |
There was a problem hiding this comment.
setupAutoDiscovery in cmd/agent/common/autodiscovery.go expects a nil config setting to return no error. Adding this fix gets the new-e2e-aml test to pass.
|
/merge |
|
🚂 MergeQueue: pull request added to the queue The median merge time in Use |
What does this PR do?
This PR begins using the new
UnmarshalKeymethod, which was implemented by #28821, in a few callsites selected arbitrarily. The oldUnmarshalKeyworked by usingyaml.Unmarshalandmapstructure.Unmarshalwhich each relied on the config being able to return its internal data as amap[interface{}]interface{}. We want to break that assumption in the config, to use a new node-based approach. The newUnmarshalKeyuses this approach and is implemented purely using reflection.This PR also changes the new
UnmarshalKeyimplementation to be case-insensitive, as is required by the config.This new method should have the same functionality as the old one, but at the same time we want to shrink the config's API and simplify its semantics. For call-sites that rely on edge-case behaviors, that behavior is opted into using an Option.
Edge cases:
Motivation
Improving the config API
Additional Notes
Possible Drawbacks / Trade-offs
It's possible more edge cases exist and this new
UnmarshalKeysubtly changes semantics, any such changes should be considered bugs.Describe how to test/QA your changes
Behavior is covered by unit tests, behavior should be identical to before.