Skip to content

Exposed stats to watchdog actions. #13151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
htuch merged 4 commits into from
Sep 23, 2020
Merged

Exposed stats to watchdog actions. #13151

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e9fd61e
Exposed stats to watchdog actions.
KBaichoo Sep 1, 2020
c10a3e8
Merge remote-tracking branch 'upstream/master' into wde-stats-expose
KBaichoo Sep 18, 2020
5d16690
Merge remote-tracking branch 'upstream/master' into wde-stats-expose
KBaichoo Sep 21, 2020
bc670e3
Added stats to profile_action. Exposed guarddog name to the actions.
KBaichoo Sep 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/envoy/server/guarddog_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "envoy/event/dispatcher.h"
#include "envoy/protobuf/message_validator.h"
#include "envoy/server/guarddog.h"
#include "envoy/stats/scope.h"

#include "common/protobuf/protobuf.h"

Expand All @@ -19,6 +20,7 @@ namespace Configuration {
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)

@htuch htuch Sep 21, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@antoniovicente antoniovicente Sep 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@htuch htuch Sep 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@antoniovicente antoniovicente Sep 22, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@htuch htuch Sep 22, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinion as long as it happens :)

@KBaichoo KBaichoo Sep 22, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do a clean up of this then as a follow up.

};

class GuardDogAction {
Expand Down
2 changes: 1 addition & 1 deletion source/server/guarddog_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ GuardDogImpl::GuardDogImpl(Stats::Scope& stats_scope, const Server::Configuratio

// We should be able to share the dispatcher since guard dog's lifetime
// should eclipse those of actions.
Configuration::GuardDogActionFactoryContext context = {api, *dispatcher_};
Configuration::GuardDogActionFactoryContext context = {api, *dispatcher_, stats_scope};
Comment thread
KBaichoo marked this conversation as resolved.
Outdated

const auto& actions = config.actions();
for (const auto& action : actions) {
Expand Down
2 changes: 2 additions & 0 deletions test/extensions/watchdog/abort_action/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ envoy_extension_cc_test(
"//include/envoy/server:guarddog_config_interface",
"//source/extensions/watchdog/abort_action:abort_action_lib",
"//source/extensions/watchdog/abort_action:config",
"//test/common/stats:stat_test_utility_lib",
"//test/test_common:utility_lib",
"@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
"@envoy_api//envoy/extensions/watchdog/abort_action/v3alpha:pkg_cc_proto",
Expand All @@ -41,6 +42,7 @@ envoy_extension_cc_test(
"//include/envoy/server:guarddog_config_interface",
"//source/extensions/watchdog/abort_action:abort_action_lib",
"//source/extensions/watchdog/abort_action:config",
"//test/common/stats:stat_test_utility_lib",
"//test/mocks/event:event_mocks",
"//test/test_common:utility_lib",
"@envoy_api//envoy/extensions/watchdog/abort_action/v3alpha:pkg_cc_proto",
Expand Down
4 changes: 3 additions & 1 deletion test/extensions/watchdog/abort_action/abort_action_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "extensions/watchdog/abort_action/abort_action.h"
#include "extensions/watchdog/abort_action/config.h"

#include "test/common/stats/stat_test_utility.h"
#include "test/test_common/utility.h"

#include "absl/synchronization/notification.h"
Expand All @@ -28,8 +29,9 @@ class AbortActionTest : public testing::Test {
protected:
AbortActionTest()
: api_(Api::createApiForTest()), dispatcher_(api_->allocateDispatcher("test")),
context_({*api_, *dispatcher_}) {}
context_({*api_, *dispatcher_, stats_}) {}

Stats::TestUtil::TestStore stats_;
Api::ApiPtr api_;
Event::DispatcherPtr dispatcher_;
Server::Configuration::GuardDogActionFactoryContext context_;
Expand Down
4 changes: 3 additions & 1 deletion test/extensions/watchdog/abort_action/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "extensions/watchdog/abort_action/config.h"

#include "test/common/stats/stat_test_utility.h"
#include "test/mocks/event/mocks.h"
#include "test/test_common/utility.h"

Expand Down Expand Up @@ -40,9 +41,10 @@ TEST(AbortActionFactoryTest, CanCreateAction) {
)EOF",
config);

Stats::TestUtil::TestStore stats_;
Event::MockDispatcher dispatcher;
Api::ApiPtr api = Api::createApiForTest();
Server::Configuration::GuardDogActionFactoryContext context{*api, dispatcher};
Server::Configuration::GuardDogActionFactoryContext context{*api, dispatcher, stats_};

EXPECT_NE(factory->createGuardDogActionFromProto(config, context), nullptr);
}
Expand Down
2 changes: 2 additions & 0 deletions test/extensions/watchdog/profile_action/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ envoy_extension_cc_test(
"//source/common/profiler:profiler_lib",
"//source/extensions/watchdog/profile_action:config",
"//source/extensions/watchdog/profile_action:profile_action_lib",
"//test/common/stats:stat_test_utility_lib",
"//test/mocks/event:event_mocks",
"//test/test_common:environment_lib",
"//test/test_common:simulated_time_system_lib",
Expand All @@ -44,6 +45,7 @@ envoy_extension_cc_test(
"//include/envoy/server:guarddog_config_interface",
"//source/extensions/watchdog/profile_action:config",
"//source/extensions/watchdog/profile_action:profile_action_lib",
"//test/common/stats:stat_test_utility_lib",
"//test/mocks/event:event_mocks",
"//test/test_common:utility_lib",
"@envoy_api//envoy/extensions/watchdog/profile_action/v3alpha:pkg_cc_proto",
Expand Down
6 changes: 4 additions & 2 deletions test/extensions/watchdog/profile_action/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "extensions/watchdog/profile_action/config.h"

#include "test/common/stats/stat_test_utility.h"
#include "test/mocks/event/mocks.h"
#include "test/test_common/utility.h"

Expand Down Expand Up @@ -42,9 +43,10 @@ TEST(ProfileActionFactoryTest, CanCreateAction) {
)EOF",
config);

Stats::TestUtil::TestStore stats;
Event::MockDispatcher dispatcher;
Api::ApiPtr api = Api::createApiForTest();
Server::Configuration::GuardDogActionFactoryContext context{*api, dispatcher};
Api::ApiPtr api = Api::createApiForTest(stats);
Server::Configuration::GuardDogActionFactoryContext context{*api, dispatcher, stats};

EXPECT_CALL(dispatcher, createTimer_(_));
EXPECT_NE(factory->createGuardDogActionFromProto(config, context), nullptr);
Expand Down
7 changes: 5 additions & 2 deletions test/extensions/watchdog/profile_action/profile_action_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "extensions/watchdog/profile_action/config.h"
#include "extensions/watchdog/profile_action/profile_action.h"

#include "test/common/stats/stat_test_utility.h"
#include "test/mocks/event/mocks.h"
#include "test/test_common/environment.h"
#include "test/test_common/simulated_time_system.h"
Expand All @@ -35,8 +36,9 @@ class ProfileActionTest : public testing::Test {
protected:
ProfileActionTest()
: time_system_(std::make_unique<Event::SimulatedTimeSystem>()),
api_(Api::createApiForTest(*time_system_)), dispatcher_(api_->allocateDispatcher("test")),
context_({*api_, *dispatcher_}), test_path_(generateTestPath()) {}
api_(Api::createApiForTest(stats_, *time_system_)),
dispatcher_(api_->allocateDispatcher("test")), context_({*api_, *dispatcher_, stats_}),
test_path_(generateTestPath()) {}

// Generates a unique path for a testcase.
static std::string generateTestPath() {
Expand Down Expand Up @@ -76,6 +78,7 @@ class ProfileActionTest : public testing::Test {
outstanding_notifies_ -= 1;
}

Stats::TestUtil::TestStore stats_;
std::unique_ptr<Event::TestTimeSystem> time_system_;
Api::ApiPtr api_;
Event::DispatcherPtr dispatcher_;
Expand Down