refactor: `config.Duration`, and config use and printing #920

ThinkChaos · 2023-03-10T04:17:55Z

I started these changes while working the hosts file resolver, and wanted to make a PR before I do the same to other resolvers, if there's interest.

The config.Duration change is to try and make using the type easier: with an embed all original methods are available.
The tradeoffs are:

defaults need to be parsed from a JSON string, which has to be escaped: struct { x Duration `default:"\"1h\""` }
converting to time.Duration is a bit less explicit: x.Cast() vs time.Duration(x). That could be x.Duration, but I thought that was awkward to read (ex: cfg.ConnectionCooldown.Duration).

That first change allowed me to not copy the values out of the config.HostsFileConfig into the HostsFileResolver since using the config.Duration directly is easy.
I think this is nice because it makes adding config options faster. And finding all references to an option/renaming it is instant. Depending on how we end up doing config reloading, it could help too: update a single field and trigger a refresh.

The config printing I moved to the config struct itself, instead of the resolver. And changed it to print via a logger instead of returning an array.
I think that's valuable so we can make what's shown more or less verbose depending on the log level.

Let me know if what's of interest, if any, and I can adapt the PR.
The first change can be merged as is. The other two, I'd apply the idea to more resolvers.
I also didn't take the time to fix the tests for the third change.

Allows directly calling `time.Duration` methods.

The idea is to make adding configuration options easier, and searching for references straight forward.

Using a logger allows using multiple levels so the whole configuration can be printed in trace/verbose mode, but only important parts are shown by default.

codecov · 2023-03-10T04:21:42Z

Codecov Report

Patch coverage: 94.65% and project coverage change: -0.03 ⚠️

Comparison is base (32fe1eb) 92.87% compared to head (2271dae) 92.85%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #920      +/-   ##
==========================================
- Coverage   92.87%   92.85%   -0.03%     
==========================================
  Files          49       63      +14     
  Lines        5378     5389      +11     
==========================================
+ Hits         4995     5004       +9     
  Misses        311      311              
- Partials       72       74       +2

Impacted Files	Coverage Δ
config/config.go	`76.49% <18.18%> (-11.92%)`	⬇️
config/caching.go	`50.00% <50.00%> (ø)`
redis/redis.go	`93.23% <50.00%> (+1.44%)`	⬆️
resolver/bootstrap.go	`92.66% <71.42%> (-2.80%)`	⬇️
resolver/ede_resolver.go	`87.09% <83.33%> (+19.82%)`	⬆️
config/duration.go	`85.71% <85.71%> (ø)`
config/blocking.go	`88.09% <88.09%> (ø)`
config/client_lookup.go	`100.00% <100.00%> (ø)`
config/conditional_upstream.go	`100.00% <100.00%> (ø)`
config/custom_dns.go	`100.00% <100.00%> (ø)`
... and 27 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

config/duration.go

resolver/bootstrap.go

0xERR0R · 2023-03-10T07:32:26Z

server/server.go

