Exposed stats to watchdog actions.#13151
Conversation
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
| struct GuardDogActionFactoryContext { | ||
| Api::Api& api_; | ||
| Event::Dispatcher& dispatcher_; // not owned (this is the guard dog's dispatcher) | ||
| Stats::Scope& stats_; // not owned (this is the server's stats scope) |
There was a problem hiding this comment.
FYI, this is inconsistent with other factory contexts, where we use pure virtual functions. By having pure interfaces, we give implementations flexibility on how they want to back each of these attributes.
There was a problem hiding this comment.
I see CommonFactoryContext in include/envoy/server/factory_context.h
I wonder if we should prefer passing that in. We do need to pass in the dispatcher associated with the guarddog since there are multiple watchdog instances, each of which needs to keep it's own list of actions.
@htuch what is your recommendation?
There was a problem hiding this comment.
In theory, it would be ideal to pass in a minimal factory context, since the provider doesn't need to back it with unused features. In practice, most of these factory contexts now have a lot in common, e.g. compare TransportFactoryContext with CommonFactoryContext. I'd say that if it's trivial to replace with CommonFactoryContext at the site-of-instantiation, then go with a subclass of this for now. I'd add the subclass even if empty, to prevent future interface churn for 3rd party devs.
There was a problem hiding this comment.
Changing this to an interface seems like a good cleanup. Do you have an opinion about doing it in this PR, as a PR merged before this one or as a followup PR?
There was a problem hiding this comment.
No strong opinion as long as it happens :)
There was a problem hiding this comment.
Will do a clean up of this then as a follow up.
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
antoniovicente
left a comment
antoniovicente
left a comment
There was a problem hiding this comment.
Changes LGTM, waiting on feedback from htuch on the FactoryContext
|
/retest |
|
Retrying Azure Pipelines, to retry CircleCI checks, use |
3 participants
Signed-off-by: Kevin Baichoo kbaichoo@google.com
For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md
Commit Message: Exposed Stats to Watchdog Actions.
Additional Description: This will allow them to create their own counters to expose for monitoring
Risk Level: low
Testing: unit tests
Docs Changes: N/A
Release Notes: N/A?