You may want to name this: main_thread_watchdog, let's see what the API shepherds say.

Yeah, prefer main, we don't really talk about "aux" elsewhere.

Done. Going with main_thread_watchdog as suggested.

I would have named this: WatchdogConfigs watchdog_configs

Let's see what the API shepherds say.

Generally we prefer singular unless it's a repeated, but we have precedence for doing what you suggest earlier in the file, e.g. StaticResources. How about just Watchdogs watchdogs = ....

Going with Watchdogs watchdogs as suggested. Thanks!

If neither set of parameters is passed in, should we default to separate guarddogs or single guarddog?

For backwards compatibility, defaulting to the current behavior (single guard dog even if no params are set), makes sense, which is what this would do.

A possible issue I see is that we'ld see a default behavior change when removing the old config parameters in the future. I think either option is fine.

Good point. We can go with separate guard dogs with default configs that will be similar to the single guard dog case.

Once we've found good parameters that work for the different subsystems, we can do the behavior change then (and the change will be smaller)

It seems like a mistake to configure both multi_watchdog and watchdog_config, we may want to reject config in that case.

Good point, we should inform the end-user that both of those fields being set doesn't make sense.

It seems we can go about this in one of two ways:

1. Warn here, and have a preference of one of the configs if both are set.
2. Do validation such as in:

   envoy/source/server/server.cc

   Line 435 in 82e611e

   if (initial_config.admin().address()) {

   and explicitly throw and error (there's no cleaner way of propagating the information AFAIK).

Thoughts?

throw an exception if config is invalid.

Done.

For backwards compatibility, defaulting to the current behavior (single guard dog even if no params are set), makes sense, which is what this would do.

Good point, we should inform the end-user that both of those fields being set doesn't make sense.

It seems we can go about this in one of two ways:

1. Warn here, and have a preference of one of the configs if both are set.
2. Do validation such as in:

   envoy/source/server/server.cc

   Line 435 in 82e611e

   if (initial_config.admin().address()) {

   and explicitly throw and error (there's no cleaner way of propagating the information AFAIK).

Thoughts?

Requiring callers to interpret the results of calling a method by consulting a different method seems like an API smell. Would it be possible to get rid of watchdogConfig() and require callers to always treat auxWatchdogConfig() and workerWatchdogConfig() as distinct, even if they actually return the same config?

The multiWatchdog() was meant more as a lighter weight method to figure out whether we're using the single watchdog or the multiwatchdog system. I could remove it if you'd like, but then I'd have the 2/3 places with something like:

if(watchdogConfig.empty()) : do multi watchdog config... else: do single watchdog

which is a bit confusing since I'm using either the negation of the other system being present or making multiple function calls looking for the presence of two optionals having values.

The main awkwardness with just having auxWatchdogConfig() and workerWatchdogConfig() is in the single guarddog for the system case there's going to be some function call like:

guarddog = make_unique<..>(config())

and using either auxWatchdogConfig() or workerWatchdogConfig() as the config() call would be confusing (even though they would be the same); alternatively, I could try to multiplex one of these calls but that would add more complexity to a single function.

The problem is that there's no way to perfectly emulate the single watchdog case with multiple watchdogs, right?

Yep. Shouldn't be an issue now though with always having multiple guarddogs in the system.

Since the values of these fields are so tied together, consider using a type that enforces the invariants you want. Here, for instance, multi_watchdog_ tells you which of watchdog_ and (aux_watchdog_, worker_watchdog_) is valid. Consider using absl::variant as a type-safe union that will subsume these (and save some space!).

I thought about this for a while...

Using an absl::variant but it seems like they're be a good amount of boilerplate since I'd have to wrap the aux_watchdog_ and worker_watchdog_ together somehow.

I could alternatively jump to using two watchdogs systems (vs the single watchdog system), while still supporting the single watchdog field. It'd be much easier to "emulate" most of the functionality of the single watchdog system using the two watchdog system.

This would make things a lot cleaner, but would break any cross-watchdog policies (such as multikill triggering under a combination of main_thread_watchdog and worker_watchdog), but that functionality doesn't seem too helpful.

I'm leaning towards the latter.

I think the only boilerplate would be an additional struct MultiWatchDog { std::unique_ptr<Watchdog> aux_watchdog, worker_watchdog; }; that you'd template the variant with, plus maybe a constructor definition to make emplacing easier. You could also use a std::pair but that seems error-prone since the types are the same. I guess that the call sites would be a little messier with absl::variant since absl::visit isn't super nice to use.

I agree with your leaning. If you can simplify here by emulating the single watchdog case, that seems preferable.

I've made the system always have two guard dogs which removes this issue.

If the user specifies the watchdog field (the old system), then we'll just have the two guarddogs have the same configuration.

nit: unless you need a std::string, use absl::string_view

Done.

Should this be deprecated?

Yeah, I'd be keen to not overlap mechanism long-term, so let's deprecate.

Done.

Existing monitoring for "server.watchdog_miss" will break for users that are configured using the current single watchdog parameter.

Is this acceptable? Worth mentioning i release notes?

I've added it to the release notes.

From a lot of the discussion with @akonradi, trying to maintain full backwards compatibility would end up with significant complexity within the system. Furthermore, as you pointed out in #13015 (comment), there will have to be some breakage at some point (either in the future or now).

Given those, I went for simplicity at the cost of minor behavior changes now.

Great.

Yeah we don't have a stats deprecation policy right now, so it's ad hoc and "best judgement" depending on how many people we think we will break. I think this is OK.

Not sure what section the monitoring changes should be added to. Consider adding to "Incompatible Behavior Changes" instead

Done.

nit: The newlines between the accessor methods is not required, consider removing them for consistency with other accessors above.

Done.

Yeah we don't have a stats deprecation policy right now, so it's ad hoc and "best judgement" depending on how many people we think we will break. I think this is OK.