@@ -443,8 +443,17 @@ func (s *Server) printConfiguration() {
 	for res != nil {
 		logger().Infof("-> resolver: '%s'", resolver.Name(res))

-		for _, c := range res.Configuration() {
-			logger().Infof("     %s", c)
+		if resCfg, ok := res.(resolver.ConfigGetter); ok {


I think the idea here is to replace "Configuration" with new "LogValues" method in all resolvers?

Yeah exactly, just did it to a single resolver as a proof of concept for now.
The ConfigGetter interface would not exist, and we'd have the Cfg method directly in Resolver instead of the current Configuration.
I could also rename Cfg to Configuration since that name would be free.

I removed ConfigGetter and just went with config.Configurable (which is the new name for ValueLogger).
With that rename, I added IsEnabled() bool to the interface.

Cfg() was replaced by LogConfig(*logrus.Entry). That allows having a single interface that covers both the config types, and the resolvers.

0xERR0R · 2023-03-10T07:38:52Z

server/server.go

+		if resCfg, ok := res.(resolver.ConfigGetter); ok {
+			func() {
+				undo := log.PrefixMessages("     ", logger().Logger)
+				defer undo()


Is it possible to create a new logger instance with prefix formatter? I mean a new instance only for configuration logging. I think, it is a bit "dangerous" to modify the existing instance and to rollback this change after usage. Later someone can use this logic somewhere else and forget to call the undo function.

Yeah I can do that too, this was just a bit easier with the existing log code!

So I tried changing this, but using a new logger loses stuff like fields that were added.
I changed the API to be impossible to misuse though, so hopefully that solves the issue with another approach!

Edit: also using the existing logger allows nesting the prefixes, which I've used in config/blocking.go.

resolver/custom_dns_resolver.go

log/logger.go

0xERR0R · 2023-03-10T07:58:38Z

Good job, thank you 👍 🤝

I started these changes while working the hosts file resolver, and wanted to make a PR before I do the same to other resolvers, if there's interest.

I think, we should unify all resolver implementation (like config printing etc).

The config.Duration change is to try and make using the type easier: with an embed all original methods are available. The tradeoffs are:

defaults need to be parsed from a JSON string, which has to be escaped: struct { x Duration `default:"\"1h\""` }

converting to time.Duration is a bit less explicit: x.Cast() vs time.Duration(x). That could be x.Duration, but I thought that was awkward to read (ex: cfg.ConnectionCooldown.Duration).

Please see my review comments.

That first change allowed me to not copy the values out of the config.HostsFileConfig into the HostsFileResolver since using the config.Duration directly is easy. I think this is nice because it makes adding config options faster. And finding all references to an option/renaming it is instant. Depending on how we end up doing config reloading, it could help too: update a single field and trigger a refresh.

In your case, it is absolutely ok to use the host configuration directly. We should avoid to pass a reference to the whole configuration to a resolver to avoid some side effects (for example a resolver A changes a value directly in the config and resolver B uses it). The configuration handling must be reworked completely for dynamic reloading. I though we need something like a service which provides the configuration and notifies about changes. That means each resolver calls something like "configService.getConfiguration" and receives current configuraiton...

The config printing I moved to the config struct itself, instead of the resolver. And changed it to print via a logger instead of returning an array. I think that's valuable so we can make what's shown more or less verbose depending on the log level.

That looks reasonable, please check my comment regarding the logger instance. Please also keep in mind, the "old" Configuration function in each resolver has not only printed the configuration, but also the internal state (like count of elements in cache).

Let me know if what's of interest, if any, and I can adapt the PR. The first change can be merged as is. The other two, I'd apply the idea to more resolvers. I also didn't take the time to fix the tests for the third change.

👍

ThinkChaos · 2023-03-10T14:21:40Z

Thanks for taking a look so fast, I'll make the changes you proposed and do more resolvers!

ThinkChaos

Updated all resolvers, their configs and all tests!

ThinkChaos · 2023-03-11T05:14:23Z

server/server.go

+		if resCfg, ok := res.(resolver.ConfigGetter); ok {
+			func() {
+				undo := log.PrefixMessages("     ", logger().Logger)
+				defer undo()


So I tried changing this, but using a new logger loses stuff like fields that were added.
I changed the API to be impossible to misuse though, so hopefully that solves the issue with another approach!

Edit: also using the existing logger allows nesting the prefixes, which I've used in config/blocking.go.

config/duration.go

ThinkChaos · 2023-03-11T22:53:02Z

Pushed some more commits, main change there is adding Type() string to Resolver. It's used as the log prefix, and when printing the config at startup.

I also tweaked the startup config so it's nicer (that's obviously subjective, so let me know if there's anything you'd like to improve).
Here's a comparison, left is before, right after:

kwitsch · 2023-03-12T00:03:45Z

I also tweaked the startup config so it's nicer (that's obviously subjective, so let me know if there's anything you'd like to improve).

I'm in favor of the new structure. 👍

kwitsch

Enhances readability & streamlines functionality 👍

0xERR0R

Great Job! Looks very clean 🚀 👍

ThinkChaos added 3 commits March 9, 2023 22:43

refactor: make config.Duration a struct with time.Duration embed

87eb01b

Allows directly calling `time.Duration` methods.

refactor(HostsFileResolver): don't copy individual config items

36a2078

The idea is to make adding configuration options easier, and searching for references straight forward.

refactor: move config printing to struct and use a logger

c64899b

Using a logger allows using multiple levels so the whole configuration can be printed in trace/verbose mode, but only important parts are shown by default.

0xERR0R reviewed Mar 10, 2023

View reviewed changes

0xERR0R added the 🧰 technical debts Technical debts, refactoring label Mar 10, 2023

0xERR0R added this to the v0.21 milestone Mar 10, 2023

ThinkChaos added 12 commits March 10, 2023 12:46

squash: rename Cast to ToDuration

baf7d11

squash: revert Duration to a simple wrapper ("new type" pattern)

2102f98

squash: Duration.IsZero tests

0a36d6b

squash: refactor resolvers to rely on their config directly if possible

2586e4c

squash: implement IsEnabled and LogValues for all resolvers

c63e7b1

refactor: use go-enum --values to simplify getting all log fields

637cdbf

refactor: simplify QType unmarshaling

8578f22

squash: rename ValueLogger to Configurable

e832996

squash: rename UpstreamConfig to ParallelBestConfig

c647374

squash: rename RewriteConfig to RewriterConfig

b7b8e3d

squash: config tests

937ddc8

squash: resolver tests

813ce81

ThinkChaos commented Mar 11, 2023

View reviewed changes

ThinkChaos added 7 commits March 11, 2023 11:02

squash: add ForEach test and improve Chain ones

8899e75

squash: simplify implementing config.Configurable

41772d2

squash: minor changes for better coverage

3cd8d34

squash: more UnmarshalYAML -> UnmarshalText

2af8a30

refactor: move config.Upstream into own file

a995d96

refactor: add Resolver.Type method

462ab0b

squash: add log method to typed to use Resolover.Type as prefix

ba56021

ThinkChaos force-pushed the refactor/config-duration-and-print branch from 061c2a5 to 5943434 Compare March 11, 2023 22:17

ThinkChaos added 2 commits March 11, 2023 17:24

squash: tweak startup config logging

47de06b

squash: add LogResolverConfig tests

0ea058f

ThinkChaos force-pushed the refactor/config-duration-and-print branch from 5943434 to 47de06b Compare March 11, 2023 22:30

squash: make sure all options of type Duration use %s

2271dae

kwitsch approved these changes Mar 12, 2023

View reviewed changes

0xERR0R approved these changes Mar 12, 2023

View reviewed changes

0xERR0R merged commit 5088c75 into 0xERR0R:main Mar 12, 2023

ThinkChaos deleted the refactor/config-duration-and-print branch March 13, 2023 16:46

ThinkChaos mentioned this pull request May 14, 2023

Configuration option "fqdnOnly" is not recognized #1024

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: `config.Duration`, and config use and printing #920

refactor: `config.Duration`, and config use and printing #920

ThinkChaos commented Mar 10, 2023

codecov bot commented Mar 10, 2023 •

edited

Loading

0xERR0R Mar 10, 2023

ThinkChaos Mar 10, 2023

ThinkChaos Mar 11, 2023

0xERR0R Mar 10, 2023

ThinkChaos Mar 10, 2023

ThinkChaos Mar 11, 2023 •

edited

Loading

0xERR0R commented Mar 10, 2023

ThinkChaos commented Mar 10, 2023

ThinkChaos left a comment

ThinkChaos Mar 11, 2023 •

edited

Loading

ThinkChaos commented Mar 11, 2023

kwitsch commented Mar 12, 2023

kwitsch left a comment

0xERR0R left a comment

refactor: config.Duration, and config use and printing #920

refactor: config.Duration, and config use and printing #920

Conversation

ThinkChaos commented Mar 10, 2023

codecov bot commented Mar 10, 2023 • edited Loading

Codecov Report

0xERR0R Mar 10, 2023

Choose a reason for hiding this comment

ThinkChaos Mar 10, 2023

Choose a reason for hiding this comment

ThinkChaos Mar 11, 2023

Choose a reason for hiding this comment

0xERR0R Mar 10, 2023

Choose a reason for hiding this comment

ThinkChaos Mar 10, 2023

Choose a reason for hiding this comment

ThinkChaos Mar 11, 2023 • edited Loading

Choose a reason for hiding this comment

0xERR0R commented Mar 10, 2023

ThinkChaos commented Mar 10, 2023

ThinkChaos left a comment

Choose a reason for hiding this comment

ThinkChaos Mar 11, 2023 • edited Loading

Choose a reason for hiding this comment

ThinkChaos commented Mar 11, 2023

kwitsch commented Mar 12, 2023

kwitsch left a comment

Choose a reason for hiding this comment

0xERR0R left a comment

Choose a reason for hiding this comment

refactor: `config.Duration`, and config use and printing #920

refactor: `config.Duration`, and config use and printing #920

codecov bot commented Mar 10, 2023 •

edited

Loading

ThinkChaos Mar 11, 2023 •

edited

Loading

ThinkChaos Mar 11, 2023 •

edited

Loading