Simplify unmarshal logic by adding more supported hooks #4237
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4237 +/- ##
==========================================
+ Coverage 87.92% 88.05% +0.13%
==========================================
Files 173 173
Lines 10263 10200 -63
==========================================
- Hits 9024 8982 -42
+ Misses 993 977 -16
+ Partials 246 241 -5
Continue to review full report at Codecov.
|
* Add hook that supports "String -> encoding.TextUnmarshaler", e.g. zapcore.Level no longer need special unmarshaling
* Add hook that supports "String -> ComponentID"
* Add a special hook for map[string]interface{} -> map[ComponentID]interface{} to determine duplicates after space trimming, not sure if this error needs this special treatment.
Signed-off-by: Bogdan Drutu <[email protected]>
Depends on open-telemetry#4237 Signed-off-by: Bogdan Drutu <[email protected]>
There was a problem hiding this comment.
Not sure this is part of this PR, but here's how the error messages are presented to the users after this PR:
$ go run ./cmd/otelcol --config config/configunmarshaler/testdata/invalid-exporter-name-after-slash.yaml
Error: cannot load configuration: error reading top level configuration sections: 1 error(s) decoding:
* error decoding 'exporters': name part must be specified after / in type/name key
2021/10/21 14:16:58 collector server run finished with error: cannot load configuration: error reading top level configuration sections: 1 error(s) decoding:
* error decoding 'exporters': name part must be specified after / in type/name key
exit status 1
Can we do better? The error is that "exporter/" doesn't contain a name and it requires the user to parse the output (which seems duplicate) to figure that out. The closest to helping the user is "name part must be specified after / in type/name key" but that's also a bit cryptic, IMO.
Also, and now more specific to this PR: there seems to be a missing test coverage for the new functions.
config/configmap.go
Outdated
| // This is needed in combination with stringToComponentIDHookFunc since the NewComponentIDFromString may produce | ||
| // equal IDs for different strings, and an error needs to be returned in that case, otherwise the last equivalent ID | ||
| // overwrites the previous one. | ||
| func mapStringToMapComponentIDHookFunc() mapstructure.DecodeHookFunc { |
There was a problem hiding this comment.
Why don't you just define the func as var, instead of having a new func per invocation of mapStringToMapComponentIDHookFunc()?
There was a problem hiding this comment.
Also, there seems to be no coverage for this.
There was a problem hiding this comment.
Done the var part for all funcs, even though not that important since this happens during initialization, but I like to follow best practices :)
There was a problem hiding this comment.
In terms of testing, the "duplicate" ComponentID is tested in the defaultunmarshaler/duplicate_foo tests.
bogdandrutu
force-pushed
the
usemorehooks
branch
from
60b010d to
5d822f7
Compare
|
/easycla |
Signed-off-by: Bogdan Drutu <[email protected]>
bogdandrutu
force-pushed
the
usemorehooks
branch
from
5d822f7 to
b62c6a2
Compare
| @@ -1,8 +1,8 @@ | |||
| receivers: | |||
| examplereceiver: | |||
| exporters: | |||
| exampleexporter/exp: | |||
| exampleexporter/ exp : | |||
| exampleexporter /exp : | |||
There was a problem hiding this comment.
Why the changes in the duplicate yaml test files?
There was a problem hiding this comment.
Adding spaces in more parts of the name to ensure we do the right thing.
Signed-off-by: Bogdan Drutu <[email protected]>
bogdandrutu
force-pushed
the
usemorehooks
branch
from
b62c6a2 to
92e2c29
Compare
|
@codeboten PTAL |
Depends on open-telemetry#4237 Signed-off-by: Bogdan Drutu <[email protected]>
Depends on #4237 Signed-off-by: Bogdan Drutu <[email protected]>
Signed-off-by: Bogdan Drutu [email protected]