feature: watch include directory

[jonatan5524]

Use the LoadConfig to load the config and compare it in each interaction of watch and larger the interaction to 3 seconds.

Resolve #3517

[mholt]

This is an interesting approach, so instead of watching individual files' mod times, you're just relying on LoadConfig() to imply when one of the included config files has changed by comparing the bytes? And it uses less code in the process?

That's not a bad idea.

[jonatan5524]

This is an interesting approach, so instead of watching individual files' mod times, you're just relying on LoadConfig() to imply when one of the included config files has changed by comparing the bytes? And it uses less code in the process?

That's not a bad idea.

Yes! Thank you!
I saw the LoadConfig is combining the main config file with it's included, if anything changes in any file we detect it.
I tested it with simple config files, Do you think it will be much slower with bigger config files?

[mholt]

Is this to alleviate CPU and I/O load for large configs?

I wonder if 1-2s is still fast enough for most people.

[jonatan5524]

I wonder this myself, Do you have maybe a large config file to test this approach?

[mholt]

I mean, I have a fast CPU and SSD. I would never use the watch feature in production (but I know some people do). I don't know how busy those users' production servers are.

In local development, you probably want a fast refresh time like 1s. In production, 5s is probably just fine.

I'm willing to bet this is used locally more than in prod (but without telemetry it's impossible to be sure, sigh) -- so maybe we keep it 1s and then wait for complaints?

[jonatan5524]

I mean, I have a fast CPU and SSD. I would never use the watch feature in production (but I know some people do). I don't know how busy those users' production servers are.

In local development, you probably want a fast refresh time like 1s. In production, 5s is probably just fine.

I'm willing to bet this is used locally more than in prod (but without telemetry it's impossible to be sure, sigh) -- so maybe we keep it 1s and then wait for complaints?

Aha agree 😉

[mholt]

Alrighty. Well let's change the interval back to 1s and then we can merge it, give this a try :) Thanks!

[francislavoie]

I think we should adjust the caddy run CLI docs to mention in more detail how --watch works, i.e. to mention it polls every second, loading & parsing the config to compare if it has changed since the last load.

[jonatan5524]

I think we should adjust the caddy run CLI docs to mention in more detail how --watch works, i.e. to mention it polls every second, loading & parsing the config to compare if it has changed since the last load.

But do you really think the user that sees the cli "help" command cares how the watch is implemented inside? maybe mention the 1s check in the docs and how it is implemented before the watchConfigFile method?

In start and in run commands:

cmd.Flags().BoolP("watch", "w", false, "Reload changed config file automatically, check every 1s for a change")

In run long:

If --watch is specified, the config file will be loaded automatically after
changes (check every 1 second). ⚠️ This can make unintentional config changes easier; only use this
option in a local development environment.

And before the watchConfigFile function:

// watchConfigFile watches the config file at filename for changes
// and reloads the config if the file was updated. This function
// blocks indefinitely; it only quits if the poller has errors for
// long enough time. The filename passed in must be the actual
// config file used, not one to be discovered. 
// Each second the config files is loaded and parsed into an object 
// and is compared to the last config object that was loaded
func watchConfigFile(filename, adapterName string) {

[mholt]

Personally, I think with a quick refresh rate like this, the implementation is irrelevant to the user and I don't think we need to expose this. If it was a longer refresh (maybe if it was deemed expensive), then perhaps we should document that so the user doesn't wonder why their config didn't apply until several seconds later.

However, if we still feel like exposing that information is a good idea, perhaps it can go in a sentence on our docs website.

Edit: In the end, I'm indifferent about whether/where we document it. As long as documenting it doesn't cause people to rely on the specific implementation (i.e. is not a binding contract). If people start relying on the 1s scan thing, then I'd rather we not document it.

@jonatan5524 This is looking great, thanks.

I had the idea chatting about this with Francis in our dev Slack that maybe the refresh rate could be determined by the size of the last config. If we assume that configs won't (frequently) change in size by an order of magnitude, maybe the bigger a config is, the longer the refresh rate is.

But if we do decide to do that, it could go in a subsequent PR. My vote is let's try the 1 second and see if that is too fast/slow for anyone in a real use case first.

[francislavoie]

CI is failing for some weird reason. I think the caches are busted. I'm gonna try to fix that... working on it.

[mholt]

Actually I'm gonna just take out the implementation details from the CLI docs since I don't want to be bound to them, in case it changes, and I'd rather users don't rely on it by accident.

I think this is ready then!

[mholt]

It's perfect now 💯 -- let's try it out, and we can tune/optimize later, as discussed. (For example, auto-tuning the interval based on config size)